Unexpected order of operations when combining flip and offset

[jwlarocque]

I noticed that when using the flip options, offsets are applied before the flip. In my opinion this is unintuitive and I would propose either changing it (since the feature is relatively new, this hopefully wouldn't break too many workflows) or documenting the behavior in the readme.

Relevant code:

vpype-gcode/vpype_gcode/gwrite.py

Lines 133 to 145 in 1a74dec

	document.translate(offset_x, offset_y)
	invert_x = config.get("invert_x", False)
	invert_y = config.get("invert_y", False)
	flip_x = config.get("horizontal_flip", False)
	flip_y = config.get("vertical_flip", False)
	# transform the document according to inversion parameters
	if invert_x or invert_y:
	document = invert_axis(document, invert_x, invert_y)
	if flip_x or flip_y:
	document = invert_axis(
	document, flip_x, flip_y, whole_page=True, unit_scale=unit_scale
	)

Example of current behavior (gcode)

SVG 175mm tall, with a line segment in the top left: ((10, 10), (30, 10))

profile toml:

offset_x = 45
offset_y = 45

---

When vertical_flip = false, I get this gcode, as expected:

G0 X55 Y55
G0 X75 Y55

When vertical_flip = true, I would expect:

G0 X55 Y210
G0 X75 Y210

but instead I get:

G0 X55 Y120
G0 X75 Y120

(The expected result can be achieved with offset_y = -45.)

[abey79]

@tyehle Any opinion on this?

[abey79]

@jwlarocque Thanks for the feedback. IIRC, we had some discussion on ordering already. Moving offset after invert_x/y would definitely break existing workflow. I'm not sure what would be the consequences of moving flips before the offset.

[tatarize]

This seems accurate. I would expect the translate to have an absolute rather than a flipped effect. If we apply it the way we do it's x+offset and then x*-1 which actually gives us -x - offset.

Though really we might need a better arranged solution altogether since you might want some other things. And frankly this might be better off as part of vpype more properly. Something like CSS matrix parsing would make sense with a couple extra operations. translate(1in, 0) flip_x could be parsed and executed just as flip_x translate(1in, 0) could but the difference is a bit more obvious.

Changing the order of the static operations would always change how it works. Unless for some reason it multiplied by 0 or something, or otherwise couldn't matter. But flips, inverts, and offsets are equal to scale(-1, 1, cx, cy), scale(-1,1) and translate(ox, oy).

[abey79]

I like this idea of explicitly listing transforms in order!

This is how it could look like in the toml config file:

[gwrite.demo]
# ...
# scale_x = 10    # dont use, depreciated, still works as before
# offset_x = ...  # same
# flip_x = true   # same
transforms = [
    { type = "scale_x", value = 10 },
    { type = "flip_y", value = true },
    { type = "offset_x", value = "3cm" },
]

The operations would of course be applied in the listed order. The old parameters would remain and keep their effect, but be deprecated (non 0 value yield warning, removed altogether after a while).

[tatarize]

Seems like flip_y wouldn't really need a value. And that formatting feels a bit hamstrung.

[gwrite.demo]
# ...
# scale_x = 10    # dont use, depreciated, still works as before
# offset_x = ...  # same
# flip_x = true   # same
transforms = [
    "scale_x(10)",
    "flip_y",
    "offset_x(3cm)",
]

or, more straightforward:

[gwrite.demo]
# ...
# scale_x = 10    # dont use, depreciated, still works as before
# offset_x = ...  # same
# flip_x = true   # same
transform = "scale_x(10) flip_y offset_x(3cm)"

It seems like you could even just inline that as a quoted things transform = "scale_x(10) flip_y offset_x(3cm)" Though strictly speaking that's not something that would need to be restricted to gwrite since you might well have a need to apply that kind of arbitrary string of translations to other places. It's actually pretty close to how svg works. In fact, since there's a dependency on svgelements you could even just do Matrix("scaleX(10) scaleY(-1) translate(3cm, 0)") and that would work. Though clearly the flip_x and flip_y aren't part of that since Matrix doesn't know the document size to flip it across the center, but the operations that would be needed for vpype would be pretty straight forward.

See: https://github.com/meerk40t/svgelements/blob/fcf3bdd8997ca48b19fa63487cef42fa0d7d12d1/svgelements/svgelements.py#L2649 for the basic parsing routine.

[abey79]

I like the readability and conciseness of your first proposal (TOML array but SVG-like transform syntax). It somehow strikes a nice balance between self-explanatory, not too hacky in the context of a TOML config file, trivial parsing code (basically a regexp), and not too verbose (like my initial proposal).

IMHO, it wouldn't make sense to force this into direct compatibility with svgelements. The loss in extensibility/readability isn't worth the implementation gains.

[tatarize]

I was noting that parsing there is usable prior art, not that we should take it. There is no function in CSS matrix that allows flipping a document which is largely what you we want here. Also, a quick parser for the relevant transforms could probably do either several inline operations or singletons as you could send "translate(20, 40) scale(1.5) translate(-20,-40)" just as easily as the singletons.

In vpype-gcode we're calling commands like:

    document.scale(scale_x / unit_scale, scale_y / unit_scale)
    document.translate(offset_x, offset_y)
    document.translate(-origin[0], -origin[1])
    ...
    document.scale(-1 if invert_x else 1, -1 if invert_y else 1)
    document.translate(origin[0], origin[1])

        class DummyDocument:
            def __getattribute__(self, item):
                print(item)
                return print

        import re
        parse = "translate(2in, 3in) flip_x flip_y rotate(3deg) skew(1.2, 1.8)"
        transform_re = re.compile(r"(?u)(scale|translate|rotate|skew|flip_x|flip_y)[\s\t\n]*(?:\(([^)]+)\))?")
        param_re = re.compile(r"([-+]?[0-9]*\.?[0-9]+(?:[eE][-+]?[0-9]+)?)[\s\t\n]*(cm|mm|in|deg)?")
        document = DummyDocument()
        for m in transform_re.findall(parse):
            name = m[0]
            params = [(float(mag), unit, mag + unit) for mag, unit in param_re.findall(m[1])]
            if name == "scale":
                assert(len(params) == 2)
                document.scale(params[0][0], params[1][0])
            elif name == "translate":
                assert(len(params) == 2)
                document.translate(params[0][2], params[1][2])
            elif name == "rotate":
                assert (len(params) == 1)
                document.rotate(params[0][0])
            elif name == "skew":
                assert (len(params) == 2)
                document.skew(params[0][0], params[1][0])
            elif name == "flip_x":
                assert (len(params) == 0)
                document.flip_x()
            elif name == "flip_y":
                assert (len(params) == 0)
                document.flip_x()

Is a basic parser. I think things like document.translate() can take "2in", "3in" sorts of things. Given the dummy document this output:

translate
2in 3in
flip_x

flip_x

rotate
3.0
skew
1.2 1.8

Obviously it could take whatever units the documents take for those operations, or just hand it floats if needed. But, in theory that would work.

Then we leave the rest of the stuff untouched, deprecate the forced position flip_x or invert stuff. Not sure if this is better just a short-hand part inside document or restricted to vpype-gcode, this generally is just calling some document functions and you could probably add in some non-transform calls if there's some useful ones to use there.

---

With the addition of percent as a length (equal to percent of document size) it would be possible to single-line the flip_x flip_y stuff. Namely you'd allow the command scale(-1, 1, 50%, 50%) Which means do a scale based on the given position. And the position would be 50%, 50% which is 50% * document_width and 50% * document_height. Though you might break that into translate(-50%, -50%), scale(-1,1) and translate(50%, 50%) (Note: Several inline operations would use the original and not each subsequent value for the % so scale(0.9,0.9, 50%, 50%) could be seen differently after the scale and the second translate could be based on original or new size).

You could add a document.matrix() function, though we could probably just issue them one at a time in sequence. Though it would imply that rather than gcode_write having the whole flip_x thing programmed into it that maybe that should be done in vpype.

def transform(self, mx):
        for line in self._lines:
            line.real = line.real * mx.a + line.imag * mx.c + 1 * mx.e
            line.imag = line.real * mx.b + line.imag * mx.d + 1 * mx.f

Assuming the matrix bits were a-f, when really a numpy array would also tend to work there. We'd do this for speed, generally it doesn't come up that much but all the affine transformations can be applied within the matrix and then we only multiply the giant set of numbers the one time. But, I'd assume we'd stick to regular parsing, and stick to only modifying vpype-gcode. That little bit of code is pretty easy to setup.

[abey79]

Great input thanks–we could definitely do something like this.

Now, to shake things up, here is a completely different approach that just occurred to me: use actual vpype commands instead this new DSL of sort.

In the TOML:

[gwrite.demo]
# ...
transform = "translate 0 10cm scale -o 0 %vp_page_width.h/2% 1 -1 scale 0.5 0.5"

(The param would probably be better named preprocess instead of transform.)

This would be actual vpype commands, so the implementation becomes a one-liner:

doc = vpype_cli.execute(preprocess_commands, doc)

This would be nice because (1) it's trivial to implement, (2) builds on existing user knowledge instead of introducing yet another DSL, (3) easily extensible by construction.

I see at least these issues (there may be more):

1. scale -o 0 %vp_page_width.h/2% 1 -1 for flip is extremely ugly. This could be solved by introducing new fliph/flipv commands in vpype, which would be useful anyway.
2. The scale command doesn't actually scale the page size, just the contents. Adding this to the scale command would be easy/possibly useful, but that would need an explicit option for backward compatibility. Alternatively, seeing that scaling is mainly used either for flipping (as per (1)) or changing units, a new command could be introduced to deal with the latter (units cm?). This would look weird in vpype in general, but be convenient in this particular context.
3. I wonder if I'd be at risk of some re-entrancy issue with execute() (effectively runs a whole vpype pipeline) launch while executing a vpype pipeline?

[tatarize]

I think that's a winner.

If you don't call gwrite with the pre-process, it shouldn't be much of a problem. Though if you did that you should probably crash.

That does seem pretty simple and effective. Even the parsing section, I realized things like reloop could be trivially added. And well, most of that stuff is or should be in the vpype pypeline anyway. We make a copy of the document during saving so we're not modifying it directly. Even the parsing stuff feels like, if it was implemented, it should be in vpype directly. Your default unit is ppi=96 pixels or whatever, and setting the units there is basically just a unit scale and that would also work a bit better in vpype there.

You'd need to make units as a command actually track the units throughout. So if I say units in and then I make a rectangle that is 1 1 2 2 and then do units px to restore back to regular it needs to know I was in in and went back to px rather than doing the scaling for px->in and then px->px which would leave me in inches.

Also, since the goal of this project more or less is to make the project not exist, more utility moving from vpype-gcode to to vpype is quite helpful. A lot of these things feel like it's pulling stuff out of vpype-gcode in to vpype proper and if most of the options were dealt with it might be reasonable or plausible to move it more directly into just write which already does a sort of hybrid svg hpgl thing.

This gets a little weirder since invoking gwrite with a profile that preprocess a write command would, in theory, let you use gwrite for every type of write... which puts things a bit closer to endgame. Since if it was sort of just a write command instead it might actually be somewhat more mergable. Though it might need to be save or something.