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.

Sign up for GitHub

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

Jump to bottom

Enforce custom nodes to keep their original type #91341

Merged
merged 1 commit into from
Oct 24, 2024
Merged

Enforce custom nodes to keep their original type #91341

merged 1 commit into from
Oct 24, 2024

Conversation

bjornmp
Copy link
Contributor

@bjornmp bjornmp commented Apr 30, 2024 •
edited by KoBeWi
Loading

Replaces PR #82684 with a move away from Core/Object in favor of Scene/Main/Node metadata usage.

Closes godotengine/godot-proposals#7905
Closes #37479

@bjornmp bjornmp requested review from a team as code owners April 30, 2024 02:12
@dalexeev dalexeev mentioned this pull request Apr 30, 2024
Closed
@AThousandShips AThousandShips added this to the 4.x milestone Apr 30, 2024
@bjornmp bjornmp force-pushed the NewMaster branch 4 times, most recently from 0811a61 to 76f2ce2 Compare May 1, 2024 05:16
AThousandShips
AThousandShips previously requested changes May 1, 2024
View reviewed changes
scene/resources/packed_scene.cpp Outdated Show resolved Hide resolved
editor/editor_resource_picker.h Outdated Show resolved Hide resolved
editor/editor_resource_picker.cpp Outdated Show resolved Hide resolved
editor/editor_resource_picker.cpp Outdated Show resolved Hide resolved
editor/editor_resource_picker.h Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed May 11, 2024
View reviewed changes
editor/editor_data.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed May 11, 2024
View reviewed changes
editor/editor_node.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented May 11, 2024 •
edited
Loading

Not sure if showing extend icon for non-custom-type scripts is a good idea:
image
It can show for scripts that can't be extended, and more importantly, takes space, which is already too small in that area.

KoBeWi
KoBeWi reviewed May 11, 2024
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed May 11, 2024
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed May 11, 2024
View reviewed changes
scene/property_utils.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented May 11, 2024

You could try using _property_can_revert() and _property_get_revert() to handle script default. It could allow removing PackedScene changes.

@bjornmp bjornmp force-pushed the NewMaster branch 2 times, most recently from f6fd549 to ddabc7d Compare May 12, 2024 17:26
@bjornmp
Copy link
Contributor Author

bjornmp commented May 12, 2024

  • Script button "Extend" is now only shown when the script is a custom type script
  • "Clear" is only removed if resource is actually a script

I haven't found a nice way around the PackedScene changes yet. As I see it, _property_can_revert() and _property_get_revert() are virtual methods. Where would be a good place to override them to get the desired effect?

@KoBeWi
Copy link
Member

KoBeWi commented May 12, 2024

Where would be a good place to override them to get the desired effect?

In Object. Although they are part of GDClass, so not sure if it's possible. In worst case you can put some duplicate code in Node and Resource.

@bjornmp
Copy link
Contributor Author

bjornmp commented May 12, 2024 •
edited
Loading

I was trying to avoid changes to core/ as much as possible with this PR, as this was requested in the last PR attempt. That's why I initially didn't use CoreStringNames.

Maybe I can get away with the changes in packed_scene if I add a condition in PropertyUtils::is_property_value_different as I already did in PropertyUtils::get_property_default_value. No, that's not going to work.

AThousandShips
AThousandShips previously approved these changes May 15, 2024
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
scene/property_utils.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_data.cpp Outdated Show resolved Hide resolved
editor/editor_resource_picker.cpp Outdated Show resolved Hide resolved
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
scene/property_utils.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga modified the milestones: 4.x, 4.4 May 15, 2024
@bjornmp bjornmp requested a review from a team as a code owner September 27, 2024 17:26
@akien-mga
Copy link
Member

Thanks for rebasing!

@KoBeWi @AThousandShips Does this still look good to merge to you after a few months?

@KoBeWi
Copy link
Member

KoBeWi commented Oct 1, 2024

So I decided to give it a last try by testing with my project and there is a critical bug 💀

If you instantiate a scene, the value of script property will always be saved (as if it couldn't determine the default value). In case of built-in scripts, they will get embedded in the parent scene.

@AThousandShips AThousandShips dismissed their stale review October 2, 2024 07:03

Will go over again

@bjornmp
Copy link
Contributor Author

bjornmp commented Oct 2, 2024

Hi @KoBeWi, let me look into it. Can you give me minimal steps to reproduce this issue?

@KoBeWi
Copy link
Member

KoBeWi commented Oct 2, 2024

  1. Create a scene
  2. Attach any script to root node
  3. Create another scene
  4. Instantiate the first scene in the new scene
  5. Save and check the tscn file

@bjornmp bjornmp force-pushed the NewMaster branch 2 times, most recently from 03a84f3 to 52e12e3 Compare October 5, 2024 20:17
KoBeWi
KoBeWi reviewed Oct 5, 2024
View reviewed changes
scene/property_utils.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi approved these changes Oct 5, 2024
View reviewed changes
Copy link
Member

@KoBeWi KoBeWi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Now it works correctly.

I still think it's a nice feature. It improves usability of custom Nodes, though the drawback is that every newly created custom node has an additional metadata property for the base script.

It's safe to merge, because it can be reverted at any time without breaking compatibility. It's only relevant to editor.

@bjornmp bjornmp requested review from a team as code owners October 22, 2024 01:23
@KoBeWi KoBeWi mentioned this pull request Oct 22, 2024
Closed
AThousandShips
AThousandShips approved these changes Oct 22, 2024
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
scene/property_utils.cpp Outdated Show resolved Hide resolved
@bjornmp
Enforce that custom nodes keep their original type
06998a3 
Enforce that custom nodes and resources created via the "Create New Node" dialog, should permanently retain their original type (script). This means:

- Type continuity: It should be impossible for the user to (accidentally) clear the original script of a custom node that was created via the "Create New Node" dialog.

- Extensibility: The user should be able to extend custom types as usual (create a script that inherits the original type and replace the original script of that node with his own). However, if he then clears his extension-script from that node later on, the custom type should revert to its original script instead of becoming a non-scripted type.
@Repiteo Repiteo merged commit fa673be into godotengine:master Oct 24, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 24, 2024

Thanks, and congratulations on your first contribution! 🎉

@bjornmp bjornmp deleted the NewMaster branch October 26, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Repiteo Repiteo Repiteo left review comments

@AThousandShips AThousandShips AThousandShips approved these changes

@KoBeWi KoBeWi KoBeWi approved these changes

Assignees
No one assigned
Labels
enhancement topic:core
Projects
None yet
Milestone
4.4
5 participants
@bjornmp @KoBeWi @akien-mga @Repiteo @AThousandShips