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

Don't duplicate internal nodes #89442

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

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Mar 13, 2024

As commented in godotengine/godot-proposals#9282 (comment), this PR makes nodes added with INTERNAL_MODE_FRONT or INTERNAL_MODE_BACK not duplicated, similar to parent_owned. Also removed parent_owned and related methods, because they are no longer needed. AFAIK all internal nodes already use the proper internal mode. I also added helper method is_internal() to make it easier to use.

Supersedes #88114
Supersedes #89388
Supersedes #96018

Resolves godotengine/godot-proposals#9282 (with alternate solution)
Fixes #96246
Fixes #92880

@AThousandShips
Copy link
Member

Doesn't fully supersede #88114 without cherry picking

Interesting otherwise, I'd say it breaks compatibility even though the parent owned feature isn't accessible to scripting, though I'm not sure how people would handle this behavior currently in scripting

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 13, 2024

Well currently handling this is not possible via scripting. The internal nodes before this PR didn't prevent duplication. godotengine/godot-proposals#9282 proposed exposing the "parent owned" property, but it's a hack and now that we have a proper API for adding internal children, it can be used to replace this legacy code.

@AThousandShips
Copy link
Member

AThousandShips commented Mar 13, 2024

The internal nodes not handling this before is the compatibility breakage I'm talking about though 🙃, unsure if people do use it but if they do use internal nodes then their code will break now, especially if they don't intend them to not be duplicated

I'm not against this change, but it should be treated as breaking IMO to indicate this change so people can adjust

@Mickeon
Copy link
Contributor

Mickeon commented Mar 13, 2024

I agree. It would break compatibility. But "break compatibility (good)". It also gives more reasons to make use of this feature. It could use some testing to ensure it doesn't break built-in Control nodes on duplication.

@kitbdev
Copy link
Contributor

kitbdev commented Sep 8, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment