[WIP] Enable nested VoxelModifiers

[sjkillen]

Regarding #555

I was unsure of what to set the transform to initially when the mesh is parented as global transforms aren't accessible. It seems that NOTIFICATION_TRANSFORM_CHANGED is called after the parenting notification.

This implementation appears to work with both the mesh and sphere modifiers. Nested modifiers will react to parent transformations

[sjkillen]

It doesn't seem to work right if some nodes are saved to a separate scene, I suspect this is because of how NOTIFICATION_PARENTED works and that it is called before a node is attached to the full scene tree

[Zylann]

Are modifiers getting updated when the terrain is moved? They should not (and that's one big reason I didn't do this)

[Zylann]

In case the modifier already is registered in the terrain, then you can get a valid transform, because modifier transforms are relative to the terrain. It doesn't matter whether they are in the tree or not. And once that's done, it should not have to be set again, until the modifier or any of its parents below the terrain are changed.
So I'm not sure you have to set a wrong transform here... also keep in mind transform updates that end up with a post_edit have a cost that can add up to the terrain's initial update.

[sjkillen]

For the very last part, do you mean the updates should be batched initially somehow?

[Zylann]

What I mean is that the fact the terrain gets added to the scene tree may already trigger updates on its own, but if modifiers also trigger individual updates on top of that, it could mean unnecessary updates depending on the timing. Although I'm not certain that this really matters, it might already have been a problem before your PR.

[Zylann]

Minor, this is now more expensive even for direct children. At least maybe it should take a shortcut if direct child.

[Zylann]

Parenting logic should not be in transform notifications. Other code might break unexpectedly if no transformations occur.

[sjkillen]

It seems that the PARENTING notification is fired for packed scenes before they are attached to the main scene, so the volume is never found and the modifier is not initialized.
As a workaround, I moved the parenting code to the transform update. I suspect that a transform notification will always be fired after a parenting event because nodes do not have a global transform before they exist in the tree, however, I could be wrong about this. It appears to work in my tests.

[Zylann]

I would still not rely on the transform notification for parenting, at least not alone, because it's not what it's for... Alternatives are to either make a special getter function that attempts to get the volume each time it is found null, or to defer the whole logic to cases where the volume enters the scene tree, but that also has annoying drawbacks such as reparenting the terrain causing total remeshing for no reason...

[Zylann]

Why should configuration warnings update when the transform changes?

[sjkillen]

Ah, sorry. This was an artifact from copying the parenting code over to the transformation notification. Does it only need to be run if the modifier was added to the terrain?

[Zylann]

It may run only when the conditions checked are affected, so currently, only when _volume changes.

[Zylann]

I believe this is called everytime any parent of the modifier is moved. If the terrain or any of its parents move, we actually don't want modifiers to get updated. Imagine a planet with a hundred craters made with modifiers. If the planet moves in space (as is the case in the Solar System demo), that would cause all of the craters to remesh every frame, even though they actually dont change locally. That's one reason I didnt implement nesting originally.

[sjkillen]

I added a commit that will check whether the transform that is local to the terrain has changed, however, because the computation of this transform relies on global transforms, there is some floating point error and it will sometimes update modifiers when the terrain is moved.
Would it be acceptable to measure some epsilon difference between transforms and only update if transforms differ by that amount? I'm not sure if there are helpers for this sort of comparison in Godot.

[Zylann]

I thought of this too, unfortunately at this point it already required computing a bunch of transforms unnecessarily, which can also suffer from floating point precision issues so the check might still fail

[sjkillen]

I observed some floating point errors causing unnecessary updates. Do you think it would work to check whether the transforms differ by some small amount instead of checking for equality?

[Zylann]

I would rather prefer nothing to happen at all if possible... because even when doing this, those 100 craters I talked about in another comment would then have to constantly invert, multiply and compare matrices every frame for no reason. It seems like supporting nesting requires extra complexity/workarounds I didnt want to add originally (especially if direct children have to pay the same cost). I don't know how else it can be solved. I'm also not aware of nodes in Godot that have transform-dependent logic getting registered all the way to a specific Node3D parent like this, or usually each node in the hierarchy is aware of the system (like CSG nodes).

[sjkillen]

A few more revisions,
In particular, I tried to make the code as close to the current for the case of immediate children.
Immediate children of terrain nodes don't need to check for floating point error and use their local transform rather than computing the nested local transform

I extracted the logic to locate the volume and added a function to compare transforms using an epsilon so that moving the terrain does not update nested modifiers.
I didn't know where to put this math code, so it's in the same file - there is also still a WARN message for each time the modifier is updated

[sjkillen]

Perhaps since there is a bit of an additional cost for the immediate child case, it would make sense to give a warning suggesting immediate children be used?