Required virtual methods should be required at compile-time

[TitanNano]

Fixes #192.

I assembled the list of required methods via

rg "GDVIRTUAL_REQUIRED_CALL" -g *.cpp -g *.h -g !thirdparty

and

rg "EXBIND[^(]*" -g *.cpp -g *.h -g !thirdparty

(which internally calls GDVIRTUAL_REQUIRED_CALL).

For classes where all virtual methods are required, I just match the class name to reduce the length of the list. Additionally, I grouped all match patterns by their class name for readability.

---

TODO:

• re-check ScriptExtension for required functions. (See: Required virtual method (not?) overriding  #826 (comment))
• fix or remove native structure passing itest

[GodotRust]

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

[Bromeon]

Hey, thanks a lot!

When seeing how much boilerplate code this imposes, I have second thoughts about this, see comment.

One thing we don't know is whether "required" always implies "is indeed called". Because in all the cases where a required virtual method is not called, we now impose useless code on the user.

[Bromeon]

OK, I can't come up with a better solution for the moment, so to prevent this PR from landing in review limbo, I'd say we move ahead 🚚

Could you rebase on master and address the open code style feedback?

To avoid excessively many lines for the boilerplate code, we could compact it. Instead of

    fn get_surface_count(&self) -> i32 {
        unreachable!()
    }

    fn surface_get_array_len(&self, _index: i32) -> i32 {
        unreachable!()
    }

    fn surface_get_array_index_len(&self, _index: i32) -> i32 {
        unreachable!()
    }

, could you do

    fn get_surface_count(&self) -> i32 { unreachable!() }
    fn surface_get_array_len(&self, _index: i32) -> i32 { unreachable!() }
    fn surface_get_array_index_len(&self, _index: i32) -> i32 { unreachable!() }

And then use #[rustfmt::skip] on the impl? This at least brings down the number of lines per method from 4 to 1, and thus requires less scrolling to get to the useful parts.

---

For the native_structures_test.rs, let's look for alternatives -- one might be AudioEffectInstance::process() accepting *mut AudioFrame. Not sure if/how easily we could simulate it though. Any opinions on this?

[TitanNano]

For the native_structures_test.rs, let's look for alternatives -- one might be AudioEffectInstance::process() accepting *mut AudioFrame. Not sure if/how easily we could simulate it though. Any opinions on this?

I already explored this and while AudioEffectInstance is an attractive class, I did not find a good way to actually test it. The engine does not expose any public methods on the AudioEffectInstance or the AudioServer that would allow testing the effect instance.

It might be possible to use the AudioStreamGenerator to create samples and two AudioEffectIntances to validate correctness of the native structure passing. I will try to look into.

[Bromeon]

Don't spend too much effort on this, otherwise we can remove the test. It's also not ideal if a test about native structs contains 80% audio logic 🙂

Maybe we don't actually need a Godot API for native struct marshalling and could use a user-defined #[func] instead -- they use the same underlying mechanism.

[TitanNano]

@Bromeon it wasn't too complex to replace the out pointer test with an audio effect-based one. It doesn't come with tradeoffs, though:

1. the new test requires a few audio classes, which are currently not part of the minimal codegen.
2. audio is processed in a separate audio thread, and so are the audio effects. The test requires the experimental-threads feature, or it panics.

Maybe we don't actually need a Godot API for native struct marshalling and could use a user-defined #[func] instead -- they use the same underlying mechanism.

I wasn't able to identify a way to pass a native struct raw pointer through the engine to a custom func.

[Bromeon]

Thanks a lot for the ongoing improvements! 👍

---

the new test requires a few audio classes, which are currently not part of the minimal codegen.

Quite a lot, it seems 🤔

use godot::classes::{
    AudioEffect, AudioEffectInstance, AudioServer, AudioStreamGenerator,
    AudioStreamGeneratorPlayback, AudioStreamPlayer, Engine, IAudioEffect, IAudioEffectInstance,
}

I'm not sure about generating extra code for all these classes in every CI. On the other hand, it might be interesting to have some "real-world" examples, and you spent already a lot of effort refining this PR, so we can stay with it for now. Maybe I can still change it at some point in the future.

Could you add a comment (e.g. above one of the entries in SELECTED_CLASSES) why the audio ones are needed, and that it might potentially change?

---

I wasn't able to identify a way to pass a native struct raw pointer through the engine to a custom func.

A nice trick to involve engine roundtrip is to just use the generic reflection mechanism:

// Assuming we have a user-defined #[func] MyClass::custom_func:
let obj: Gd<MyClass> = ...;
let native_arg: *mut CaretInfo = ...;

let variant: Variant = obj.call("custom_func".into(), &[native_arg.to_variant()]);
let native_return: *const CaretInfo = variant.to();

(Yes, the above call should technically be unsafe, at some point we should maybe disallow call() with known unsafe functions and provide a unsafe_call() instead...)

[Bromeon]

Thanks a lot! I feel bad that this devolved into audio tests and CI changes 🙈 unfortunately making virtual methods required is really quite an impactful change...

Rebased onto latest master.

[Bromeon]

Please don't change the default, the whole point of not having full codegen is to allow for fast local development and CI.

Also keep changes to CI only on a minimal level (godot -> itest feature), would be good to not burden existing CI jobs with more workload.

[TitanNano]

The problem I'm trying to solve here is that rust-analyzer (and other IDE tools possibly as well) enable all default features for all workspace crates, which causes godot/__codegen-full to be enabled but itest/godegen-full remains disabled. This causes errors during development. In check.sh, default features are disabled.

[Bromeon]

The original design was that itest would never use full codegen. (The fact that it did through dev-dependencies is a bug).

It's this PR which introduces the differentiation between minimal+full codegen, and with it some problems.

It probably makes sense to have it so that audio tests don't always run, but it seems this has quite a complexity cost:

• more features to be propagated
• another dimension in which tests can run differently (next to platform, API level, threads...) -> combinatoric explosion

I guess it's fine, as I don't have a better solution right now, but I'll probably need to get back to this in the future.

[TitanNano]

Audio tests only run in the experimental Godot itest in CI. This codegen issue comes from the changing IPrimitiveMesh trait, depending on the scope of the codegen. An alternative would be to add the Material class to the minimal codegen.

[Bromeon]

Could you maybe split the CI/tooling changes into commit(s) separate from the virtual-method feature?

[TitanNano]

@Bromeon I was able to track down the dependencies that enabled full codegen. Some crates have a dev-dependency on godot but missed disabling the default features. (Moved this fix to: #842)

I understand that you don't want the itest default feature, but since full codegen is a default feature of the godot crate, I believe the itest/codegen-full feature has to be kept in lock step as best as possible. If we do not make the itest/codegen-full a default feature, running cargo build will cause itest compilation to fail. Compiling all crates in the workspace will enable the godot/__codegen-full feature, but the itest/codegen-full will remain inactive.

---

Currently, the new audio test is leaking some objects, but so far, I was unable to pinpoint what causes it.

[Bromeon]

Rebased.

[Bromeon]

Thanks a lot! 👍 also for all the side quests 🙂