Add return values when generating virtual methods #207

jbarnoud · 2023-03-27T21:14:11Z

Fixes #190

Some node methods are expected to return values. The corresponding virtual methods, however, do not reflect that.

For instance, this is the virtual method Node.get_configuration_warnings before this commit:

fn get_configuration_warnings(&self) {
    unimplemented!()
}

This is the same method with this commit:

fn get_configuration_warnings(&self) -> PackedStringArray {
    unimplemented!()
}

This commit adapts what I think is the relevant code from
godot_codegen::class_generator::make_method_definition into make_virtual_method.

Note that I am very unfamiliar with most of the code I had to touch here as well as with the project.

Bromeon

Thanks a lot for this PR! 🙂

It looks like you copy-pasted a lot of code from other parts of class_generator.rs. Would it be possible to reuse that code instead, so it's only defined once -- if necessary with extra functions?
Could you maybe add a test that uses a method with a return type? There are existing tests in itest/rust/src/virtual_methods_test.rs.

Also, in the end please squash commits to 1 😉

lilizoey · 2023-03-28T12:18:25Z

a minor note, many of these virtual methods will likely not work until #204 is merged. basically any virtual method call that takes/returns anything but pass-by-value types are currently broken (excluding the receiver), which likely includes the one you're referencing too.

as a workaround, string types, arrays, and dictionaries need to have a clone/share std::mem::forget before the function returns. Gd types just do not work though

jbarnoud · 2023-03-28T12:22:08Z

I'll wait for #204 to move on before fixing the PR, then 😃

jbarnoud · 2023-04-01T09:08:52Z

I implemented a basic test that calls an overloaded virtual method. The test passes, but I now get the following warning:

WARNING: ObjectDB instances leaked at exit (run with --verbose for details).
     at: cleanup (core/object/object.cpp:1982)

@SayakS Is this the kind of thing you are fixing on #204, or is it something I need to look at?

lilizoey · 2023-04-01T18:35:30Z

I implemented a basic test that calls an overloaded virtual method. The test passes, but I now get the following warning:
WARNING: ObjectDB instances leaked at exit (run with --verbose for details).
     at: cleanup (core/object/object.cpp:1982)
@SayakS Is this the kind of thing you are fixing on #204, or is it something I need to look at?

This should be fixed by #204 yes. if you want you can try and see if it does make this test pass, i can't really test it in that PR since it uses a return type. It would actually be pretty good to have tested.

Bromeon · 2023-04-02T07:39:16Z

Trying with latest CI changes...

bors try

bors · 2023-04-02T08:03:47Z

try

Build failed:

full-ci

jbarnoud · 2023-04-03T08:57:05Z

itest/rust/src/virtual_methods_test.rs

+    let output = obj.bind().get_configuration_warnings();
+    assert!(output.contains("Hello".into()));
+    assert_eq!(output.len(), 1);
+    obj.bind_mut().queue_free();


This line missing was the cause of the memory leak and, therefore, of the pipeline failing. Another way of solving the memory leak would be to add the obj node to the test_context which, I guess, gets cleaned at the end of the test. I went for queue_free because it looked more concise, but I do not know if there is a recommended practice here.

oh yeah no that makes sense. nodes outside of the scene tree need to be manually freed.

You can use free() directly, no? If yes, that should be preferred over queue_free().

Also, you can apply that directly on the Gd pointer:

obj.free();

jbarnoud · 2023-04-03T12:22:52Z

I added a test and refactored the code to avoid repetitions. I also rebased against master and squashed the commits. I think this is ready for review.

I haven't seen issues regarding #204. Just to be sure, I tested locally with both this PR and #204 merged together and the tests passed all the same.

The memory leak I encountered earlier had nothing to do with #204, but was due to me not cleaning after myself. See my comment above.

Bromeon · 2023-04-03T16:09:55Z

bors try

bors · 2023-04-03T16:23:01Z

try

Build succeeded:

full-ci

Bromeon

Thanks! Looks mostly good to me 🙂

At first sight I was a bit concerned that the is_virtual code path in make_function_definition() adds a lot of special-casing for relatively few functionality, as it doesn't use a lot of the function codegen logic. But overall I think it's nice to handle those things in one place, and it can also prevent bugs like this one. Thanks for the refactoring! 👍

Bromeon · 2023-04-04T21:11:14Z

godot-codegen/src/class_generator.rs

+        (true, _, _) => {
+            quote! {
+                unimplemented!()
+            }
+        }


Could you maybe move this to the top?
Then it's immediately clear that no more statements handle is_virtual == true.

Bromeon · 2023-04-04T21:14:29Z

itest/rust/src/virtual_methods_test.rs

+    let output = obj.bind().get_configuration_warnings();
+    assert!(output.contains("Hello".into()));
+    assert_eq!(output.len(), 1);
+    obj.bind_mut().queue_free();


You can use free() directly, no? If yes, that should be preferred over queue_free().

Also, you can apply that directly on the Gd pointer:

obj.free();

jbarnoud · 2023-04-04T21:30:25Z

Done 👍

Fixes godot-rust#190 Some node methods are expected to return values. The corresponding virtual methods, however, do not reflect that. For instance, this is the virtual method `Node.get_configuration_warnings` before this commit: ```rust fn get_configuration_warnings(&self) { unimplemented!() } ``` This is the same method with this commit: ```rust fn get_configuration_warnings(&self) -> PackedStringArray { unimplemented!() } ``` This commit makes `godot_codegen::clas_generator::make_function_definition` and `make_return` aware of virtual method. It also adds a test making sure we can call a virtual method that has a return value.

Bromeon

Thanks a lot!

bors r+

bors · 2023-04-05T08:30:44Z

Build succeeded:

full-ci

Bromeon reviewed Mar 28, 2023

View reviewed changes

Bromeon added bug c: engine Godot classes (nodes, resources, ...) labels Mar 28, 2023

lilizoey mentioned this pull request Mar 31, 2023

Add abstraction for ScriptInstanceExtension #196

Closed

bors bot added a commit that referenced this pull request Apr 2, 2023

Try #207:

864b06e

jbarnoud commented Apr 3, 2023

View reviewed changes

jbarnoud force-pushed the virtual_method_returns branch from c56ab9a to 5e255f1 Compare April 3, 2023 12:10

bors bot added a commit that referenced this pull request Apr 3, 2023

Try #207:

02dd8eb

bors bot added a commit that referenced this pull request Apr 3, 2023

Try #207:

d83307b

Bromeon reviewed Apr 4, 2023

View reviewed changes

jbarnoud force-pushed the virtual_method_returns branch from 5e255f1 to 9822843 Compare April 4, 2023 21:28

jbarnoud force-pushed the virtual_method_returns branch from 9822843 to e81d5d1 Compare April 4, 2023 21:41

Bromeon approved these changes Apr 5, 2023

View reviewed changes

bors bot merged commit 04c2323 into godot-rust:master Apr 5, 2023

jbarnoud deleted the virtual_method_returns branch April 5, 2023 08:34

lilizoey mentioned this pull request Apr 19, 2023

Project status overview #24

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add return values when generating virtual methods #207

Add return values when generating virtual methods #207

jbarnoud commented Mar 27, 2023

Bromeon left a comment •

edited

Loading

lilizoey commented Mar 28, 2023 •

edited

Loading

jbarnoud commented Mar 28, 2023

jbarnoud commented Apr 1, 2023

lilizoey commented Apr 1, 2023 •

edited

Loading

Bromeon commented Apr 2, 2023

bors bot commented Apr 2, 2023

jbarnoud Apr 3, 2023

lilizoey Apr 3, 2023

Bromeon Apr 4, 2023

jbarnoud commented Apr 3, 2023

Bromeon commented Apr 3, 2023

bors bot commented Apr 3, 2023

Bromeon left a comment

Bromeon Apr 4, 2023

Bromeon Apr 4, 2023

jbarnoud commented Apr 4, 2023

Bromeon left a comment

bors bot commented Apr 5, 2023

Add return values when generating virtual methods #207

Add return values when generating virtual methods #207

Conversation

jbarnoud commented Mar 27, 2023

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

lilizoey commented Mar 28, 2023 • edited Loading

jbarnoud commented Mar 28, 2023

jbarnoud commented Apr 1, 2023

lilizoey commented Apr 1, 2023 • edited Loading

Bromeon commented Apr 2, 2023

bors bot commented Apr 2, 2023

try

jbarnoud Apr 3, 2023

Choose a reason for hiding this comment

lilizoey Apr 3, 2023

Choose a reason for hiding this comment

Bromeon Apr 4, 2023

Choose a reason for hiding this comment

jbarnoud commented Apr 3, 2023

Bromeon commented Apr 3, 2023

bors bot commented Apr 3, 2023

try

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon Apr 4, 2023

Choose a reason for hiding this comment

Bromeon Apr 4, 2023

Choose a reason for hiding this comment

jbarnoud commented Apr 4, 2023

Bromeon left a comment

Choose a reason for hiding this comment

bors bot commented Apr 5, 2023

Bromeon left a comment •

edited

Loading

lilizoey commented Mar 28, 2023 •

edited

Loading

lilizoey commented Apr 1, 2023 •

edited

Loading