-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Render G2/G3 arcs in 3D view by interpolating them as line segments #1092
Conversation
@IgorYeremin thank you for your contributions! @volconst could I have your thoughts on this and Igor's other two active PRs? They look good to me but I would like your opinion before I merge them. |
@kliment , Sorry for the delay, I have missed it's referencing me. |
Sure! I'm testing Marlin on a plasma machine, so here are gcode files generating using Fusion360. |
@IgorYeremin do you have uncommitted changes in your work dir, I am getting exception: Loading file: /home/.../test-fusion-plasma-arcs/test-fusion-plasma-m3.gcode |
I do not have uncommitted changes in my work dir. |
No, maybe the python class needs changes |
I am not using gcoder_line. |
Ah ok, didn't realize you can run without Cython, here's the copy function for PyLine |
mid = copy.copy(gline) | ||
mid.current_x = cx + math.cos(a) * r | ||
mid.current_y = cy + math.sin(a) * r | ||
yield (gline_idx, mid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying and translating G2 Arc commands and pretending they are lines is neither semantically, nor performance-wise desirable. Would make sense only if we have to fool some foreign interface we do not control. Fortunately the site of consumption of interpolate_arcs is in the same file.
Here it's seen that the only value/information that we add is tuple(current_x, current_y), which a better candidate for return value
I think the clean way is to add a separate inner iteration loop which eventually expands glines to points, so we have both access to the original gline and the interpolated points, without copying.
The indisputable advantage of the current setup is that it exists and works :)
@kliment Let's merge #1092, #1093, #1094 . |
@volconst okay, I'll merge them, thank you for reviewing |
No description provided.