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 saving branch as scene saves children without owner set #82802

Conversation

marcinn
Copy link
Contributor

@marcinn marcinn commented Oct 4, 2023

Fixes #82756 and preserves fix for #42611

@marcinn marcinn requested a review from a team as a code owner October 4, 2023 17:30
@KoBeWi KoBeWi added this to the 4.2 milestone Oct 4, 2023
@marcinn marcinn force-pushed the fix-82756-prevent-saving-children-without-owner branch 2 times, most recently from 05a9507 to ae3a477 Compare October 4, 2023 17:37
@KoBeWi
Copy link
Member

KoBeWi commented Oct 4, 2023

With this, the changes from #61966 might not be necessary anymore.

@marcinn
Copy link
Contributor Author

marcinn commented Oct 4, 2023

With this, the changes from #61966 might not be necessary anymore.

I prefer to separate this fix from reverting the other one. I'm not sure if there is a use case where an internal node has an owner set. I'm afraid that reverting #61966 may (re)introduce some issues. Preserving both for a while will be safer for stability.

@Rindbee
Copy link
Contributor

Rindbee commented Oct 13, 2023

Maybe just pass in the corresponding base (child) nodes and find their owners, if the number of their child nodes is the same (Will it be different?).

editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
editor/scene_tree_dock.h Outdated Show resolved Hide resolved
editor/scene_tree_dock.h Outdated Show resolved Hide resolved
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
@marcinn1
Copy link

Thanks for suggestions 👍 , I'll look at them and improve the code as soon as possible.

@YuriSizov YuriSizov changed the title Fix #82756: Save branch as scene saves children without owner set Fix saving branch as scene saves children without owner set Nov 13, 2023
@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Nov 13, 2023
@YuriSizov
Copy link
Contributor

@marcinn1 @marcinn (not sure which account you're actively using?)

Just bumping to remind you about unresolved review comments. This can't make it into 4.2 at this point, but if you could find time to address the notes above, this should help to move this PR along.

@marcinn1
Copy link

Thanks. I'd look at this at evening.

@marcinn marcinn force-pushed the fix-82756-prevent-saving-children-without-owner branch 2 times, most recently from f65d13f to d4821aa Compare November 14, 2023 02:28
@marcinn1
Copy link

I've rebased the branch, added suggested changes (excl r_ prefix), tested with MRP. Seems to be ok.

@YuriSizov YuriSizov requested a review from KoBeWi November 14, 2023 12:22
@marcinn marcinn force-pushed the fix-82756-prevent-saving-children-without-owner branch from d4821aa to 5ede386 Compare November 14, 2023 12:48
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

There are a couple of unresolved minor comments, but otherwise looks ok.

@marcinn1
Copy link

There are a couple of unresolved minor comments, but otherwise looks ok.

Should I add these changes before merging?

@KoBeWi
Copy link
Member

KoBeWi commented Nov 15, 2023

Yep.

@marcinn1
Copy link

Yep.

OK, this evening.

@marcinn marcinn force-pushed the fix-82756-prevent-saving-children-without-owner branch from 5ede386 to 561fcf5 Compare November 16, 2023 00:14
@marcinn1
Copy link

Updated.

Ok it is necessary, but in this situations we normally use const_cast<>.

Thanks for reminding me why I abandoned c++ ~20yrs ago ;)

@akien-mga akien-mga merged commit df8b7fe into godotengine:master Jan 3, 2024
15 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
None yet
Development

Successfully merging this pull request may close these issues.

Save branch as scene saves children without owner set
7 participants