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

Reorder C# script properties to fix editor serialization #97014

Merged

Conversation

hayahane
Copy link
Contributor

Fixes #96983

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

I tested these changes and they fix the linked issue. However, properties exported by overriding _get_property_list now appear under the base class properties:

4.3.0 This PR

But this was also introduced by #93779 and we missed it.

@hayahane
Copy link
Contributor Author

hayahane commented Sep 16, 2024

I tested these changes and they fix the linked issue. However, properties exported by overriding _get_property_list now appear under the base class properties:

Pushed a fix for this. And I also found when _get_property_list is overridden, the base class's _get_property_list doesn't work. However in gdscript it works. Here is a similar test with derived class in #70146.
I only enable it for the derived class since I don't know how to call the base class's _get_property_list.

GDScript C#
image image

@raulsntos
Copy link
Member

And I also found when _get_property_list is overridden, the base class's _get_property_list doesn't work.

That's because in Godot _get_property_list is a multi-level function and it's treated as such in GDScript, but C# doesn't treat it as a multi-level function and you have to call base._GetPropertyList from the implementation.

But it would be a more involved fix for a pre-existing issue (#75271), so it can handled in a follow-up PR.

@hayahane
Copy link
Contributor Author

In this pr, only the _get_property_list overridden in the script will be called, and those from base classes will not. This should fix previous property ordering issue leaving #75271 for a future pr. (Currently don't have any idea how to call a method in base class from cpp side.)
Does this behaviour fit?

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Yes, I think this is an improvement over what we had before. Thanks!

modules/mono/csharp_script.cpp Outdated Show resolved Hide resolved
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

LGTM

@akien-mga akien-mga merged commit ae872a4 into godotengine:master Sep 19, 2024
20 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.

Reordering properties from #93779 unsets editor set custom properties for [Tool] nodes.
4 participants