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

Auto-update properties when replacing a node #78300

Conversation

ajreckof
Copy link
Member

Fixes #78299

The idea is to do something similar to what is done for auto updating NodePath

@ajreckof ajreckof requested review from a team as code owners June 16, 2023 00:34
@Calinou Calinou added this to the 4.1 milestone Jun 16, 2023
@ajreckof ajreckof force-pushed the fix-replace-node-destroying-Node-properties branch from fd53c83 to ddd6b58 Compare June 16, 2023 01:08
@Calinou
Copy link
Member

Calinou commented Jun 18, 2023

With this PR, changing the node type from Node2D to Node does nothing – the node remains a Node2D.

Attempting to change from Node2D to Node3D prints:

  ./scene/main/node.cpp:1992 - Parameter "common_parent" is null.

While the latter case isn't expected to work, it should print an human-readable error message.

PS: I removed the cherrypick:4.0 label as I can't reproduce this on 4.0.3: #78299 (comment)

@ajreckof
Copy link
Member Author

ajreckof commented Jun 18, 2023

With this PR, changing the node type from Node2D to Node does nothing – the node remains a Node2D.

I can't reproduce locally. Did you check by removing the script because right now changing the type of a node that has a script feels like it is not working because icon does not change because the node still inherit the last type and the script now dictates the icon even without @icon. I don't know if there is an issue tracking this.

Attempting to change from Node2D to Node3D prints:

  ./scene/main/node.cpp:1992 - Parameter "common_parent" is null.

While the latter case isn't expected to work, it should print an human-readable error message.

I agree with you it should print a better error. I remember thinking about it but I didn't know what to do about it. I'm wondering maybe just removing the reference ?
Just for reference before this PR this was already a problem because the node would have still pointed to a Node that did not correspond to the filter.

Edit : I was able to reproduce the error but then it disappeared. There is something strange on how it appears :(.

@ajreckof ajreckof force-pushed the fix-replace-node-destroying-Node-properties branch from ddd6b58 to 20efe03 Compare June 18, 2023 21:28
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
@ajreckof ajreckof force-pushed the fix-replace-node-destroying-Node-properties branch from 20efe03 to d0f1a62 Compare June 19, 2023 22:47
@akien-mga akien-mga requested a review from KoBeWi June 23, 2023 08:23
@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 23, 2023
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master dcd187d), it works as expected. The node type changes correctly and the exported variable reference is kept.

Code looks good to me at a glance.

Update editor/scene_tree_dock.cpp

Co-Authored-By: Hugo Locurcio <hugo.locurcio@hugo.pro>
@ajreckof ajreckof force-pushed the fix-replace-node-destroying-Node-properties branch from 9d4fda6 to 4da9927 Compare August 3, 2023 16:38
@YuriSizov YuriSizov changed the title Auto update Node properties. Auto-update properties when replacing a node Aug 3, 2023
@YuriSizov YuriSizov merged commit e4b8dc8 into godotengine:master Aug 3, 2023
14 checks passed
@YuriSizov
Copy link
Contributor

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.

changing type of a node removes reference from Node properties
5 participants