Transforms are broken -- tracking issue

[icefoxen]

This has been the case for a while and I thought "oh it's just a quick fix" and I put it off forever and tried to mess with it yesterday and couldn't fix it so it's obviously not quick so I need to stop putting it off and give it its own issue.

Basically, in Image::draw(), the offset value is one way or another not being applied correctly.

There's also the various breakage with canvases and such, and in general all the drawing transform code needs to be looked over to make sure it Actually Works Right.

This is irritating since it's kinda spread out in various places and I don't understand how it composes well enough, so blah.

[icefoxen]

shear should probably be a Vector2 also.

[icefoxen]

Made a transforms example to have a nice way to play with this. Results:

• dest and offset are in units of pixels
• scale, shear and src are in units of image sizes

We want to be able to support both but not at the same time.

Shear can probably be gotten rid of safely tbh.

As mentioned in #439 (comment) , offset and scale combine incorrectly, and the docs on offset units are incorrect. The following math for DrawTransform::from::<DrawParam>() appears to fix that part of it:

let transform = translate * offset * rotation * offset_inverse * shear * scale;

[icefoxen]

Text with gfx_glyph appears to not be affected by graphics::set_screen_coordinates().

Things also get screwy as heck with dpi involved, sigh...

This is what I get drawing labels and points every 100,100 pixels on an 800x600 window on a display with dpi factor of 1.5...

[icefoxen]

@termhn You're way better at math than me. I'll buy you a cake if you help me with this, 'cause by now it's all some twisted mess and I can't figure out what's happening where, let alone what should be happening where.

I spent over an hour messing around with making the transform offset work in the range of [0-1] instead of in pixel units and have concluded that I have no earthly idea how it ever worked in the first place.

[termhn]

😅 IDK about that, I'll try to take a look soon though.

[icefoxen]

Okay, so per #439 there's issues with the offset matrix calculation:

translate * offset * rotation * scale * offset_inverse

(shear omitted for simplicity)

and it should be

translate * offset * rotation * offset_inverse * scale

In 0.4 that code is here, in 0.5 it is in the same file but slightly different place.

So I implemented this change in 0.5 and there's TWO issues

First, the offset isn't scaled properly by the size of the image, which is adjusted here.

0.4 has MOST of the desired behavior: The image is drawn from the top-left corner minus offset, rotation starts from the offset point, and offset is in the range of [0-1]. However, per #439 there are some bugs with that when using a custom coordinate system (and scaling?) which need to be looked in to more.

0.5 currently has Very Wrong behavior: The image is drawn from the top-left corner ignoring offset (or ignoring scale???), rotation starts from the offset point, and offset is in units of pixels (times whatever scale factor exists). However, if I return it to the old way (offset_inverse multiplied after everything else) we still get wrong behavior: the image is drawn from the top-left corner + offset, rotation starts from the offset point, and offset is in unit of pixels (not multiplied by scale).

Also, in both versions, Image::draw() does some fiddling with scale to make it in units of pixels. In 0.4:

        let real_scale = Point2::new(
            src_width * param.scale.x * self.width as f32,
            src_height * param.scale.y * self.height as f32,
        );
        let mut new_param = param;
        new_param.scale = real_scale;

In 0.5:

        let real_scale = nalgebra::Vector3::new(
            src_width * f32::from(self.width),
            src_height * f32::from(self.height),
            1.0,
        );
        let new_param = param.mul(Matrix4::new_nonuniform_scaling(&real_scale));

These are very different operations and that might be a source of problems, and the whole DrawTransform thing might have been ill-guided since we cannot easily pull an angle or a scale out of a Matrix4 and modify them.

Well, progress? I need to actually write down everywhere that a transformation to the coordinate system happens in this silly system.. Frankly the transform stack system doesn't really make that any easier, though in this case I'm not even using it at all so it does nothing; it just adds another thing going on to filter out.

[icefoxen]

Image is partially fixed in the undo-drawtransform branch, commit e698801 . There is one obvious error that I can't find the cause of; in 0.4 if you draw something twice with the same coordinates but different offsets, it offsets the drawing as well as any rotation origin:

In devel currently that doesn't happen. Which is weird since the math should be functionally identical?

[icefoxen]

Fixed that at least, in an ad-hoc way, in commit 27e40bd . Text and canvases need fixing.

[icefoxen]

The text stuff is happening because resizing the screen doesn't touch GraphicsContext.screen_rect, and that's what the text transform is going off of. Not sure how to fix it yet; I don't think resizing the screen should touch screen_rect 'cause it's set by the user. It should use GraphicsContext.projection instead.

Also beware that Text implements Drawable and thus Text::draw() is not quite identical to draw_queued_text().

[Lokathor]

So I tried to fiddle with this a bit and I looked at old matrix math tutorials and ended up with

let transform = translate * offset * rotation * scale * offset_inverse;

And it looks like what I think should be happening when I run the translations demo. The image rotates around it's "center" point while being scaled down and offset from one of the grid points.

link to my fork: https://github.com/Lokathor/ggez

[icefoxen]

Oh right, part of the pain in the ass is that DrawTransform is not actually helpful. Sigh. Image needs to fiddle with the scaling factors to make it so that one texel in the image lines up with one pixel on the screen, while also having the coordinates for the offset term be in units of 0 to 1. Which is quite impractical with everything squashed down into a matrix. Sooooo all that work is getting ripped out.

[icefoxen]

Resolving #493 will fix this for Text, since it currently just uses a SpriteBatch for text drawing and everything goes through the same graphics pipeline as Image does. Ideally. That is almost done, so that just leaves Canvas.

[icefoxen]

With 19a5df0 I'm pretty sure this is closed now. \o/ The shadows example doesn't work, since it heckin' breaks even on minor changes and this has been anything but minor, but that'll get fixed as part of cleanup.