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

Use the class_name for the field when building PropertyInfo #441

Merged
merged 2 commits into from
Oct 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions godot-core/src/builtin/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,10 @@ macro_rules! impl_builtin_traits_inner {
impl Eq for $Type {}
};

( PartialOrd for $Type:ty => $gd_method:ident ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Nice improvement!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Clippy actually flagged the whole PartialOrd implementation as "incorrect" because there was an implementation for Ord which was interesting.

impl PartialOrd for $Type {
( Ord for $Type:ty => $gd_method:ident ) => {
impl Ord for $Type {
#[inline]
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
let op_less = |lhs, rhs| unsafe {
let mut result = false;
::godot_ffi::builtin_call! {
Expand All @@ -81,22 +81,18 @@ macro_rules! impl_builtin_traits_inner {
};

if op_less(self.sys(), other.sys()) {
Some(std::cmp::Ordering::Less)
std::cmp::Ordering::Less
} else if op_less(other.sys(), self.sys()) {
Some(std::cmp::Ordering::Greater)
std::cmp::Ordering::Greater
} else {
Some(std::cmp::Ordering::Equal)
std::cmp::Ordering::Equal
}
}
}
};

( Ord for $Type:ty => $gd_method:ident ) => {
impl_builtin_traits_inner!(PartialOrd for $Type => $gd_method);
impl Ord for $Type {
impl PartialOrd for $Type {
#[inline]
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
PartialOrd::partial_cmp(self, other).expect("PartialOrd::partial_cmp")
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}
};
Expand Down
8 changes: 5 additions & 3 deletions godot-macros/src/class/data_models/property.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ pub fn make_property_impl(class_name: &Ident, fields: &Fields) -> TokenStream {
continue;
};

let field_variant_type = util::property_variant_type(field_type);
let field_class_name = util::property_variant_class_name(field_type);
let field_name = field_ident.to_string();

// rustfmt wont format this if we put it in the let-else.
Expand Down Expand Up @@ -178,8 +180,8 @@ pub fn make_property_impl(class_name: &Ident, fields: &Fields) -> TokenStream {
let usage = #usage_flags;

let property_info = ::godot::builtin::meta::PropertyInfo {
variant_type: <<#field_type as ::godot::bind::property::Property>::Intermediate as ::godot::builtin::meta::VariantMetadata>::variant_type(),
class_name: #class_name_obj,
variant_type: #field_variant_type,
class_name: #field_class_name,
property_name: #field_name.into(),
hint,
hint_string,
Expand All @@ -194,7 +196,7 @@ pub fn make_property_impl(class_name: &Ident, fields: &Fields) -> TokenStream {
unsafe {
::godot::sys::interface_fn!(classdb_register_extension_class_property)(
::godot::sys::get_library(),
#class_name::class_name().string_sys(),
#class_name_obj.string_sys(),
std::ptr::addr_of!(property_info_sys),
setter_name.string_sys(),
getter_name.string_sys(),
Expand Down
2 changes: 1 addition & 1 deletion godot-macros/src/util/list_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ impl ListParser {

/// Take the next element of the list, if it is a key-value pair of the form `key = expression`.
pub(crate) fn try_next_key_value(&mut self) -> Option<(Ident, KvValue)> {
let Some(kv) = self.peek() else { return None };
let kv = self.peek()?;

if let Ok((key, value)) = kv.as_key_value() {
_ = self.pop_next();
Expand Down
10 changes: 10 additions & 0 deletions godot-macros/src/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,16 @@ pub fn class_name_obj(class: &impl ToTokens) -> TokenStream {
quote! { <#class as ::godot::obj::GodotClass>::class_name() }
}

pub fn property_variant_type(property_type: &impl ToTokens) -> TokenStream {
let property_type = property_type.to_token_stream();
quote! {<<#property_type as ::godot::bind::property::Property>::Intermediate as ::godot::builtin::meta::VariantMetadata>::variant_type()}
}

pub fn property_variant_class_name(property_type: &impl ToTokens) -> TokenStream {
let property_type = property_type.to_token_stream();
quote! {<<#property_type as ::godot::bind::property::Property>::Intermediate as ::godot::builtin::meta::VariantMetadata>::class_name()}
}

pub fn bail_fn<R, T>(msg: impl AsRef<str>, tokens: T) -> ParseResult<R>
where
T: Spanned,
Expand Down
2 changes: 1 addition & 1 deletion itest/rust/src/framework/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ fn get_execution_time(test: &Variant) -> Option<Duration> {
fn get_errors(test: &Variant) -> Array<GodotString> {
test.call("get", &["errors".to_variant()])
.try_to::<Array<GodotString>>()
.unwrap_or(Array::new())
.unwrap_or_default()
}

#[must_use]
Expand Down
110 changes: 97 additions & 13 deletions itest/rust/src/object_tests/property_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@

use godot::{
bind::property::ExportInfo,
engine::{global::PropertyHint, Texture},
engine::{
global::{PropertyHint, PropertyUsageFlags},
Texture,
},
prelude::*,
test::itest,
};
Expand Down Expand Up @@ -351,20 +354,101 @@ fn derive_export() {
.iter_shared()
.find(|c| c.get_or_nil("name") == "foo".to_variant())
.unwrap();
assert_eq!(
property.get_or_nil("class_name"),
"DeriveExport".to_variant()
// `class_name` should be empty for non-Object variants.
check_property(&property, "class_name", "");
check_property(&property, "type", VariantType::Int as i32);
check_property(&property, "hint", PropertyHint::PROPERTY_HINT_ENUM.ord());
check_property(&property, "hint_string", "A:0,B:1,C:2");
check_property(
&property,
"usage",
PropertyUsageFlags::PROPERTY_USAGE_DEFAULT.ord(),
);
assert_eq!(
property.get_or_nil("type"),
(VariantType::Int as i32).to_variant()
}

#[derive(GodotClass)]
#[class(init, base=Resource)]
pub struct CustomResource {}

#[godot_api]
impl CustomResource {}

#[godot_api]
impl ResourceVirtual for CustomResource {}
Comment on lines +376 to +377
Copy link
Member

Choose a reason for hiding this comment

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

The whole impl is not needed anymore 😉

(the inherent impl above is still required due to a bug, though...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neat, when did that happen? Thought I was getting errors for not having that pretty recently

Copy link
Member

Choose a reason for hiding this comment

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

The inherent one is still needed, but not the trait one.


#[derive(GodotClass)]
#[class(init, base=Resource, rename=NewNameCustomResource)]
pub struct RenamedCustomResource {}

#[godot_api]
impl RenamedCustomResource {}

#[godot_api]
impl ResourceVirtual for RenamedCustomResource {}

#[derive(GodotClass)]
#[class(init, base=Node)]
pub struct ExportResource {
#[export]
#[var(usage_flags=[PROPERTY_USAGE_DEFAULT, PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT])]
pub foo: Option<Gd<CustomResource>>,

#[export]
pub bar: Option<Gd<RenamedCustomResource>>,
}

#[godot_api]
impl ExportResource {}

#[godot_api]
impl NodeVirtual for ExportResource {}

#[itest]
fn export_resource() {
let class: Gd<ExportResource> = Gd::new_default();

let property = class
.get_property_list()
.iter_shared()
.find(|c| c.get_or_nil("name") == "foo".to_variant())
.unwrap();
check_property(&property, "class_name", "CustomResource");
check_property(&property, "type", VariantType::Object as i32);
check_property(
&property,
"hint",
PropertyHint::PROPERTY_HINT_RESOURCE_TYPE.ord(),
);
check_property(&property, "hint_string", "CustomResource");
check_property(
&property,
"usage",
PropertyUsageFlags::PROPERTY_USAGE_DEFAULT.ord()
| PropertyUsageFlags::PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT.ord(),
);
assert_eq!(
property.get_or_nil("hint"),
(PropertyHint::PROPERTY_HINT_ENUM.ord()).to_variant()

let property = class
.get_property_list()
.iter_shared()
.find(|c| c.get_or_nil("name") == "bar".to_variant())
.unwrap();
check_property(&property, "class_name", "NewNameCustomResource");
check_property(&property, "type", VariantType::Object as i32);
check_property(
&property,
"hint",
PropertyHint::PROPERTY_HINT_RESOURCE_TYPE.ord(),
);
assert_eq!(
property.get_or_nil("hint_string"),
"A:0,B:1,C:2".to_variant()
check_property(&property, "hint_string", "NewNameCustomResource");
check_property(
&property,
"usage",
PropertyUsageFlags::PROPERTY_USAGE_DEFAULT.ord(),
);

class.free();
}

fn check_property(property: &Dictionary, key: &str, expected: impl ToVariant) {
assert_eq!(property.get_or_nil(key), expected.to_variant());
}