Change scene reimport behaviour to address corruption/crashes #92182
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* The reimported instance attempt to preserve ownerless nodes. * A recursive function call to '_nodes_scene_reimported' so these can be recreated if required. * Clears instance scene_state on new instantiated replacement nodes.
SaracenOne
force-pushed
the
scene_reload_crashfix
branch
from
904f127 to
338675d
Compare
SaracenOne
changed the title
|
Superseded by #92279. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes an important change to how the dynamic scene reimporting works in order to hopefully prevent the crashing regression.
The issue described seems to have been triggered the introduction of PhysicalBoneSimulator3D node which is created by the Skeleton3D node automatically upon entry to the scene tree. The existing function scans the children of all nodes that require reimporting and will attempt preserve them in the hierarchy as best they can. It will also attempt to preserve ownerless nodes which I believe was the main issue with initial implementation. The PR instead does NOT preserve these ownerless nodes and makes the assumption that they are transient and should be left to the discretion of the nodes which created them.
However, for any nodes (including user's custom editor scripts) which create ownerless nodes for visual feedback purposes, there is currently no direct way to tell these nodes that these will require recreating. Since it was decided that a dedicated NOTIFICATION for this event was undesirable, the system will now recurisvely walk the scene tree and attempt to call a script-facing method called '_nodes_scene_reimported' with a list of nodes which have scenes which were reimported, giving the opportunity to recreate the missing nodes, consistent with other editor behaviours. An example project of this in action is attached to this PR.
To test it, open the test_scene then attempt to reimport xbot.gltf. The editor script will automatically instantiate the xbot scene as an ownerless child but also attach a Sprite3D node to it too. The script uses the _nodes_scene_reimported function to detect if the PackedScene used by the 'model_scene' property has changed and will recache everything according, as well as creating a new Sprite3D node and attach it.
container_nodes.zip