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

Add get_property_list #707

Merged
merged 4 commits into from
May 25, 2024
Merged

Conversation

lilizoey
Copy link
Member

@lilizoey lilizoey commented May 10, 2024

This does three main things

Adds get_property_list

Also adds property_get_revert/property_can_revert (combined into one function).

This allows the user to fully dynamically generate the property list as well as set all relevant options for the property list. This also provides a mechanism to group exports in the editor #226, however it's not the most ergonomic way as it requires all grouped properties to be defined in the get_property_list method.

Adds some convenience functions to PropertyInfo

This makes it easier to generate property infos corresponding to the usual property types. For instance to generate a #[var] declaration you can now just do PropertyInfo::new_var::<Type>("property_name") rather than needing to specify all the individual properties.

I also added documentation to each field of PropertyInfo since it's a more public facing type like this.

Adds sys-conversions that pass ownership for PropertyInfo, GString, and StringName

Unlike sys(), these methods named into_owned_(string/property)_sys and free_owned_(string/property)_sys allow us to convert a PropertyInfo directly into a sys::GDExtensionPropertyInfo while:

  • not allocating anything extra
  • not needing to keep the original values alive
  • not leaking any memory (since the free_* function can appropriately free the value later)

This is used in get_property_list and free_property_list to pass a list of property infos to Godot and then later free the values.

closes #665

@lilizoey lilizoey added feature Adds functionality to the library quality-of-life No new functionality, but improves ergonomics/internals c: register Register classes, functions and other symbols to GDScript labels May 10, 2024
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-707

@lilizoey lilizoey force-pushed the feature/get-property-list branch 2 times, most recently from d401513 to e9efb15 Compare May 10, 2024 17:37
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Looks great, thanks also for the extra documentation!

#(#cfg_attrs)*
impl ::godot::obj::cap::GodotGetPropertyList for #class_name {
fn __godot_get_property_list(&mut self) -> Vec<::godot::builtin::meta::PropertyInfo> {
// Only supported in godot api > 4.3. If support is added for earlier versions this is still needed.
Copy link
Member

Choose a reason for hiding this comment

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

You probably mean >= 4.3 here and not >.

Also not exactly sure what you mean by "If support is added for earlier versions this is still needed.", could you maybe elaborate what you mean by "support" and "this is still needed"?

(also capitalization, "Godot API")

use crate::engine::global;
use crate::engine::global::{self, PropertyHint, PropertyUsageFlags};
use crate::property::{Export, PropertyHintInfo};
use crate::{builtin::*, property::Var};
Copy link
Member

Choose a reason for hiding this comment

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

flat 😉

Comment on lines +299 to +307
// No UB or anything else like a crash or panic should happen when `property_can_revert` and `property_get_revert` return
// inconsistent values, but in case something like that happens we should be able to detect it through this function.
"property_changes" => {
if INC.fetch_add(1, std::sync::atomic::Ordering::AcqRel) % 2 == 0 {
None
} else {
Some(true.to_variant())
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice work!

Copy link
Member

@Bromeon Bromeon 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, feel free to merge when ready!

@lilizoey lilizoey added this pull request to the merge queue May 25, 2024
Merged via the queue into godot-rust:master with commit 89f5052 May 25, 2024
15 checks passed
@lilizoey lilizoey deleted the feature/get-property-list branch May 25, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add get_property_list to gdextension classes
3 participants