Allow rotating entity selectionboxes

[appgurueu]

Ready for review. Closes #12372. Written with @runsy's petz in mind.

How to test

1. /spawnentity testentities:selectionbox
2. Point at it, punch & rightclick it a few times
3. Point at other (rotating) objects (items in particular) and observe that their boxes are unaffected
4. Other raycast testing (normals etc. should still work)

[MisterE123]

oh, this is so good. I hope it gets merged for 5.6. How does it affect walking on collisionboxes?

[appgurueu]

oh, this is so good. I hope it gets merged for 5.6. How does it affect walking on collisionboxes?

It doesn't affect collisionboxes because that's way more complex, only selectionboxes.

[appgurueu]

Carts may benefit from this as well: They are nonphysical either way and may be rotated by 45° when going uphill.

[ghost]

Yeah, this could be cool for entity selection.

[Andrey2470T]

Generally works fine. 👍 However, I noticed at high (even middle) rotation speed the selection boxes may a bit lag from their entities' rotation. Visually this appears in the following way:

[appgurueu]

I noticed at high (even middle) rotation speed the selection boxes may a bit lag from their entities' rotation.

Presumably the same issue as #11905 and thus somewhat out of scope for this PR to fix... There are two issues:

1. The broken rotation interpolation is only applied when drawing (after the pointed thing has been determined) - this does not seem to matter though and is mostly negligible;
2. For some reason gcao->getSceneNode()->getAbsoluteTransformation().getRotationDegrees() yields an outdated rotation despite being called after the Generic CAO's step has been called, which is definitely not how it's supposed to behave.

The fix for 2. is to simply call updateAbsolutePosition(). Latest commit fixes it.

[appgurueu]

automatic_rotate probably won't work serverside...

[appgurueu]

What about intersection normals: Should the Lua API return the rotated normal or the face normal as pointed_thing.intersection_normal? Up until this PR the two have been equal.

[x2048]

I've noticed that server-side rotation is slightly different from client-side rotation, even for a static entitiy (automatic_rotation = 0). Compare the cross shapes at the screenshots below:

@appgurueu

[appgurueu]

I've noticed that server-side rotation is slightly different from client-side rotation, even for a static entity (automatic_rotate = 0).

Was the entity static from the get go (in this case this would be a clear bug) or did you stop its automatic rotation after a while (then this would be expected to some extent)?

Edit: I've been able to reproduce this for edge cases (thin boxes, significantly rotated) even after setting the rotation by punching. This might be related to float precision issues. I suspect this is related to me not resetting the variable keeping track of the total automatic rotate properly.

[appgurueu]

After further consideration: This is indeed the error incurred through the server guessing automatic rotate without knowing when the client started applying it; an error of automatic rotate times RTT / stepsize is to be expected.

Setting automatic rotate to zero and then setting the rotation does not get rid of the rotation incurred through automatic rotate; automatic rotate only affects the rotation of the child node, which stays and is always additionally applied.

We can not break this behavior due to backwards compatibility.

Another caveat is that presumably this will reset when the entity is unloaded client-side. When the client comes back to the entity after a while, it will start with a fresh automatic rotate Y-axis rotation. In this case, client & server might experience a larger desync. In fact, different clients might see different total rotations incurred by automatic rotate - which one should the server use in its calculations?

It seems with the current protocol, we can not reliably know, but only guess the rotation of an entity with automatic_rotate != 0.

Some options:

• Keep as-is & document the aforementioned caveats; would work well enough in singleplayer
• Get rid of automatic_rotate-related code and jus document that using automatic_rotate + rotatable selectionbox won't work well serverside (similar to attachments) - modders can reimplement serverside raycasting including the automatic rotate guessing (which turns out to be a hack) themselves if they really want to
• Complicate protocol to sync automatic rotate; probably out of scope for this PR

[appgurueu]

Went with the first option. Note:

• Client-side, everything is still fine;
• For free/detached entities without automatic rotate, everything is still fine & consistent both server- and clientside;
• In singleplayer, the error of the server's "best guess" will usually be acceptable;
• The only cases where this really fails (excepting attachments) thus are:
  • singleplayer, rotated selectionbox + automatic rotate, resetting automatic rotate to 0 and starting it again later (as the test entities may do) such that the error adds up
  • multiplayer, rotated selectionbox + automatic rotate, automatic rotate desync may happen

[sfan5]

Needs rebase.

[appgurueu]

Rebased.

[ghost]

This is basic for the mob mods.

https://github.com/runsy/petz/issues/117

[ghost]

I think this should implement only in singleplayer.

[appgurueu]

I think this should implement only in singleplayer.

Please elaborate: Are you referring to rotatable selectionboxes in general (if so: why?) or do you mean taking automatic rotate into account specifically?

[ghost]

I think this should implement only in singleplayer.

Please elaborate: Are you referring to rotatable selectionboxes in general (if so: why?) or do you mean taking automatic rotate into account specifically?

I mean the preccision errors.

[appgurueu]

I think this should implement only in singleplayer.

Please elaborate: Are you referring to rotatable selectionboxes in general (if so: why?) or do you mean taking automatic rotate into account specifically?

I mean the preccision errors.

The precision errors are only relevant for automatic rotate. Disabling the approximation in multiplayer would require additional work and IMO be dirty. Adding the note for modders IMO is enough.

[ghost]

Can anyone more review this? please

@nerzhul @TurkeyMcMac @rubenwardy @SmallJoker @lhofhansl @savilli @x2048 @Df458

[TurkeyMcMac]

Works. Just some minor things.

[ghost]

With two approvals this should be merged, isn't it?

[appgurueu]

With two approvals this should be merged, isn't it?

Sometimes core devs like to let some time pass, but yes, this does now fulfill the requirements to be merged.

[ghost]

Sometimes core devs like to let some time pass, but yes, this does now fulfill the requirements to be merged.

I prefer a exhaustive review and checking than a faulty release.

Well, I guess this should be ready for 5.7 release.