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

Implement Var and Export for DynGd<T, D> #998

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Yarwin
Copy link
Contributor

@Yarwin Yarwin commented Jan 6, 2025

  • Derive Var and Export for DynGd<T, D>
  • Add proper tests for both

@Yarwin Yarwin force-pushed the derive-var-and-export-for-dyn-gd branch from a90edf2 to 710e774 Compare January 6, 2025 11:02
@GodotRust
Copy link

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

1 similar comment
@GodotRust
Copy link

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

@Yarwin Yarwin marked this pull request as draft January 6, 2025 11:13
@Yarwin Yarwin force-pushed the derive-var-and-export-for-dyn-gd branch 2 times, most recently from 669f127 to 07772e9 Compare January 6, 2025 11:50
@Yarwin Yarwin marked this pull request as ready for review January 6, 2025 11:55
@Yarwin Yarwin force-pushed the derive-var-and-export-for-dyn-gd branch from 07772e9 to 61bf820 Compare January 6, 2025 11:57
- Fix tests (make sure that all instances will be properly freed), move wrong type test to one that contains nothing but RefCounted instances
@Yarwin Yarwin force-pushed the derive-var-and-export-for-dyn-gd branch from 61bf820 to 9beae85 Compare January 6, 2025 11:59
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.

Thanks a lot! Added some comments (half just minor formatting things) 🙂

dyn_gd_exporter.second = node
assert_eq(dyn_gd_exporter.second, node)

# RefcHealth is valid candidate for `first` field
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# RefcHealth is valid candidate for `first` field
# RefcHealth is valid candidate for `first` field.

Comment on lines 71 to 78
func test_export_dyn_gd():
var dyn_gd_exporter = DynGdExporter.new()
# Nodehealth is valid candidate both for `empty` and `second` fields.
var node = NodeHealth.new()
dyn_gd_exporter.first = node
assert_eq(dyn_gd_exporter.first, node)
dyn_gd_exporter.second = node
assert_eq(dyn_gd_exporter.second, node)
Copy link
Member

Choose a reason for hiding this comment

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

Formatting nitpick (you can apply directly, but no need to keep my authorship):

Suggested change
func test_export_dyn_gd():
var dyn_gd_exporter = DynGdExporter.new()
# Nodehealth is valid candidate both for `empty` and `second` fields.
var node = NodeHealth.new()
dyn_gd_exporter.first = node
assert_eq(dyn_gd_exporter.first, node)
dyn_gd_exporter.second = node
assert_eq(dyn_gd_exporter.second, node)
func test_export_dyn_gd():
var dyn_gd_exporter = DynGdExporter.new()
# NodeHealth is valid candidate both for `empty` and `second` fields.
var node = NodeHealth.new()
dyn_gd_exporter.first = node
assert_eq(dyn_gd_exporter.first, node)
dyn_gd_exporter.second = node
assert_eq(dyn_gd_exporter.second, node)

Comment on lines 87 to 93
func test_export_dyn_gd_should_fail_for_wrong_type():
var dyn_gd_exporter = RcDynGdExporter.new()
var refc = RefcHealth.new()
disable_error_messages()
dyn_gd_exporter.only_node_health = refc # Should fail.
enable_error_messages()
assert_fail("`DynGdExporter.only_node_health` should only accept NodeHealth if it implements InstanceIdProvider trait")
Copy link
Member

Choose a reason for hiding this comment

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

Also here, please use empty lines to group things.

Suggested change
func test_export_dyn_gd_should_fail_for_wrong_type():
var dyn_gd_exporter = RcDynGdExporter.new()
var refc = RefcHealth.new()
disable_error_messages()
dyn_gd_exporter.only_node_health = refc # Should fail.
enable_error_messages()
assert_fail("`DynGdExporter.only_node_health` should only accept NodeHealth if it implements InstanceIdProvider trait")
func test_export_dyn_gd_should_fail_for_wrong_type():
var dyn_gd_exporter = RcDynGdExporter.new()
var refc = RefcHealth.new()
disable_error_messages()
dyn_gd_exporter.only_node_health = refc # Should fail.
enable_error_messages()
assert_fail("`DynGdExporter.only_node_health` should only accept NodeHealth if it implements InstanceIdProvider trait")

Comment on lines 459 to 466
#[derive(GodotClass)]
#[class(base=Node)]
struct DynGdExporter {
#[export]
first: Option<DynGd<Object, dyn Health>>,
#[export]
second: Option<DynGd<foreign::NodeHealth, dyn InstanceIdProvider>>,
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[derive(GodotClass)]
#[class(base=Node)]
struct DynGdExporter {
#[export]
first: Option<DynGd<Object, dyn Health>>,
#[export]
second: Option<DynGd<foreign::NodeHealth, dyn InstanceIdProvider>>,
}
#[derive(GodotClass)]
#[class(base=Node)]
struct DynGdExporter {
#[export]
first: Option<DynGd<Object, dyn Health>>,
#[export]
second: Option<DynGd<foreign::NodeHealth, dyn InstanceIdProvider>>,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Also first should be a var instead of export)

Comment on lines 468 to 476
#[godot_api]
impl INode for DynGdExporter {
fn init(_base: Base<Node>) -> Self {
Self {
first: None,
second: None,
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

init could be generated, no?

Comment on lines 483 to 505
impl<T, D> Var for DynGd<T, D>
where
T: GodotClass,
D: ?Sized + 'static,
{
fn get_property(&self) -> Self::Via {
self.to_godot()
}

fn set_property(&mut self, value: Self::Via) {
*self = FromGodot::from_godot(value);
}
}

impl<T, D> Export for DynGd<T, D>
where
T: GodotClass,
D: ?Sized + 'static,
{
fn as_node_class() -> Option<ClassName> {
Some(T::class_name())
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if these should not simply delegate to the Var and Export impls of Gd<T>. This would ensure that the implementations stay in line; otherwise we risk diverging behavior in case of bugfixes/amendments.

Copy link
Contributor Author

@Yarwin Yarwin Jan 6, 2025

Choose a reason for hiding this comment

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

as_node_class now uses Gd::<T>::as_node_class(), while get_property delegates it to self.obj.get_property()

(Everything else, as far as I'm aware, is going via Self::via which is Gd)

Copy link
Member

Choose a reason for hiding this comment

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

(Everything else, as far as I'm aware, is going via Self::via which is Gd)

Which is currently Gd, but this may change 🤔

I assume we can't use this...

    fn set_property(&mut self, value: Self::Via) {
        self.obj.set_property(value)
    }

...because that would slice the object by not setting the erased_obj field. If yes, could you make a corresponding comment? Not that this falls victim to a future refactoring making the two more symmetric.

Regarding as_node_class, you could make more explicit that you're using the Export impl, as follows:

impl<T, D> Export for DynGd<T, D>
where
    T: GodotClass + Bounds<Exportable = bounds::Yes>,
    D: ?Sized + 'static,
{
    fn as_node_class() -> Option<ClassName> {
        <Gd<T> as Export>::as_node_class()
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<Gd<T> as Export>::as_node_class() makes more sense; I also changed *self = FromGodot::from_godot(value); to *self = Self::from_godot(value); to make it more obvious and added proper comment

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if that's more obvious, as it now looks like it could be an inherent associated function 😀
Might then as well go the full route and use

*self = <Self as FromGodot>::from_godot(value);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, applied

Yarwin added 2 commits January 6, 2025 13:17
- Use T in DynGd<T, T> for Export
- Formatting fixes - use newline to seperate steps in given testcase
@Yarwin Yarwin force-pushed the derive-var-and-export-for-dyn-gd branch from 73f060e to 5a7e2aa Compare January 6, 2025 12:22
- Add better errors message for `test_export_dyn_gd_should_fail_for_wrong_type` testcase
@Bromeon
Copy link
Member

Bromeon commented Jan 6, 2025

The discussion here raised another question:

Since you're using FromGodot::from_godot(), that will panic if the conversion doesn't succeed. Concretely, when the object being set in the UI doesn't implement the D trait (or isn't registered as such). How should we handle this?

(I think we already have the problem in some places with Export...)

- Add better errors message for `test_export_dyn_gd_should_fail_for_wrong_type` testcase
@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Jan 6, 2025
@Yarwin
Copy link
Contributor Author

Yarwin commented Jan 6, 2025

Since you're using FromGodot::from_godot(), that will panic if the conversion doesn't succeed. Concretely, when the object being set in the UI doesn't implement the D trait (or isn't registered as such). How should we handle this?

IMO It definitively should panic, with proper error message, and it does so in best case scenarios. In other words – it should be user responsibility. Moreover, godot itself doesn't have any mechanism for proper error handling and follows philosophy of keeping the software running even if something goes wrong (sometimes terribly wrong).

To elaborate more about best case scenarios and UX: If both objects are marked as a tool, then user will be properly informed while trying to set given property in case of an error (godot handles everything nicely – the old value is being kept unless explicitly cleared/replaced); if none of them are tool (in other words they are inactive) then the panic happens at runtime as soon as given object is instantiated; if parent object is marked as a tool, but the one being set as a value for property isn't then… user can't set anything as this property, no matter if it is valid object or not 🙃 (that's ULTRA janky and confusing) . While the first two are rather obvious and in-line with what godot is doing, the third case should be properly documented (I'll do it if we are fine with it as it is).

2025-01-06.15-06-21.mp4

video showcasing the case when both objects are marked as a tool

@Bromeon
Copy link
Member

Bromeon commented Jan 6, 2025

Oh, this looks quite nice! Happy to see that the existing panics cover this case well 👍 thanks for the video!

if parent object is marked as a tool, but the one being set as a value for property isn't then… user can't set anything as this property, no matter if it is valid object or not 🙃 (that's ULTRA janky and confusing) . While the first two are rather obvious and in-line with what godot is doing, the third case should be properly documented (I'll do it if we are fine with it as it is).

Hm, since you mentioned "in line with Godot", does GDScript (with/without @tool) handle this differently?

Is there even something we can do better on godot-rust side? The tool-or-not distinction is a flag (runtime_class) we specify when registering a class... so it might need to be addressed in GDExtension?

Either way, I think documenting this would be great. Probably the Export docs are the best place, as it may apply to other FromGodot::from_godot errors, too -- but the DynGd case could be mentioned as an example. What do you think?

@lilizoey
Copy link
Member

lilizoey commented Jan 6, 2025

Since you're using FromGodot::from_godot(), that will panic if the conversion doesn't succeed. Concretely, when the object being set in the UI doesn't implement the D trait (or isn't registered as such). How should we handle this?

IMO It definitively should panic, with proper error message, and it does so in best case scenarios. In other words – it should be user responsibility. Moreover, godot itself doesn't have any mechanism for proper error handling and follows philosophy of keeping the software running even if something goes wrong (sometimes terribly wrong).

i would like to note that godot has an official way to do error reporting when it comes to incorrectly configured errors: Node._get_configuration_warnings. I don't believe there is anything we could do with that here though, since it's something the node that has the property would need to implement.

One thing we might be able to do is use PROPERTY_HINT_NODE/RESOURCE_TYPE with a list of all allowable classes. though it would require us to somehow enumerate all valid classes.

@Yarwin
Copy link
Contributor Author

Yarwin commented Jan 6, 2025

I'll look into using PROPERTY_HINT_X_TYPE

@Yarwin Yarwin marked this pull request as draft January 6, 2025 22:27
- Use PropertyHint::NODE_TYPE and PropertyHint::RESOURCE_TYPE to mark what can and what can't be exported via the editor
@Yarwin
Copy link
Contributor Author

Yarwin commented Jan 7, 2025

I implemented export_hint for DynGd using PropertyHint::NODE_TYPE and PropertyHint::RESOURCE_TYPE.

I had to split class registration process into two (or somehow pass the Global to export_hint() function every time when I'm calling it… which, if I'm not mistaken, can happen outside the class registration… and to make matters worse, obviously not all classes might be registered at this point) which I don't like, but seems to be the best choice :/. So far I haven't tested it with nodes, only with various resources – I'll do some manual testing later on

if you are fine with implementation let me know; I'll update the DynGd docs to add information about noticed export quirks & squash the commits afterwards.

- Remove unnecressary derive macros, make some classes private, add few readability fixes.
@Yarwin Yarwin force-pushed the derive-var-and-export-for-dyn-gd branch from d253147 to a9873ed Compare January 7, 2025 08:26
- Fix typo while concatenating the names of classes implementing given `dyn Trait`
@Yarwin Yarwin marked this pull request as ready for review January 7, 2025 09:08
@Yarwin
Copy link
Contributor Author

Yarwin commented Jan 7, 2025

Ok, this is pretty cool QoL feature actually, thanks @lilizoey 🫡

Comment on lines 511 to 515
// theoretically unreachable - `Object`s are not suitable for export.
return PropertyHintInfo {
hint: PropertyHint::NONE,
hint_string: GString::default(),
};
Copy link
Member

Choose a reason for hiding this comment

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

i think it'd be better to just use unreachable! here, that's what we do with gd:

unreachable!("classes not inheriting from Resource or Node should not be exportable")

maybe honestly it'd be better to just defer to the Gd impl but change the hint string? like

PropertyHintInfo {
  hint_string: get_dyn_property_hint_string::<D>(),
  ..Gd::<T>::export_hint(),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deffering to Gd seems like obvious choice 😅, implemented

Comment on lines +355 to +386
pub(crate) fn get_dyn_property_hint_string<D>() -> GString
where
D: ?Sized + 'static,
{
let typeid = any::TypeId::of::<D>();
let dyn_traits_by_typeid = global_dyn_traits_by_typeid();
let Some(relations) = dyn_traits_by_typeid.get(&typeid) else {
let trait_name = sys::short_type_name::<D>();
godot_warn!("No gdext class has been linked to trait {trait_name} with #[godot_dyn].");
return GString::default();
};

let mut relations = relations.iter();
let first_relation = relations
.next()
.map(|r| r.implementing_class_name.to_string())
.unwrap_or_else(|| {
panic!(
"\
Trait {trait_name} has been registered as DynGd Trait \
despite no class being related to it \
**this is a bug, please report it** \
",
trait_name = sys::short_type_name::<D>()
)
});

GString::from(relations.fold(
first_relation,
|hint_string, relation| format! {"{hint_string},{r}", r=relation.implementing_class_name},
))
}
Copy link
Member

Choose a reason for hiding this comment

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

oh this is gonna conflict somewhat with my PR #1003, though i dont think it'll be a huge issue at least? but we can probably merge this PR first and then i just fix it up in my PR.


register_class_raw(info);
/// Lets Godot know about all classes that have self-registered through the plugin system.
pub fn auto_register_classes(init_level: InitLevel) {
Copy link
Member

Choose a reason for hiding this comment

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

what happened here exactly btw? git is a bit confusing about what exactly you changed, is this just a rename/move?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is mostly rename/move (since we actually need to populate dyn_traits_by_typeid); it looks horrifying ngl

I split the class registration into two – one in its own function populate_the_registries which loads all the plugin info into registers, and the second one that actually registers the classes in godot. Outside of that nothing has been changed 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's possible to move the method to its previous location, so a diff is actually visible 🤔

- Delegate DynGd's Export::export_hint to inner Gd<T> type
@Bromeon Bromeon changed the title Derive Var and Export for DynGd<T, D> Derive Var and Export for DynGd<T, D> Jan 7, 2025
@Bromeon Bromeon changed the title Derive Var and Export for DynGd<T, D> Implement Var and Export for DynGd<T, D> Jan 7, 2025
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.

Limiting to a set of classes is indeed very cool! Thanks a lot for adding this QoL feature 👍

Comment on lines +196 to +200
/// Populates all the registries with information derived from ClassRegistrationInfo
fn populate_the_registries(
map: &mut HashMap<ClassName, ClassRegistrationInfo>,
init_level: InitLevel,
) {
Copy link
Member

Choose a reason for hiding this comment

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

The doc says not much more than the function name/signature. Which information concretely is derived from ClassRegistrationInfo?

Maybe the function name can also be made more specific? You could btw remove "the" 🙂


register_class_raw(info);
/// Lets Godot know about all classes that have self-registered through the plugin system.
pub fn auto_register_classes(init_level: InitLevel) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's possible to move the method to its previous location, so a diff is actually visible 🤔

@@ -337,6 +352,39 @@ pub(crate) fn try_dynify_object<T: GodotClass, D: ?Sized + 'static>(
Err(error.into_error(object))
}

pub(crate) fn get_dyn_property_hint_string<D>() -> GString
Copy link
Member

Choose a reason for hiding this comment

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

Brief docs what the hint string contains would be nice.

let dyn_traits_by_typeid = global_dyn_traits_by_typeid();
let Some(relations) = dyn_traits_by_typeid.get(&typeid) else {
let trait_name = sys::short_type_name::<D>();
godot_warn!("No gdext class has been linked to trait {trait_name} with #[godot_dyn].");
Copy link
Member

Choose a reason for hiding this comment

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

I've started using "godot-rust" instead of "gdext" in error/info messages, my thinking is:

  • gdnative is barely used anymore, and the distinction isn't relevant when one uses Godot 4
  • however, users may have extensions from different languages (godot-cpp, godot-rust, etc).

You could even prefix it:

Suggested change
godot_warn!("No gdext class has been linked to trait {trait_name} with #[godot_dyn].");
godot_warn!("godot-rust: No class has been linked to trait {trait_name} with #[godot_dyn].");

Comment on lines +373 to +377
"\
Trait {trait_name} has been registered as DynGd Trait \
despite no class being related to it \
**this is a bug, please report it** \
",
Copy link
Member

Choose a reason for hiding this comment

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

Note that the line breaks here do not translate to line breaks in the message, so punctuation before "This is a bug" (maybe uppercase as well) would probably help.

First and last line are also not necessary? And the whole thing can be indented 😊

Copy link
Member

Choose a reason for hiding this comment

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

Furthermore, it might be a bit clearer to remove the map and instead add a

let some_var_name = first_relation.implementing_class_name.to_string();

Because that separates error handling from extraction logic.

The whole relations.next().unwrap_or_else(|| panic!(...)) could then simply become

assert!(!relations.is_empty(), "...");

Comment on lines +452 to +457
#[derive(GodotClass)]
#[class(init)]
struct RcDynGdExporter {
#[var]
only_node_health: Option<DynGd<foreign::NodeHealth, dyn Health>>,
}
Copy link
Member

Choose a reason for hiding this comment

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

What does "Rc" stand for here?

If you mean "RefCounted", our typical abbreviation is "Refc" to distinguish from Rust's standard Rc pointer.

Also, do we need two separate exporter classes here, or can we just have one more field in DynGdExporter? Every class added to tests adds permanent runtime overhead in CI, which may add up over time...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RcDynGdExporter is Refcounted which makes it eligible to check for failing tests (since it will be properly cleaned up without much issue).

There is another way though – I'll move these tests to already existing test suite (the one with HasProperty)

Copy link
Member

Choose a reason for hiding this comment

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

So you do need 2 classes RefcDynGdExporter and DynGdExporter? If yes, then keep them.

No need to create huge non-locality, it's better to keep this in the dyn_gd_test.rs file if possible, otherwise tests can become quite hard to follow. I just thought it would be easy to merge the two, but restructuring unrelated parts of the tests isn't worth it.

}

fn set_property(&mut self, value: Self::Via) {
// `set_property` can't be delegated to Gd<T> since we have to set `erased_obj` as well
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// `set_property` can't be delegated to Gd<T> since we have to set `erased_obj` as well
// `set_property` can't be delegated to Gd<T>, since we have to set `erased_obj` as well.

Comment on lines +382 to +385
GString::from(relations.fold(
first_relation,
|hint_string, relation| format! {"{hint_string},{r}", r=relation.implementing_class_name},
))
Copy link
Member

Choose a reason for hiding this comment

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

Instead of "calculate initial value + fold()", would reduce() be a possibility? Generally it's not so nice that the implementing_class_name needs to be extracted twice.

I'm also OK with keeping it simple and just repeatedly appending to a string in a for loop.

When I write code myself, if functional approaches can't be done in an elegant and obvious way, I usually prefer procedural ones. Simple-to-understand code is better than clever one 🙂

Copy link
Member

Choose a reason for hiding this comment

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

i think we could do something like

let relations: Vec<String> = relations.iter().map(|rel| rel.implementing_class_name.to_string()).collect();
let hint_string: String = relations.join(",");
// check if is empty and error 

Copy link
Member

Choose a reason for hiding this comment

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

It's not great that the easiest way here is an inefficient one (intermediate Vec allocated, on top of all temporary strings). Not that it matters much, but even the for loop doesn't have this problem.

I now remember this wasn't the first time this annoyed me, we can probably just use this:

pub fn join_with<T, I, F>(mut iter: I, sep: &str, mut format_elem: F) -> String
where
I: Iterator<Item = T>,
F: FnMut(&T) -> String,
{
let mut result = String::new();
if let Some(first) = iter.next() {
result.push_str(&format_elem(&first));
for item in iter {
result.push_str(sep);
result.push_str(&format_elem(&item));
}
}
result
}

You could even call this on the outer collection and pass the extractor function as format_elem parameter.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants