-
-
Notifications
You must be signed in to change notification settings - Fork 217
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
Add #[export_group]
Attribute for Creating Property Groups
#214
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,12 @@ struct HasProperty { | |
texture_val: Gd<Texture>, | ||
#[export(get = get_texture_val, set = set_texture_val, hint = PROPERTY_HINT_RESOURCE_TYPE, hint_desc = "Texture")] | ||
texture_val_rw: Option<Gd<Texture>>, | ||
|
||
#[export_group(name = "foo group", prefix = "grouped_")] | ||
#[export] | ||
grouped_foo: i32, | ||
#[export] | ||
grouped_bar: i32, | ||
} | ||
|
||
#[godot_api] | ||
|
@@ -128,6 +134,8 @@ impl NodeVirtual for HasProperty { | |
string_val: GodotString::new(), | ||
texture_val: Texture::new(), | ||
texture_val_rw: None, | ||
grouped_foo: 0, | ||
grouped_bar: 0, | ||
Comment on lines
+137
to
+138
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't test the group association -- is there a way to check that programmatically at all? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what I do not (yet) know. Will experiment a bit with Gdscript property groups and introspection and then revisit this issue. |
||
base, | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why these variables are renamed here?
Probably easier to understand if the same name is used as on the call site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the extracting into function makes it a bit hard to see changes 🙈
But I agree with splitting it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason but being lazy. Will change the name back.
Re: extracting
Yup, hard to see, but a separate commit would have been overkill I think.