Hello!
In our project we’re able to efficiently use world transform freezing because we have an API layer between the user and Babylon (so we can dirty effectively).
However, when dealing with rigid animations I ran into an issue that I’m unsure if it is a conscious decision or not:
The issue is explained in the comments, but I’ll also add a more thorough description here:
Transform property setters in TransformNode set the this._isDirty = true which marks the current world transform as dirty.
Then there’s also a function on Node, overriden by TransformNode which is called markAsDirty which does the same thing, but also marks children as dirty through markAsDirty (necessary for our example).
These 2 ways of setting dirty are conflicting because they use the same variable and do different things, an issue amplified by the fact that the markAsDirty function early outs if the _isDirty flag is already set to true.
This means when running an animation, even if we were to manually call markAsDirty on the animated node to manually tell our children they are dirty, the function early outs and is a no-op. You can call freezeWorldMatrix() to forcefully set _isDirty = false, but then you lose out on the nice optimization to not have to iterate children every time you set the transform as dirty.
Is is intended behaviour to only dirty the current node when setting transform properties, or is this a bug? A simple solution would obviously be to always call markAsDirty internally, instead of setting the _isDirty flag directly, but that can have performance implications on very complex hierarchies.
Otherwise, separating the flags would also solve our issue, as we know when to manually dirty anyway.
Do you think any of the suggestions sounds like a good solution? If so I can make a PR
Well, with the separate flags I would simply add a new boolean flag, let’s call it _hasBeenMarkedDirty which would replace the old _isDirty in only the markAsDirty function. It would not be pretty though, and may be confusing to have to dirty flags (so I don’t like the idea). It wouldn’t solve the bug necessarily, but it would let us call markAsDirty manually to propagate the dirtying. So it would only be useful as a tool, not fixing the actual bug.
A better actual solution would be to do something like the following:
// New function
private markAsDirtyInternal(): void {
if (this._isDirty) return;
this._isDirty = true;
if (this._children) {
for (const child of this._children) {
// Dirty the child if it is a transform node and has world matrix freezing (otherwise the dirtying is not necessary)
if (child instanceof TransformNode && child._isWorldMatrixFrozen) {
child.markAsDirtyInternal();
}
}
}
}
public set position(newPosition: Vector3) {
this._position = newPosition;
// Mark ourselves and any children using world matrix freezing as dirty
this.markAsDirtyInternal()
}
This would give the most of the performance penalty only for world matrix freezing situations.
As I read it, all abstractMesh subclasses will have a weird behaviour here as well with dirtying (not dirtying children). Would you say this is a bug as well? If so, I could include in the same PR to call super.markAsDirty here to get consistent behaviour.
For anyone with this issue in the future:
Another separate PR with an alternative solution was merged instead, letting you do custom dirtying when needed!
Explanation: I did not anticipate that case 