Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix script properties being lost and prevent node reference corruption upon scene reimport #92279

Merged
merged 2 commits into from
May 29, 2024

Conversation

SaracenOne
Copy link
Member

@SaracenOne SaracenOne commented May 23, 2024

Expands and depends upon the previous PR (#92182) to fix further bugs in the hot scene reloader. It now prevents script properties on reimported nodes from getting lost, and prevents node reference properties referencing nodes which get replaced by the reimport system from being retained, preventing several cases which can cause editor crashes.

@SaracenOne SaracenOne added this to the 4.3 milestone May 23, 2024
@fire fire requested a review from a team May 23, 2024 12:58
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested https://github.com/godotengine/godot/files/15415243/reimport_test.zip.

On 4.3 and this pr it keeps the last value

Screen.Recording.2024-05-23.at.7.23.22.PM.mov

On 4.2 it resets on reimport

Screen.Recording.2024-05-23.at.7.20.01.PM.mov

Will do more testing.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the broken cases. See video. It seems to resolve property reload bugs.

Didn't see anything obviously problematic.

The last attempt to introduce this fix created a signal. In this bugfix pull request we use _nodes_scene_reimported as the callback as a replacement.

@fire
Copy link
Member

fire commented May 24, 2024

@lyuma I expect this to solve your common case of "click on the skeleton" and click reimport to see Godot Engine crash bug.

editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
@SaracenOne SaracenOne force-pushed the scene_reload_crashfix_2 branch from f83bdeb to 686a167 Compare May 28, 2024 10:08
editor/editor_node.cpp Outdated Show resolved Hide resolved
* 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.
@fire fire force-pushed the scene_reload_crashfix_2 branch from 686a167 to 97d2730 Compare May 28, 2024 15:48
@fire
Copy link
Member

fire commented May 28, 2024

  1. updated to the latest godot master
  2. removed the #include "core/core_string_names.h" line
  3. renamed _script to script.

Preserves exported script variables
Prevents corruption of direct node references.
@fire fire force-pushed the scene_reload_crashfix_2 branch from 892786c to e57312d Compare May 28, 2024 16:24
@SaracenOne
Copy link
Member Author

Thanks @fire, I was a bit confused how the CoreStringNames API was meant to be used.

@akien-mga akien-mga merged commit 47fa384 into godotengine:master May 29, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Immediate Blocker
4 participants