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

Restore UNIQUE_SCENE_NAME toggle to foreign scenes #82545

Closed

Conversation

SaracenOne
Copy link
Member

@SaracenOne SaracenOne commented Sep 29, 2023

This PR restores the ability to toggle the unique name flag on nodes which are the children of instanced or inherited scenes, which should be permissible as this flag is treated as singular property for nodes. I haven't checked the history, but this mistake was likely done when I was conforming the editor to restrict editing operations in foreign scenes. It's still possible to toggle the unique scene flag off, but not turn it back on, so this make the behaviour consistent once again.

@SaracenOne SaracenOne added this to the 4.2 milestone Sep 29, 2023
@SaracenOne SaracenOne requested a review from a team as a code owner September 29, 2023 19:39
@KoBeWi
Copy link
Member

KoBeWi commented Sep 29, 2023

It's still possible to toggle the unique scene flag off

I'd say it's a bug.
By meddling with unique names of another scene it's easy to run into some unexpected behavior, e.g. flag getting cleared on scene load:

godot.windows.editor.dev.x86_64_IlRWL3Gw68.mp4

Also, since the property is not visible in the inspector, it's not revertable and you can't tell whether it was modified or not.

@SaracenOne
Copy link
Member Author

SaracenOne commented Sep 30, 2023

There's really two things we can do: either we straight up disallow the removal of the unique scene name property on foreign scenes by clicking on the icon OR we introduce a special case where if the unique scene name is part of a foreign scene which has a unique scene name in its base class, instead of removing the icon, we replace it with a new 'disabled' icon instead. First is simpler, but I don't know if there might be some weird edge cases where users might actually want to remove the unique scene name property, but on reflection, yeah, maybe it does make more sense to just fully disallow unique_scene_name toggles on foreign scenes.

@akien-mga
Copy link
Member

I think unique scene names are only meant to be unique in their owner's scope, not bubble further up where they are instantiated. So it sounds like even with editable children, this shouldn't be editable indeed. Haven't used it much myself in practice so TIWAGOS.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 30, 2023

The engine seems to discern correctly whether the unique name is part of the base scene or an instance, but there are a few corner-cases (like the one I posted) that I think might not be solvable.

@SaracenOne
Copy link
Member Author

Okay, I'm now sharing the vibe I'm feeling is that we should instead make these not editable in foreign scenes at all. I'll probably close this and make a new PR instead. One possible corner case I might need to look at though is making foreign scenes local. These will probably need to remove their unique name flags.

@SaracenOne
Copy link
Member Author

Superceded by #83370

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.

4 participants