Use true pitch/yaw/roll rotations without loss of precision by pgimeno

[p-ouellette]

From https://gitlab.com/pgimeno/minetest/merge_requests/3/

Store the rotation in the node as a 4x4 transformation matrix internally (through IDummyTransformationSceneNode), which allows more manipulations without losing precision or having gimbal lock issues.

Network rotation is still transmitted as Eulers, though, not as matrix. But it will stay this way in 5.0.

Fixes #7927

EDIT by paramat: Applies implementation discussed from #7927 (comment) onwards.
Details #7927 (comment)

[ghost]

Thanks for submitting it!

[paramat]

Thanks.
This needs careful consideration and is a complex issue, see linked issue.

[stujones11]

This looks good to me and is pretty much the 'officially' endorsed solution to this common problem. While doing this is fine for your main game actor, my only minor concern would be the creation of an additional dummy scene node for every single game object.

I doubt that it will be a problem but we really should try to get some numbers on this.

[ghost]

I doubt that it will be a problem but we really should try to get some numbers on this.

I don't notice any difference with a bunch of objects simultaneously visible, but I don't have a particularly slow computer. My measurement attempts have been hindered by total lack of vsync control. With normal vsync I get 60 fps; setting __GL_SYNC_TO_VBLANK=0 I get 62 FPS. I filled up my inventory and started digging, and got the scene below. No matter where I looked I got 62 FPS. Only when I looked at forests with lots of leaves, I got a drop in FPS, but that drop isn't related to the patch.
Edit: Ahh, got it, it was the MaxFPS. No noticeable difference.

[stujones11]

I don't have a particularly slow computer

I would consider a better test to be spawn 1000+ objects, with and without this PR and then compare memory usage and other vital stats.

[paramat]

Tested with my original boats patch #7395 (comment)
Rotation for the boat is now an intuitive 'intrinsic' yaw-pitch-roll.
Left-right for yaw, forward-back for pitch and jump-sneak for roll.

[paramat]

As the large majority of objects in MT will only use yaw (dropped items, most mobs), might there be benefit (would it even be possible?) in dividing objects into yaw-only and yaw-pitch-roll versions? Adding a new object property to select between those.

[stujones11]

dividing objects into yaw-only and yaw-pitch-roll versions?

Although rather messy, we could simply have a boolean property to enable 6d rotation and only add the matrix node for those objects. Hopefully, this should not be necessary though.

[ghost]

Yes, it would indeed get messy. A bunch of new methods would be necessary to abstract the operations that are currently performed sometimes on the object scene node and sometimes on the transformation node, so that they can work both with and without the latter transparently. That would make the patch larger and more invasive.

Considering that at some point, Irrlicht will fix this on their side (hopefully in not many years), keeping the patch as simple as possible will help reverting the changes when that happens.

As for memory and other vital stats, I'll defer testing to whoever knows what and how to measure.

[paramat]

Ok thanks, i was suspecting it would create too much mess, and there may not be performance issues anyway.

[ghost]

Added unit tests. @pauloue could you fetch/push please?

[p-ouellette]

There was a conflict in networkprotocol.h. That's the reason for the force push. If that's a problem you can do the rebase on your branch.

[ghost]

Thanks, yes I had a rebased version ready locally, waiting for someone to ask me to rebase. I wanted to remove a line there actually, so I've force-pushed my gitlab branch. You may need to delete your local branch before fetching, not sure.

[nerzhul]

except minor changes, can is good for me. Anyway untested

[paramat]

One thing i need to test is using a "cube" or "item" visual entity and checking the correct face is treated as the 'front'.

[ghost]

I've tested the dynamic updates under the same conditions. Without limiting the number of entities, the results were as follows: 42-44 FPS on master, 38-40 FPS on this branch. The numbers oscillated, which is expected due to interference between the frequencies of the server ticks and the client frames. As a consequence, this result is a bit less accurate. The extra calculation time is in this case 1/39 minus 1/44, approx. 2.9 ms for 1023 entities. Given the bigger measurement error, I think that sounds about the same. That matches my expectations.

The FPS I expected was actually worse; my calculation was a decrease by a factor of 41.8/(41.8+11.1) = 79% which means 33 FPS (all numbers are approximate). Perhaps the server isn't sending all updates to the client due not all entities being in front of the player. The camera wasn't in the same position as the previous test, even though it was in the same position during this testing; maybe that affected the results somehow too.

I'm satisfied with these results, so I'll consider it tested ant call it a day.

(Edited blunder with the way I presented the numbers, apologies)

[paramat]

Performance seems good, it tests well, what this implements is good and seems a widely accepted solution, lots of attention which i appreciate. So once i check codestyle i think all this counts for something and i will probably +1.
However, i do not completely understand some aspects, so i will request that the core dev who gives the 2nd +1 does.

[rubenwardy]

I'm so happy to see so many tests

How much testing has this had, in practice? Using old worlds and new, on different clients, etc

[ghost]

I've tested only in new worlds. I don't use MT5 to play, so I can't say it's field-tested. I haven't tried migrating old worlds. However, the protocol change introduced by this patch only affects entities that have a nonzero Z rotation, and is compatible otherwise. Therefore the only potential problem I foresee is with those people using a version which includes #7395 and #7989 and who already used a rotation with a Z component != 0. The existing entities in those conditions (if there are any out in the field, which I doubt) will have a different rotation; the rest will keep compatibility. This PR does not affect those migrating from stable-0.4.

[ghost]

In case it helps understanding the PR, here's a diagram of what the patch implements:

---

Without patch:

---

With patch:

[paramat]

Looked through code, partially understand it 👍

Resolved.

[ClobberXD]

The diagram helps a lot. I can understand how it works now. Neat. :)

But upon looking through the changes, I notice that the rotations follow a different coordinate system (X: forward-backward, Y: left-right, Z: up-down), whereas all spatial vectors in MT follow the right-handed system (X: left-right, Y: up-down, Z: forward-backward). This would cause quite a bit of confusion for modders. Is there a reason why rotations need to follow the proposed coordinate system?

[ghost]

@ClobberXD There seems to be a confusion. The code is designed to be able to work with MT's axis conventions. The convention you point out is that of the native Irrlicht ISceneNode rotations, which caused #7927 (see there for details) and the whole problem that this PR is aimed to work around. Note how the ISceneNode rotation is kept at 0 with the patch, so it doesn't interfere with the different, Minetest-friendly convention introduced by setPitchYawRoll().

I may have made a mistake, though. If so, could you please let me know which specific part of the code uses Z for up?

(edit: wrong issue number fixed)

[ghost]

@ClobberXD I wonder if the source of confusion is that this patch is NOT in the 5.0.0 pre-release even if there was some discussion about including it. The 5.0.0 pre-release does behave the way you're pointing out.

(As an aside, in case you missed the edit, I wrote the wrong issue number above, the correct one is #7927)

[paramat]

I notice that the rotations follow a different coordinate system (X: forward-backward, Y: left-right, Z: up-down)

That's a confusing way to refer to rotations, how can a rotation be 'forward-backward' and what does that mean? Rotations refer to axes.
Can you specify a part of this code you are concerned about?

[ClobberXD]

@pgimeno Looks like I was indeed confused, and I misinterpreted stuff. Sorry for the trouble. Great work! 👍

That's a confusing way to refer to rotations, how can a rotation be 'forward-backward' and what does that mean? Rotations refer to axes.

I was referring to the axes and not the rotations itself, but I guess that doesn't matter now. :)

[paramat]

I'll merge this once that's done.

[ghost]

@pauloue could you update it please?

[p-ouellette]

Updated

[ghost]

Big thanks to @pauloue for hosting this PR.