Nested voxel modifiers

[sjkillen]

Is your feature request related to a problem? Please describe.

I have a post-import script that converts mesh instances into voxel modifier meshes. The modifiers do not work unless they are immediate children of a voxel terrain. If an imported scene contains multiple instances, they cannot be made scene root.

Describe the solution you'd like
For modifiers to work if they are nested children of Node3D nodes

Workaround
Collapse the parent structure of the modifiers after import. However, this only works in runtime and not in editor

[sjkillen]

(Possibly) relevant TODO

godot_voxel/modifiers/godot/voxel_modifier_gd.cpp

Line 132 in 02596c1

// TODO Handle nesting properly, though it's a pain in the ass

[sjkillen]

For NOTIFICATION_LOCAL_TRANSFORM_CHANGED
Here https://docs.godotengine.org/en/stable/classes/class_node3d.html

This is not received when the transform of a parent node is changed.

If we have nodes under the terrain update their children when their local transform changes, we might be able to get around subscribing to global transform changes

[Zylann]

Something I find really annoying in this situation, is that the hierarchy seems useless here, it's just in the way and requires the engine to do extra work. What makes it required is the import workflow and the way Godot works I guess? Though I'm not fully understanding why your scripts can't also automate the flattening of hierarchy, so that everything gets setup optimally (Think of it like CollisionShapes of a RigidBody).

Optimization aside, nested modifiers are a natural thing to support eventually, but didn't have the need for it before, and the way Godot propagates events has put me off the moment I had the suggestion in mind. In fact I'm not using modifiers much at the moment, I originally added them because it seemed cool to have, as I saw another voxel engine have something similar. But I moved on other things and didnt take time to update them as a result.

[sjkillen]

I can see it also being useful for parent/child transforms, but that's also achievable manually.

As far as I know, an EditorScenePostImport must return a scene which becomes a packed scene resource which, as far as I know, can only have a single root. So it's impossible to flatten the structure from an EditorScenePostImport
It might be possible from an editor import plugin, but that's less convenient.

In my mind, it wouldn't be ideal to merge meshes into a single mesh SDF as each one has limited bake precision -but that assumption comes from my limited understanding of their implementation.

If you don't think it would be too non-trivial, I might have a shot at implementing it.

[Zylann]

Couldn't you import your scene as a terrain?
Or when you instantiate these imported scenes, can't you just pick their children and put them as child of the terrain? These workarounds seem relatively easy, compared to adding an entire support for nesting just to workaround Godot's import workflow in the first place.

But if you really want this instead, you could try implementing it, as I don't plan to work on it at the moment.

[sjkillen]

I think it would work to have a separate terrain for each import as you suggest. Although if this were the case would the data all be separate?
I could reparent the children at runtime very easily, but imported scenes can't be edited in the editor.

It seems like a fun task and a good way to get my feet wetter :)

[Zylann]

I've been thinking more about nesting recently.

What concerned me so far is the issue NOTIFICATION_TRANSFORM_CHANGED poses:
We are only interested in listening to transform changes that happened between our child and the parent it registers to (excluding the parent). If we only listen to NOTIFICATION_TRANSFORM_CHANGED, we would trigger updates every time any parent moves, which can be every frame if the parent we care about is moving. This is very bad for performance.

The following will therefore focus on transform changes while already in the scene tree.
Other concerns were raised about different things in godot#77937, which have their own solutions, so I won't talk about them.

Intro

This is also happening in godot#77937 and appears to be a recurring pattern, so for the sake of making this readable from both points of view, I will generalize terms:

• Terrain, CollisionObject2D/3D -> Container
• Modifier, CollisionShape2D/3D -> Shape
• trigger voxel remeshing, update shape in parent -> update

Example of scene tree we could be dealing with, just for visual reference:

...
	GrandParentOfContainer
		ParentOfContainer
			Container <-- Node in which Shapes register into
				Shape1 <--
				IntermediateChild1
					Shape2 <--
					IntermediateChild2
						Shape3 <--
						...

First, all the following approaches have to use NOTIFICATION_TRANSFORM_CHANGED. But if a Shape is an immediate child of its Container, it can use a different code path and only listen to NOTIFICATION_LOCAL_TRANSFORM_CHANGED (like we do now), which is faster.

Now let's see what we can do in the other cases:

Approach 1: naive transform comparison

When a Shape updates, cache its transform relative to Container. Next time it receives NOTIFICATION_TRANSFORM_CHANGED, compare with the new relative transform. If it changed, trigger an update (might differ slightly from what godot#77937 proposes at this time, but unless I'm missing something important, I think the idea is the same). Simple?

Downsides:

• It requires doing math computations
• Caching requires to use more memory in the node
• We're comparing floats so it is prone to floating-point precision errors (though we could argue that if nothing changes, then whatever imprecision there is will also not change; still, I'm not a fan).
• It sounds silly to check again if the transform changes when the algorithm of notifications implicitly knows what has changed already. Surely we can do better. In fact, none of the next approaches involve any transform comparisons.

Approach 2: instigator concept

When a node's transform change starts a cascade of notifications, propagate the relative node depth alongside the notification:
For example, if ParentOfContainer moves, it notifies Container with depth 1, then Shape1 and IntermediateChild with depth 2, then Shape2 with depth 3. This is just an incrementing number when we propagate to children.

Shapes know how far they are from Container in their hierarchy: they can look it up with a few pointer lookups, or cache it for performance. This virtually costs no memory because it's such a small number it could fit in a byte and benefit from good packing next to other members.
When a Shape receives the notification, it can then check this depth: if it is lower than the parenthood distance to Container, then we know the instigator of the transform change was a child or grandchild of it.
The same approach would work if we propagate a pointer to the instigator node instead, which could have more uses, though getting depth from it might cost a bit more.
No need to calculate, compare or cache relative transforms.

This is my preferred approach.

Downsides, mostly not technical:

• Notifications don't have any payload, so it requires to tweak Godot's code, which can take months/years to get agreed upon and merged (the existing PR is proof of that). I posted a comment about the idea in godot#77937, but it seems to have been ignored.
• It adds something to a more abstract feature (transform notification) instead of only the nodes having the issue. This could be seen as against development philosophy, but I believe the concept of "instigator" actually benefits more nodes, basically every time we have this pattern of a "parent" with specific children that register to it (presently, 2D colliders, 3D colliders, our modifiers system, maybe more). Also, runtime cost and complexity of this change are very small.

Note: some notifications actually do have a payload (input events). So it isn't actually that crazy to think about having one.

Approach 3: pre-notification

If adding stuff to Godot notifications is not an option (which is our case, since AFAIK there is no plan yet to change it):

When Container receives NOTIFICATION_TRANSFORM_CHANGED, have the Container notify all its Shapes that are not direct children, to tell them to NOT update the next time they receive a NOTIFICATION_TRANSFORM_CHANGED, by setting booleans (which WILL happen within the same cascade of notifications). This should work assuming it occurs before children get their notification. Conversely, if the instigator is a child of Container, this will obviously not happen and all child Shapes will then update.
This requires Container to traverse its children, or maintain a cache of its indirect Shape nodes, which uses a bit of memory.

Approach 4: counters

Still in case the notification system can't be touched:

First, let's clarify an assumption already made since Approach 3:
We may assume transform notifications are sent exactly when each single node's transform changes, from parent to children (i.e it all happens within the setter of any property that changes the transform). Therefore, we can deduce all nodes receiving the notification will be a subtree, where the root is the node that moved (getting the notification first), and every child below it keeps the same transform relative to their immediate parents (getting the notification next).

Add counter variables to Container and Shape.
When Container's global transform changes, increment its counter (it can be a byte that wraps on overflow, or maybe even a boolean?).
When Shape's global transform changes, compare its counter with Container:

• If the Shape's counter is different than Container, we assume the Container or a parent of it moved, so we know the Shape's transform relative to the Container cannot have changed, so we don't update the Shape. Then we set the Shape counter equal to the Container.
• If the Shape's counter is equal to the Container, we can assume the Container didn't move (otherwise it would have received the notification first and incremented its counter), so the instigator must have been a child of it. Therefore it means the Shape's transform relative to the Container must have changed, so we update the Shape.

It is slightly better than Approach 3, because it does not require to allocate any memory to maintain a list of shapes in Container.
If Approach 2 is not retained, that would be my second choice.

I might experiment this at some point in the future, since at this point there has been 2 PRs just to add this (but both doing approach 1)