[3.x] Physics interpolation - Zero server side multimesh data

[lawnjelly]

To prevent possibility of use of uninitialized data.

Notes

• Noticed by @rburing in Physics interpolation: MultiMesh #91818
• I can't remember whether this problem can occur in the wild, but seems worth zeroing just in case. 👍

[rburing]

Is there some alternative that would avoid interpolating with zero as the starting value? For example the MRP in #91818 (comment) (in the first post, not in the first reply) prints the position of the first instance, and without interpolation it is:

(-2, 1, 0)
(-2, 2, 0)
(-2, 3, 0)
(-2, 4, 0)
(-2, 5, 0)
...

whereas with interpolation it is:

(-2, 0, 0)
(-1.92669, 0.963345, 0)
(-1.929336, 1.893236, 0)
(-1.929237, 2.822802, 0)
(-1.92924, 3.753382, 0)
...

The script just does vertical movement, so it seems at least the interpolation with zero at the start has "attracted" the instance to the origin.

[rburing]

I think there should be a reset_physics_interpolation that works for the whole buffer / all instances, and we should call this when the respective notification is received.

[lawnjelly]

I think there should be a reset_physics_interpolation that works for the whole buffer / all instances, and we should call this when the respective notification is received.

Potentially there could be.

The existing multimesh_instance_reset_physics_interpolation() can do all the different variations, so it's a question of whether the use case / demand will be convincing to add another specific command like multimesh_instances_reset_physics_interpolation() to make this simpler / more efficient.

Certainly I wouldn't be against adding it. I erred on the side of implementing the minimum viable product and then waiting for feedback before adding more features. And we are likely to get feedback a lot quicker for 4.x as there are many more users.

BTW I'm guessing you are hinting about having it auto-detect the first update and do a reset. Will write more but have to turn PC off as there is lightning storm here. ⚡

[rburing]

I wanted to add (using the Godot 4 syntax):

void RendererMeshStorage::multimesh_reset_physics_interpolation(RID p_multimesh) {
	MultiMeshInterpolator *mmi = _multimesh_get_interpolator(p_multimesh);
	if (mmi) {
		mmi->_data_prev = mmi->_data_curr;
	}
}

to reset all instances, and call it when MultiMeshInstance3D receives NOTIFICATION_RESET_PHYSICS_INTERPOLATION. This seems good, but it doesn't work when the node is first added to the tree, because the original buffer (loaded from disk) will have been set when interpolation was not yet active.

Adding the following workaround to the script then does fix the "attraction to zero" issue mentioned above:

func _ready():
        multimesh.buffer = multimesh.buffer
        reset_physics_interpolation()

I'm not sure what's the best way to make it work out-of-the-box.

Edit: The following would make the buffer reassignment automatic:

diff --git a/servers/rendering/storage/mesh_storage.cpp b/servers/rendering/storage/mesh_storage.cpp
index eb2640a9f1..0148176ade 100644
--- a/servers/rendering/storage/mesh_storage.cpp
+++ b/servers/rendering/storage/mesh_storage.cpp
@@ -261,7 +261,11 @@ void RendererMeshStorage::multimesh_set_buffer_interpolated(RID p_multimesh, con
 void RendererMeshStorage::multimesh_set_physics_interpolated(RID p_multimesh, bool p_interpolated) {
        MultiMeshInterpolator *mmi = _multimesh_get_interpolator(p_multimesh);
        if (mmi) {
+               bool was_interpolated = mmi->interpolated;
                mmi->interpolated = p_interpolated;
+               if (p_interpolated != was_interpolated) {
+                       multimesh_set_buffer(p_multimesh, multimesh_get_buffer(p_multimesh));
+               }
        }
 }

Now I just need to find why we need to call reset_physics_interpolation() manually.

Edit 2: Ah, I just didn't port the part that does it automatically on NOTIFICATION_ENTER_TREE for 3D.

Does this solution look reasonable? (We can still additionally zero the data, for people using servers.)

Edit 3: Oh, the automatic mechanism sending NOTIFICATION_RESET_PHYSICS_INTERPOLATION will probably run before the _refresh_interpolated() on MultiMeshInstance3D...

Edit 4: We could just hardcode an extra reset in MultiMeshInstance3D's NOTIFICATION_ENTER_TREE. One last question is whether RendererMeshStorage::multimesh_set_physics_interpolated should do the reset conditional on mmi or mmi && mmi->interpolated. The latter would make the first reset a no-op, but I don't know if it would have any downsides.

[lawnjelly]

This is really getting off topic for this PR, as the PR is just a safety to prevent reading uninitialized data rather than trying to solve resets. Reset discussion should really go in its own PR or proposal.

But to continue:
I'm not sure I would recommend trying to tack resetting multimesh instances onto the node reset (NOTIFICATION_RESET_PHYSICS_INTERPOLATION).
They do two different things:

• One resets the node transform
• One resets instance transforms

The instance transforms are defined in the node coordinate space, both are potentially interpolated and both should be capable of resetting independently. Resetting both together would lose a degree of freedom. Consider for example you might want to move a node, reset the node, but keep the instances interpolating in local space. Or you might wish to reset the instances but not reset the node transform.

Currently, internally you can just already just call multimesh_set_as_bulk_array_interpolated() with the previous and current arrays the same and it will do a reset on all the instances (although casual users might not be familiar with the internal format for set_as_bulk_array()). For casual users they would currently have to call multimesh_instance_reset_physics_interpolation() for each instance, it is doable, but a more convenient / efficient function could be added.

I (unsurprisingly 😁 ) did look quite a bit into doing an automatic reset when adding 3D nodes to the tree and experimented with it, but there were some situations in which it created problems (I forget the details, it may have been things like reparenting nodes within the tree without mucking up their interpolation, moving origins, and there can be issues with order of operations on loading / construction).

In the end I decided to postpone 3D resets on adding to the tree until we have a bit more testing in the wild, and all the potential problems ironed out. I'm a bit wary that in future we could end up having to add ugly workarounds as a result of jumping the gun on such automation.

Overall initially I would advise being cautious and implementing the minimum in general until we have further use data. Users will be overjoyed to have the basics working, and will be far better off waiting a little for us to get it right on the "icing on the top" features rather than rushing on the wrong solution and making things unworkable in the future.

[rburing]

Fair enough. Thanks for the explanation! I will continue with the minimalist approach.

[akien-mga]

Thanks!