Fixed definition inconsistencies and rotation matrix generation. Technically a breaking change!

[ThePythonator]

Changed matrix field declarations to reflect the logical matrix layout. Fixed rotation matrix generation so that it rotates the right way (anticlockwise)!

[Daft-Freak]

:thinking: I wonder how much this messes up the flight example (biggest user of Mat3::rotate in the examples). Something to check at least. (Also, geometry and tilemap-test though that probably just spins the demos the other way...)

[ThePythonator]

🤔 I wonder how much this messes up the flight example (biggest user of Mat3::rotate in the examples). Something to check at least. (Also, geometry and tilemap-test though that probably just spins the demos the other way...)

Looks like the flight example just creates the rotation matrices for one-time use, which can be replaced by Vec2's rotate method anyway. I haven't checked the other examples.

[Daft-Freak]

That's a good point, that would still result in rotating the other way though.

Anyway, I tried it and the trees end up rotating the other way to the ground... which this fixes:

diff --git a/32blit/graphics/mode7.cpp b/32blit/graphics/mode7.cpp
index ea9fe25e..7bf84dc3 100644
--- a/32blit/graphics/mode7.cpp
+++ b/32blit/graphics/mode7.cpp
@@ -75,13 +75,13 @@ namespace blit {
     forward *= Mat3::rotation(angle);

     Vec2 left = forward;
-    left *= Mat3::rotation((fov / 2.0f));
+    left *= Mat3::rotation(-(fov / 2.0f));

     Vec2 right = forward;
-    right *= Mat3::rotation(-(fov / 2.0f));
+    right *= Mat3::rotation((fov / 2.0f));

     float distance = ((far - near) / float(s.y - viewport.y)) + near;
-    
+
     Vec2 swc = pos + (left * distance);
     Vec2 ewc = pos + (right * distance);

(Yeah, still not using Vec2::rotate, kind-of pretending the mode7 code doesn't exist)

Then it's just a matter of the left/right inputs being swapped... (expected)

Seems geometry uses both Mat3::rotation and Vec2::rotate... and is negating the angle passed to one to compensate for this bug so now has the ship rotated the wrong way relative to the beam... Should be an easy fix?

[ThePythonator]

Aah, I changed the flight example code but couldn't work out why it still wasn't working properly. Didn't realise the mode7 code also used the rotation matrices.

Using the rotate method would be better, since it should be marginally faster (no creation of a Mat3 object, with the extra unnecessary fields, and less multiplication I think). Obviously it doesn't matter too much, but I feel that you kind of want the firmware code to be as fast as is reasonably possible.

[Daft-Freak]

Not disagreeing about rotate likely being faster, I just usually avoid mixing fixes with optimisations.

There's probably quite a bit that could be improved about the mode7 stuff, but I had enough problems just trying to use it that I wrote an entirely different one for Super Blit Kart. Anyway, that's a different issue involving things like the other map class...

[ThePythonator]

If I'm not mistaken, the two Mat3::rotation(angle) occurrences also need to be changed to Mat3::rotation(-angle)?

[ThePythonator]

Hopefully that fixes everything :)

[Daft-Freak]

If I'm not mistaken, the two Mat3::rotation(angle) occurrences also need to be changed to Mat3::rotation(-angle)?

Hmm, seems a bit inconsistent for mode7 to rotate the other way compared to doing it yourself. Also, you still need to swap left/right in screen_to_world to fix the trees.

[ahnlak]

While I agree that it's probably a good thing to fix what's logically broken, I'm also aware this is a change in behaviour and probably worth flagging loudly when it gets merged, so anyone else using Mat3::rotation in their own code isn't bitten by it.

Github search (such that it is) only throws up a couple of uses, mind...

[Daft-Freak]

Quick grep on the local repos the cupboard bot has: (only covers 32blit tagged repos)

./mikerr/32blit-games/xmas/xmas3d.cpp:    t *= Mat3::rotation(angle) ;
./mikerr/32blit-games/vector3d/vector3d.cpp:    t *= Mat3::rotation(angle) ;
./mikerr/32blit-games/3dcube/3dcube.cpp:    point *= Mat3::rotation(rot.x);
./mikerr/32blit-games/3dcube/3dcube.cpp:    point *= Mat3::rotation(rot.y);
./mikerr/32blit-games/3dcube/3dcube.cpp:    point *= Mat3::rotation(rot.z);

./mikerr/stickman/stickman.cpp:     point *= Mat3::rotation(rot.x);
./mikerr/stickman/stickman.cpp:     point *= Mat3::rotation(rot.y);
./mikerr/stickman/stickman.cpp:     point *= Mat3::rotation(rot.z);

./Loxrie/32blitman/main.cpp:  rotation *= Mat3::rotation(deg2rad(90));

... and 2/3 of those repos fail to build with the current SDK anyway.

But yeah, this is technically a breaking change.

[ThePythonator]

If I'm not mistaken, the two Mat3::rotation(angle) occurrences also need to be changed to Mat3::rotation(-angle)?

Hmm, seems a bit inconsistent for mode7 to rotate the other way compared to doing it yourself. Also, you still need to swap left/right in screen_to_world to fix the trees.

The trees seemed to be fine once I negated the angle in every rotation call... I can't try stuff out at the moment, but I have a feeling that swapping left/right has the same effect as negating the other rotations.

[Daft-Freak]

I did test it and the trees are definitely not attached to the ground. (Actually, I think it's the ground that's backwards... which makes sense as the the start/end coords of the scanline would be swapped).

[ThePythonator]

Think that fixes everything, and also avoids the nasty negatives in the calls to Mat3::rotation. Commits somehow got tangled up but it seems to work now. I'm sure @Gadgetoid will make it clear that this change may break a few projects when merged (but at least the rotations are technically correct and consistent now!).

[ThePythonator]

Did that fix it?