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 edge-case errors when renaming nodes in scene tree #83368

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SaracenOne
Copy link
Member

@SaracenOne SaracenOne commented Oct 15, 2023

Introduces the following rules to the standard NodePath fixer when renaming nodes in the scene tree:

  • Disallow renaming in read-only resources.
  • Disallow renaming of default properties in subscenes.
  • Disallow renaming of properties and resources marked INTERNAL.

Animation NodePaths are marked as internal, but the updating of animation node paths (for animations which are editable), should still occur due to some special case handling for them. This just silences the warnings about broken NodePaths for the standard NodePath update pass.

Closes #83361

@SaracenOne SaracenOne added this to the 4.2 milestone Oct 15, 2023
@SaracenOne SaracenOne requested a review from a team as a code owner October 15, 2023 04:29
Disallow renaming in read-only resources,
Disallow renaming of default properties in subscenes
Disallow renaming of properties and resources marked INTERNAL
@KoBeWi
Copy link
Member

KoBeWi commented Oct 16, 2023

Some of the issues you mentioned were fixed by #83263, so this might need re-assessing.

@SaracenOne
Copy link
Member Author

Yeah, your PR would probably have fixed stuff, but it's still possible this PR may cover other edge cases. Denying the ability to update nodepaths for read-only resources may still make sense a general-purpose rule, but yours probably still covers the more obvious fail-cases. I'll leave it open in case its felt this still makes sense.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 16, 2023

I mean, the PR would still be useful if it fixes some remaining known issues. I didn't test, but the problems you described should've been fixed at least partially, so the PR and the linked issue should be tested if something still needs fixing.

btw it's still possible for properties with PROPERTY_USAGE_INTERNAL to be serialized and/or exposed in the inspector, skipping them might spawn new bugs.
The part about readonly stuff makes sense.

@SaracenOne
Copy link
Member Author

Okay, I'll edit this to remove the PROPERTY_USAGE_INTERNAL stuff since your PR already fixed the most notable thing (animations) this PR was meant to address. The others are hypothetical fixes I discovered on the way, but I think they still make sense as rules that might avoid obscure edge-cases.

@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Nov 13, 2023
@KoBeWi KoBeWi modified the milestones: 4.3, 4.4 Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_update_node_path lacks sufficent context to properly rename paths upon node renames, resulting in errors.
3 participants