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

Expose _validate_property() for scripting #75778

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Apr 7, 2023

_validate_property() has (at least) 2 uses in scripting:

  • convenient customization of exported property list (you can change usage without using _get_property_list()+_get()+_set())
  • customizing built-in properties (hiding them, disabling etc. Right now it's not possible at all)

This PR exposes it to users. I did it only for GDScript, as I'm not sure how to do it in other languages.
I also added a call to _validate_property() for exported properties. This is now possible:

@export var test = 1

func _validate_property(property):
    if property.name == "test":
        property.usage |= PROPERTY_USAGE_READONLY

@KoBeWi KoBeWi added this to the 4.x milestone Apr 7, 2023
@KoBeWi KoBeWi requested review from a team as code owners April 7, 2023 12:16
@YuriSizov
Copy link
Contributor

I'm not sure how to do it in other languages.

I'm pretty sure this is why I didn't expose it in #64339. I may even have an old branch somewhere that does the same as you do, but the problem with passing property info to extensions remains.

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 7, 2023

the problem with passing property info to extensions remains

Pretty sure it can be done the same way as in _get_property_list(). My problem was with how these functions are defined in extensions. It's a spaghetti of method pointer typedefs. I didn't even look at C#, because I could compile without it 🙃

@YuriSizov
Copy link
Contributor

Pretty sure it can be done the same way as in _get_property_list().

For me the problem was that with validate it's a two way communication, whereas property list only returns a list of properties. For validate we need to construct GDExtensionPropertyInfo from PropertyInfo, pass it to extensions, and then somehow handle changes (is it a reference, is it a value?) Which was way beyond my comfort zone to implement. I'm sure it's doable :)

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 3, 2023

I changed the calling order. Now the script _validate_property() is called after the virtual one, so users can validate already validated properties.

I also added a C# method stub.

Comment on lines +678 to +680
virtual void validate_property(PropertyInfo &p_property) const override {
// TODO
}
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to have this implemented before merging. CC @godotengine/gdextension

Copy link
Contributor

Choose a reason for hiding this comment

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

This'll require adding a new function pointer to GDExtensionScriptInstanceInfo, which (per the pattern being established in PR #78634) actually means making a new struct with the new property and a new version of the script_instance_create function in gdextension_interface.h. We've been holding off on these sort of changes until PR #78634 is merged, wanting to get at least the first one in before doubling down on this pattern.

In any case, I'd be happy to either write the code to do this (not sure the best way to provide it with this unmerged, though?) or to provide guidance on how to do it.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. I guess let's wait for #78634, we could merge this PR with the TODO in the meantime, and make note / open an issue about implementing the GDExtension counterpart before the 4.2 release.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 28, 2023
@akien-mga akien-mga merged commit 76d318d into godotengine:master Aug 29, 2023
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.

5 participants