Allow godot-visible functions to receive a Gd instead of self. #396

mhoff12358 · 2023-09-04T03:49:48Z

This is done by tagging the function with #[func(gd_self)].

I haven't written tests because there was an unrelated issue on my machine, but I have tested with a different project and it seems to work.

godot-macros/src/class/data_models/func.rs

GodotRust · 2023-09-04T03:55:59Z

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

gg-yb · 2023-09-04T06:05:38Z

Did you test whether this solves the issue in #338 ?

mhoff12358 · 2023-09-04T06:12:09Z

It doesn't solve #338 because that involves implementing a trait that already defines on_notification to use &mut self.
I did test that this does enable interleaving rust and gdscript function calls/signals though. So if the trait was rewritten to use this it would work. But there seemed to be a debate on whether that was better or not.

lilizoey · 2023-09-04T08:05:24Z

I'd find it a bit weird for it to not be possible to do this for the trait methods as well as regular methods. It would require a bigger change to codegen to do this though which would probably cause a conflict with #387. that's kinda why i waited with doing this. but it might still be nice to have this for user-defined functions before then.

would it be better to tag the argument rather than the function?

also there should definitely be some tests for this.

i dont think using .get() is the right approach for getting the base field. since it means you can't use this on classes without a base field, and it means we can't call a method that takes a Gd<Self> argument if we currently have a mutable reference to self. adding a Base to InstanceStorage seems like a better idea to me at least.

mhoff12358 · 2023-09-04T23:39:56Z

Yeah, agreed that it'd be way better if this could also be done for trait methods. But I'm not sure how to address the issues around not wanting to require it everywhere. I think it's better to have it at least available for user-defined functions, but I could see preferring consistency. Do you think this should be shelved until there's an answer for what to do for traits?
I could see tagging the argument instead. It was really easy to implement this way, but if you think that's better I'll take a look at it when I have a chance.
Yeah, that's one of the reasons I tried to make it clear it's a draft. I need to figure out why the tests are failing on the master branch on my machine before I can write new ones, but that needs to happen before this could be merged.
You said earlier you wrote the code for adding a Base<> to InstanceStorage. Do you want to merge that and I can rebase on top of it? Or share it in some other way for me to include?

gg-yb · 2023-09-05T04:55:35Z

Wouldn't it be possible to scan for whether the type of the first parameter is Gd<Self> (or equivalent Gd<T> when implemented on T) when collecting `#[func]'s? That way no tag would be needed.

lilizoey · 2023-09-05T23:08:31Z

Wouldn't it be possible to scan for whether the type of the first parameter is Gd<Self> (or equivalent Gd<T> when implemented on T) when collecting `#[func]'s? That way no tag would be needed.

how do you distinguish between that and a static method that takes a Gd<Self> as its first argument?

lilizoey · 2023-09-05T23:10:09Z

4. You said earlier you wrote the code for adding a `Base<>` to `InstanceStorage`. Do you want to merge that and I can rebase on top of it? Or share it in some other way for me to include?

i could yeah, i didn't test it very much. i just did it quickly to see what would be needed to implement this. I'll see if i can find it again.

gg-yb · 2023-09-06T05:59:35Z

Wouldn't it be possible to scan for whether the type of the first parameter is Gd<Self> (or equivalent Gd<T> when implemented on T) when collecting `#[func]'s? That way no tag would be needed.

how do you distinguish between that and a static method that takes a Gd<Self> as its first argument?

My bad, thought rust-lang/rust#44874 was already stabilized, which would have lended itself for this purpose. You are right, it does not work if we cannot name the Gd parameter self

lilizoey · 2023-09-06T17:45:12Z

@mhoff12358 here: https://github.com/lilizoey/gdextension/tree/feature/instance-storage-get-gd. I haven't tested this much beyond just checking that it works (by manually calling all the functions). But i think this should work.

mhoff12358 · 2023-09-10T05:29:09Z

I've pulled in lilizoey's commit adding a base field to InstanceStorage, which does seem much better.
I still haven't gotten to figuring out why the tests weren't working on my machine to write new tests, so this'll stay a draft until I get a little more time for this.

lilizoey · 2023-09-10T14:26:31Z

godot-core/src/storage.rs

@@ -113,7 +122,7 @@ mod multi_threaded {
    use std::sync;
    use std::sync::atomic::{AtomicU32, Ordering};

-    use crate::obj::GodotClass;
+    use crate::obj::{Base, Gd, GodotClass, Share};


Oh i forgot to add Inherits as an import here, that's why the CI failed.

mhoff12358 · 2023-09-15T06:48:33Z

Alright, tests finally added. It's a really simple case, but still.

Lemme know if you'd like it to do something more complex.

mhoff12358 · 2023-09-15T07:05:30Z

Whoops, the test broke clippy. Seems I introduced a warning. But I really need to get to bed, I'll fix it tomorrow

Bromeon

Thanks a lot, this is a nice feature on the road to #338!

godot-core/src/obj/base.rs

godot-core/src/storage.rs

godot-macros/src/class/data_models/func.rs

itest/godot/ManualFfiTests.gd

itest/rust/src/register_tests/func_test.rs

godot-macros/src/class/godot_api.rs

mhoff12358 · 2023-09-17T21:54:28Z

I improved the test case to actually trigger the error condition.

Bromeon

Thanks for the update! 🙂
Some more comments. There are also some conflicts at the moment.

itest/godot/TestRunner.gd

godot-core/src/obj/base.rs

itest/rust/src/register_tests/func_test.rs

godot-macros/src/class/godot_api.rs

Bromeon · 2023-09-18T07:50:08Z

godot-macros/src/class/godot_api.rs

+                        sig.params[0].0 = FnParam::Receiver(FnReceiverParam {
+                            attributes: vec![],
+                            tk_ref: Some(proc_macro2::Punct::new('&', proc_macro2::Spacing::Alone)),
+                            tk_mut: None,
+                            tk_self: Ident::new("self", proc_macro2::Span::call_site()),
+                        });


You construct a &self parameter here, but what is this used for?
And how do you know it's not &mut self?

This is an attempt to simplify func.rs's get_signature_info method. Now there's no case where both gd_self being true and the first argument being &self or &mut self can conflict set receiver_type twice.
I don't love this, and could also go back and just add a panic! to get_signature_info if the first argument is &self.

Thinking on it again, there's already a panic! in get_signature_info.
I think it makes sense to inelegantly handle the case where gd_self is true and the first argument is &self in get_signature_info with a panic! as a fallback. And then have process_godot_fns check for that case and emit a prettier looking bail error instead.

I'll upload a separate commit that switches to that, so it'll be easier to go with which you prefer.

godot-macros/src/class/godot_api.rs

mhoff12358 · 2023-09-22T06:21:11Z

The test case that triggers the error seems to cause the integration test runner to fail on any non-windows machine. Even though every individual test passes.
It's probably better to just delete the case that verifies that an error occurs? It makes it more clear that this is actually fixing the issue, but isn't strictly necessary to catch any future regressions.

Bromeon · 2023-09-22T09:39:09Z

CI checks for the string SCRIPT ERROR: in the stderr output and aborts immediately if it sees that. It's a bit curious that this doesn't happen on Windows though...

What you could try is explicitly disable type annotations by using Variant:

var gd_self_reference: Variant
# or just:
var gd_self_reference

instead of:

var gd_self_reference: GdSelfReference

This should move the error from parse time to runtime.

Bromeon · 2023-09-30T08:41:08Z

@mhoff12358 Do you think you'll have time to update this over the next few days? 🙂

mhoff12358 · 2023-09-30T13:50:05Z

Yeah, hopefully tonight or tomorrow now that it's the weekend.

…

On Sat, Sep 30, 2023, 3:41 AM Jan Haller ***@***.***> wrote: @mhoff12358 <https://github.com/mhoff12358> Do you think you'll have time to update this over the next few days? 🙂 — Reply to this email directly, view it on GitHub <#396 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHXX7ZUKRRQGBLUYXXQBTDX47LK7ANCNFSM6AAAAAA4JZO4NE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

This is done by tagging the function with #[func(gd_self)].

mhoff12358 · 2023-10-01T15:02:24Z

CI checks for the string SCRIPT ERROR: in the stderr output and aborts immediately if it sees that. It's a bit curious that this doesn't happen on Windows though...

What you could try is explicitly disable type annotations by using Variant:
var gd_self_reference: Variant
# or just:
var gd_self_reference
instead of:
var gd_self_reference: GdSelfReference
This should move the error from parse time to runtime.

This actually is a runtime error. Godot spits out "SCRIPT ERROR: Invalid call. Nonexistent function 'function_name' in base 'RustClass'." at runtime if function_name does exist but panics inside rust.

I don't see any easy way to do something that would assert for that error and prevent it from causing a real failure, so I commented out the test with a todo to re-add it if someone does build a way to depend on that kind of failure.

It looks like tests are passing, other than an issue with credentials in the docs test. (They're currently rerunning because I rebased to not have an unnecessary extra commit).

Were there any other outstanding issues?

Bromeon

Thanks! The docs is something I have to address separately, but it's not part of the main CI.

mhoff12358 force-pushed the mhoff12358/gd_as_self_argument branch from 4418908 to 4b9c57a Compare September 4, 2023 03:51

mhoff12358 commented Sep 4, 2023

View reviewed changes

godot-macros/src/class/data_models/func.rs Outdated Show resolved Hide resolved

mhoff12358 mentioned this pull request Sep 4, 2023

Allow custom receivers in virtual methods #359

Open

Bromeon added c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library labels Sep 4, 2023

mhoff12358 force-pushed the mhoff12358/gd_as_self_argument branch 2 times, most recently from 76ec1f5 to 0dae372 Compare September 10, 2023 05:25

lilizoey reviewed Sep 10, 2023

View reviewed changes

mhoff12358 force-pushed the mhoff12358/gd_as_self_argument branch from 0dae372 to 1d66d7a Compare September 15, 2023 06:46

Bromeon requested changes Sep 15, 2023

View reviewed changes

mhoff12358 force-pushed the mhoff12358/gd_as_self_argument branch 2 times, most recently from 9f79a6d to 9f04e1f Compare September 17, 2023 21:52

mhoff12358 changed the title ~~Draft: Allow godot-visible functions to receive a Gd instead of self.~~ Allow godot-visible functions to receive a Gd instead of self. Sep 17, 2023

mhoff12358 mentioned this pull request Sep 17, 2023

Implicit binds in virtual calls cause borrow errors #338

Closed

Bromeon reviewed Sep 18, 2023

View reviewed changes

mhoff12358 force-pushed the mhoff12358/gd_as_self_argument branch from 9f04e1f to 23fbfd3 Compare September 21, 2023 04:59

mhoff12358 force-pushed the mhoff12358/gd_as_self_argument branch 4 times, most recently from c2b9dfb to 94f4866 Compare September 21, 2023 05:28

Add a get_gd method to InstanceStorage

3151842

mhoff12358 force-pushed the mhoff12358/gd_as_self_argument branch 2 times, most recently from 9853ffc to cd921c7 Compare September 22, 2023 06:02

mhoff12358 force-pushed the mhoff12358/gd_as_self_argument branch 2 times, most recently from 3d71798 to d33813c Compare October 1, 2023 13:55

Allow godot-visible functions to receive a Gd instead of self.

c28de15

This is done by tagging the function with #[func(gd_self)].

mhoff12358 force-pushed the mhoff12358/gd_as_self_argument branch from d33813c to c28de15 Compare October 1, 2023 15:01

Bromeon approved these changes Oct 1, 2023

View reviewed changes

Bromeon added this pull request to the merge queue Oct 1, 2023

Merged via the queue into godot-rust:master with commit c3e7023 Oct 1, 2023
13 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow godot-visible functions to receive a Gd instead of self. #396

Allow godot-visible functions to receive a Gd instead of self. #396

mhoff12358 commented Sep 4, 2023

GodotRust commented Sep 4, 2023

gg-yb commented Sep 4, 2023

mhoff12358 commented Sep 4, 2023

lilizoey commented Sep 4, 2023

mhoff12358 commented Sep 4, 2023

gg-yb commented Sep 5, 2023

lilizoey commented Sep 5, 2023

lilizoey commented Sep 5, 2023

gg-yb commented Sep 6, 2023

lilizoey commented Sep 6, 2023

mhoff12358 commented Sep 10, 2023

lilizoey Sep 10, 2023

mhoff12358 commented Sep 15, 2023 •

edited

Loading

mhoff12358 commented Sep 15, 2023

Bromeon left a comment

mhoff12358 commented Sep 17, 2023

Bromeon left a comment

Bromeon Sep 18, 2023

mhoff12358 Sep 21, 2023

mhoff12358 Sep 21, 2023

mhoff12358 commented Sep 22, 2023

Bromeon commented Sep 22, 2023 •

edited

Loading

Bromeon commented Sep 30, 2023

mhoff12358 commented Sep 30, 2023 via email

mhoff12358 commented Oct 1, 2023

Bromeon left a comment •

edited

Loading

Allow godot-visible functions to receive a Gd instead of self. #396

Allow godot-visible functions to receive a Gd instead of self. #396

Conversation

mhoff12358 commented Sep 4, 2023

GodotRust commented Sep 4, 2023

gg-yb commented Sep 4, 2023

mhoff12358 commented Sep 4, 2023

lilizoey commented Sep 4, 2023

mhoff12358 commented Sep 4, 2023

gg-yb commented Sep 5, 2023

lilizoey commented Sep 5, 2023

lilizoey commented Sep 5, 2023

gg-yb commented Sep 6, 2023

lilizoey commented Sep 6, 2023

mhoff12358 commented Sep 10, 2023

lilizoey Sep 10, 2023

Choose a reason for hiding this comment

mhoff12358 commented Sep 15, 2023 • edited Loading

mhoff12358 commented Sep 15, 2023

Bromeon left a comment

Choose a reason for hiding this comment

mhoff12358 commented Sep 17, 2023

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon Sep 18, 2023

Choose a reason for hiding this comment

mhoff12358 Sep 21, 2023

Choose a reason for hiding this comment

mhoff12358 Sep 21, 2023

Choose a reason for hiding this comment

mhoff12358 commented Sep 22, 2023

Bromeon commented Sep 22, 2023 • edited Loading

Bromeon commented Sep 30, 2023

mhoff12358 commented Sep 30, 2023 via email

mhoff12358 commented Oct 1, 2023

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

mhoff12358 commented Sep 15, 2023 •

edited

Loading

Bromeon commented Sep 22, 2023 •

edited

Loading

Bromeon left a comment •

edited

Loading