Fix reparent() losing owner

[twobitadder]

Fixes #81480, but I am open to suggestions on any changes that need to be made - I just took the most obvious path suggested in the issue for a fix.

Thanks!

[RandomShaper]

I think this makes sense, but let me bring a couple of points up:

• Setting the owner requires additional steps, like checking the new owner is valid (an ancestor). Let me suggest moving the code of Node::set_owner() to a private function _set_owner() that also takes a bool p_show_warning. The Node::set_owner() would just call through it with true. reparent() would instead call it with false to try to set the original owner, but not printing an error in case that one is no longer valid. Additionally, in cases like this we likely don't want owner_changed_notify() to be called (see _set_owner_nocheck()) when the original owner is successfully restored. Maybe this warrants a more comprehensive rewrite so all this is taken into account at a more elemental level instead of being that "patchy."
• Documentation would need to be updated (haven't checked, but it likely says that reparent() is exactly like removing plus re-adding somewhere else; the owner-preserving behavior should be mentioned).

[twobitadder]

this doesn't feel much less patchy, but maybe we can include a bool p_pending_reparent in the Node class that will skip clearing owner on Node::_propagate_after_exit_tree() if true? that way there doesn't have to be a private Node::_set_owner() implementation. just set it to true before reparenting and then false immediately after

in the meantime i'll get the docs checked and updated properly

edit: on thinking about this it would also need to propagate this property to children before reparenting. i see what you mean about the changes on an elemental level. this feels like a pretty big thread to be pulling on.

[YuriSizov]

this doesn't feel much less patchy, but maybe we can include a bool p_pending_reparent in the Node class that will skip clearing owner on Node::_propagate_after_exit_tree() if true? that way there doesn't have to be a private Node::_set_owner() implementation. just set it to true before reparenting and then false immediately after

That would be very hacky and prone to issues, like leaving pointers to the old owner when the nodes are no longer in a relationship. What Pedro suggests is much more robust and in line with how we often do stuff. And also much simpler.

[twobitadder]

i guess the issue i'm seeing (and realizing my original solution is definitely inadequate for) is that children of the reparented node will lose their owner, which would mean that if the unique node is a child of the reparented node, the reference is lost. i can use the private _set_owner() call to set the owner of the reparented node, but any children will find their owner lost.

i also may have misunderstood the original intention here! i was under the impression that you would want to keep the owner during reparent, even if the new parent doesn't share a branch with the original parent.

[YuriSizov]

i was under the impression that you would want to keep the owner during reparent, even if the new parent doesn't share a branch with the original parent.

Owner must be an ancestor of the node. If after reparenting this is no longer the case, then losing the owner is the correct behavior.

[twobitadder]

my mistake! so i think the assessment of this is the nodes being reparented need to be checked for who their owner is and if they're being reparented to a descendant of that owner. then the owner can be reset on those nodes (and not any nodes that did not originally have that owner).

i apologize, i'm sure it's obvious that i'm also familiarizing myself with the idiomatic way of doing things as i go and want to make sure this is done correctly, but it definitely ended up being a bit more involved than i anticipated 😅

[RandomShaper]

No need to apologize! You're trying, learning and addressing feedback. You can't be asked for more.

[KoBeWi]

btw turns out the label "good first issue" was wrongly assigned on this one. It's more complex than I anticipated, sorry 🙃

[twobitadder]

all good! i appreciate the patience everybody. once i'm back in town middle of next week i'll sit down and get this worked out.

[twobitadder]

here's a first pass at a way to resolve all of the needs of this fix. it's, admittedly, an extremely naive take - my c++ experience is about ten years out of use at this point and was already rusty as i was using it, haha. i'm welcome to any recommendations as a result, and if this needs to be a more fundamental change, then so be it! as it stands i'm a little uncertain how this might be taken as it involves three iterations in order to solve the original issue, which is a bit undesired, i feel like.

[twobitadder]

... once i fix all of the style issues, of course.

[RandomShaper]

I will likely add new comments in the future (as soon as I can reason carefully about this). That said, your C++ is fine. (And if it's "rusty," well, it's trendy. 😃)

[twobitadder]

hi! i just wanted to see if there was any chance to check this out a little more. if not, no problem, but i would love to be able to contribute a fix if possible.

[RandomShaper]

This is still under my radar, but I have been too busy. I will come back very soon!

[twobitadder]

no problem at all! i appreciate the update, i also know it's probably gotten quite a bit busier with all of the new people recently :)

[twobitadder]

not a nitpick at all! i, uh, didn't realize i had been taking everybody else on the same journey i had been going on while updating this PR

[RandomShaper]

Sorry for stepping in when everything looked settled. Just a few minor things I think can be improved.

[twobitadder]

no problem! i'm definitely also learning as i go with this experience so it's useful for me as well

[akien-mga]

For the record, we discussed this together with other PR with Yuri and Clay from the production team and decided that this wasn't critical for 4.2 (which is nearing release candidate), and might need some testing to ensure everyone agrees with the behavior change, hence the milestone bump to 4.3.

It should likely be good to merge as soon as dev reopens for 4.3 in a few weeks.

[twobitadder]

sounds good - i think that's completely reasonable given this is a fairly common action

[twobitadder]

hi! just wanted to check in and see if this is still a good candidate for 4.3, and if there's anything else required of me to assess/change

[AThousandShips]

The review comments you marked as resolved but didn't apply maybe? Or at least explain why not applying them since you marked them as resolved 🙂

[twobitadder]

hmm, i'm not really sure... i applied all of the requested changes as far as i know, so i'm not certain why it's saying that there are more changes. maybe i missed one? reviewing the four requested changes i do see, i did enact them all before listing them as resolved (at the very least, the current version of the code reflects those changes)

[AThousandShips]

Unresolved the ones you didn't apply 🙂

[twobitadder]

ahh okay - the ones from KoBeWi were previously applied, but i had changed them out based on later requested changes by RandomShaper. they were resolved at the time though they do conflict now, is there a consensus on which direction i should go with those particular lines?

[AThousandShips]

Then resolved, my bad, the change is correct 🙂 the interface is confusing when it considers code not outdated when it's changed back and forth 🙃

[twobitadder]

not a problem at all, and i appreciate the check! i just wanted to be sure i wasn't missing anything, haha

agreed on it not being intuitive as well, it'd be super cool if it noted that there were conflicting requested changes lol

[akien-mga]

CC @RandomShaper

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[pc9098]

This doesn't seem to be included in changelog

[twobitadder]

correct me if i'm wrong, but i think this would only be in there if it is a part of a release. this missed the dev 2 window so it won't be in engine until dev 3 at earliest

[akien-mga]

The interactive changelog isn't updated automatically, it was last synced when I released 4.2-dev2. So yeah changes merged after dev2 aren't reflected yet.