plottertools / vpype-gcode

gcode extension for vpype
MIT License
35 stars 7 forks source link

Invert document instead of geometry #28

Closed tyehle closed 1 year ago

tyehle commented 1 year ago

If the page size has not been set, then fall back to inverting just the geometry.

Possible solution to #25

tatarize commented 1 year ago

Now suffers a minor conflict with merged corrections for #26

I am inclined to merge this as well, however, I do have to classify fixing something like this as a breaking change. So 0.13.0

tyehle commented 1 year ago

Yeah, I think it would be possible to add a flag in the config (eg: invert_document or something) to turn on this behavior, but to me it seems that flipping the document what people would want almost all of the time.

abey79 commented 1 year ago

I like this and agree that this is what most people want in most cases. It will break 90+% of existing setups though (most physical coord systems are top-left/y-down). May I suggest adding a vertical_flip flag (along with a horizontal_flip flag for orthogonality). This wording might resonate best when people find y-inverted results on first try. This flag would work along side the existing invert_y (probably not the best idea to use both but why not), and would fail with error if page_size is undefined.

Advantages:

Drawback:

tyehle commented 1 year ago

@abey79 Changed implementation as you suggested, with horizontal_flip and vertical_flip config options, and fixed the hardcoded units mess by doing the flipping before any scaling.

abey79 commented 1 year ago

I have a small doubt with the order of operation. Prior to this PR, the axis inversion happened after the scaling/offsetting. Now the opposite is true. I haven't checked, but I believe that this will change the offset sign on axes where inversion/flipping happens. This is probably a breaking change.

tyehle commented 1 year ago

@abey79 you are right. I changed the order back to what it was before

tyehle commented 1 year ago

I think all the concerns about this have now been resolved. Is this good to merge?

abey79 commented 1 year ago

Yes, this looks good to me!