-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Jalapeno/snapmaker z skip #14060
Jalapeno/snapmaker z skip #14060
Conversation
Hello at first glance I absolutely love these changes. I will properly review it as soon as possible, but the first impression is very good. The limit implementation, the EPSILON changes and the force_ method make a lot of sense. The only small issue is that I do not want to merge the documentation changes in this PR. Would you mind creating a separate PR for these (if you still want to merge these)? I am not a fan of listing the concrete tests in the markdown file as these are subject to change. But I do not want to discuss that here, as I do not want to block these great improvements by nitpicking about documentation. Also, it would be great if you could squash your commits to a few reasonable blocks (I would aim for 3 to 4 commits in total, whatever makes sense to you based on the changes). |
Thank you for your positive review.
Then I'll update #14060 to reduce the number of commits as outlined above. |
It seems that OrcaSlicer (forked from earlier PrusaSlicer) also suffers from this Z speed problem on Snapmaker 2.0... |
…XPORT_DIGITS and add unit tests prusa3d#13420 test_gcodewriter.cpp add tests for XYZF_EXPORT_DIGITS Check that movement GCodes are emitted / not emitted according to XYZF_EXPORT_DIGITS. The new tests demonstrate that XYZF_EXPORT_DIGITS is NOT respected. prusa3d#13420 GCodeWriter separate XYZ_EPSILON consistent with XYZF_EXPORT_DIGITS The value of EPSILON=1e-4 (0.0001) in libslicer3r.h is inconsistent with XYZF_EXPORT_DIGITS=3 (0.001). This change addresses the inconsistency by introducing XYZ_EPSILON which is computed from XYZ_EXPORT_DIGITS, and changing all relevant GCodeWriter functions which used EPSILON to use XYZ_EPSILON instead.
…ge get_travel_to_*_gcode() -> travel_to_*_force() prusa3d#13420 generate_ramping_layer_change_gcode() remove const const prevents calling GCodeGenerator::travel_to functions which change the m_pos internal state. prusa3d#13420 GCodeWriter add travel_to_XXX_force methods These methods allow travel moves to be made without checking the distance against XYZ_EPSILON, which may prevent the move from being emitted. This is useful when we want an important comment to be emitted, like a layer change. prusa3d#13420 replace get_travel_to_gcode with travel_to() get_travel_to_gcode() functions don't maintain m_pos, which later causes incorrect speed calculations as the movement vector is measured incorrectly. prusa3d#13420 GCodeWriter remove get_travel_to_gcode() Ensure that client code can't make moves without maintaining m_pos.
prusa3d#13402 test_gcodewriter.cpp add travel_speed_z tests These tests fail with the legacy implementation of GCodeWriter::travel_to_xyz_force() because it does not decompose the speed into it's components according to the movement unit vector. prusa3d#13420 Correct Z speed too high
9efbd3a
to
2699020
Compare
Hello, I have reviewed the changes and it is OK. Currently, we need to do some rounds of internal testing and after that we will hopefully (if all goes well) merge this 🎉 |
Thanks a lot. Merged in PrusaSlicer 2.9.1-alpha1. Closing. |
These changes address issue 14320.
See discussion at #13420 (comment)