Skip to content

IAudioStreamPlaybackResampled requires useless implementation of "mix" #1133

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

Open
djcsdy opened this issue Apr 20, 2025 · 12 comments · May be fixed by #1136
Open

IAudioStreamPlaybackResampled requires useless implementation of "mix" #1133

djcsdy opened this issue Apr 20, 2025 · 12 comments · May be fixed by #1136
Labels
c: engine Godot classes (nodes, resources, ...) quality-of-life No new functionality, but improves ergonomics/internals

Comments

@djcsdy
Copy link

djcsdy commented Apr 20, 2025

Consider the following minimal implementation of AudioStream and AudioStreamPlaybackResampled:

use godot::classes::native::AudioFrame;
use godot::classes::{AudioStreamPlayback, IAudioStream, IAudioStreamPlaybackResampled};
use godot::prelude::*;

#[derive(GodotClass)]
#[class(init, base=AudioStream)]
struct Foo {}

#[godot_api]
impl IAudioStream for Foo {
    fn instantiate_playback(&self) -> Option<Gd<AudioStreamPlayback>> {
        Some(FooPlayback::new_gd().upcast())
    }
}

#[derive(GodotClass)]
#[class(init, base=AudioStreamPlaybackResampled)]
struct FooPlayback {}

#[godot_api]
impl IAudioStreamPlaybackResampled for FooPlayback {
    unsafe fn mix_resampled(&mut self, _dst_buffer: *mut AudioFrame, frame_count: i32) -> i32 {
        godot_print!("mix_resampled");
        frame_count
    }

    fn get_stream_sampling_rate(&self) -> f32 {
        8000.0
    }

    unsafe fn mix(&mut self, _buffer: *mut AudioFrame, _rate_scale: f32, frames: i32) -> i32 {
        godot_print!("mix");
        frames
    }
}

Include the above code in a godot-rust project, then open the corresponding Godot project.

Create a scene containing an AudioStreamPlayer.

Attach a new instance of Foo as the stream property of the AudioStreamPlayer in the inspector.

Tick Autoplay in the inspector.

Run the scene.

Godot produces the following output:

mix_resampled
mix_resampled
mix_resampled
mix_resampled
mix_resampled
mix_resampled
...etc

No sound plays because the minimal implementation doesn't actually generate any sound, but if it did (in mix_resampled) then that would be working as expected.

However, notice that IAudioStreamPlaybackResampled requires an implementation of mix but Godot never calls it. I assume Godot always calls its internal implementation of AudioStreamPlaybackResampled::mix, which calls mix_resampled and then resamples the sound for us.

@Bromeon
Copy link
Member

Bromeon commented Apr 20, 2025

Not sure if I'm missing something -- but if GDExtension exposes APIs that have to be implemented but serve no purpose, that seems like a bug in Godot and should probably be reported there?

@djcsdy
Copy link
Author

djcsdy commented Apr 20, 2025

You are probably right. I will try to find some time to dig into GDExtension and figure out what's going on.

@djcsdy
Copy link
Author

djcsdy commented Apr 20, 2025

I am wondering if the problem is something like:

  • AudioStreamPlayback has abstract method mix
  • AudioStreamPlaybackResampled extends AudioStreamPlayback, implements mix, but has its own abstract method mix_resampled.

I'm wondering if the tool that generates bindings for godot-rust gets confused and treats abstract methods of the base class AudioStreamPlayback as also being abstract in AudioStreamPlaybackResampled.

So far I haven't investigated enough to know any more than what the symptoms are and how to work around them, so it's quite possible the problem is in GDExtension/Godot itself.

@djcsdy
Copy link
Author

djcsdy commented Apr 20, 2025

btw @Bromeon I am a bit worried that me reporting a flurry of bugs today might have come across as relentless negativity, which is not what I intended at all!

What has happened here is that I've been using godot-rust for a game jam all week, and now that it is over I'm reporting the handful of issues I encountered.

But I want to be clear that I am very happy and impressed with godot-rust! Thank you to you and the team for all your hard work! :-)

@Bromeon
Copy link
Member

Bromeon commented Apr 20, 2025

That sounds plausible 🤔 first, it's somewhat a violation of LSP if further-derived classes aren't supposed to overwrite a virtual method.


But theoreticals aside, the problem is that in extension_api.json, class AudioStreamPlaybackResampled only declares _mix_resampled:

{
			"name": "AudioStreamPlaybackResampled",
			"is_refcounted": true,
			"is_instantiable": true,
			"inherits": "AudioStreamPlayback",
			"api_type": "core",
			"methods": [
				{
					"name": "_mix_resampled",
					"is_const": false,
					"is_static": false,
					"is_required": true,
					"is_vararg": false,
					"is_virtual": true,
					"hash": 50157827,
					"return_value": {
						"type": "int",
						"meta": "int32"
					},
					"arguments": [
						{
							"name": "dst_buffer",
							"type": "AudioFrame*"
						},
						{
							"name": "frame_count",
							"type": "int",
							"meta": "int32"
						}
					]
				},
				{
					"name": "_get_stream_sampling_rate",
					"is_const": true,
					"is_static": false,
					"is_required": true,
					"is_vararg": false,
					"is_virtual": true,
					"hash": 1740695150,
					"return_value": {
						"type": "float",
						"meta": "float"
					}
				},
				{
					"name": "begin_resample",
					"is_const": false,
					"is_vararg": false,
					"is_static": false,
					"is_virtual": false,
					"hash": 3218959716
				}
			]
		},

Whereas its base class AudioStreamPlayback declares _mix and marks it required (field is_required), meaning it must be overridden:

		{
			"name": "AudioStreamPlayback",
			"is_refcounted": true,
			"is_instantiable": true,
			"inherits": "RefCounted",
			"api_type": "core",
			"methods": [
				...
				{
					"name": "_mix",
					"is_const": false,
					"is_static": false,
					"is_required": true,
					"is_vararg": false,
					"is_virtual": true,
					"hash": 925936155,
					"return_value": {
						"type": "int",
						"meta": "int32"
					},
					"arguments": [
						{
							"name": "buffer",
							"type": "AudioFrame*"
						},
						{
							"name": "rate_scale",
							"type": "float",
							"meta": "float"
						},
						{
							"name": "frames",
							"type": "int",
							"meta": "int32"
						}
					]
				},
			],

So I wonder if Godot should re-declare the method in the derived class, marking it as non-required this time. I don't think GDExtension does this anywhere else, and there's probably some benefits of having every method appear only once.

But it's just that again (like in the many other issues), godot-rust is constrained by what the API offers here 🙂 we can of course build manual edge cases, but I'd prefer to discuss this one with Godot maintainers first...

@Bromeon
Copy link
Member

Bromeon commented Apr 20, 2025

btw @Bromeon I am a bit worried that me reporting a flurry of bugs today might have come across as relentless negativity, which is not what I intended at all!

What has happened here is that I've been using godot-rust for a game jam all week, and now that it is over I'm reporting the handful of issues I encountered.

But I want to be clear that I am very happy and impressed with godot-rust! Thank you to you and the team for all your hard work! :-)

No worries at all, I appreciate the feedback! Your reports have been very constructive and detailed, as well 👍

You may have noticed there's a pattern of blurred responsibility between the binding (godot-rust) and the engine (Godot) when it comes to the GDExtension interface 😉 in some cases we need to take action in Rust, because Godot simply doesn't have certain concepts (thread-safety / Send), whereas in others it's possibly a matter of API spec.

Btw, may I ask which game jam that was? 🙂

@djcsdy
Copy link
Author

djcsdy commented Apr 20, 2025

It was I, Rebel: A Jeff Minter Game Jam organised by Atari.

Unfortunately I didn't finish my game. Not for lack of trying. I've been struggling to finish games for quite some time for various reasons, but this is the closest I've come for a long time and the smooth integration between Godot and godot-rust helped a great deal.

I'll be continuing to work on the game and I'll let you know how it goes if you're interested.

There were some cool entries from other developers. Not all of them have been made public yet but some of them are here: https://itch.io/c/5660983/i-rebel-2025-atari-game-jam

@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: engine Godot classes (nodes, resources, ...) labels Apr 21, 2025
@Bromeon
Copy link
Member

Bromeon commented Apr 21, 2025

@djcsdy I created PR #1136 that addresses this. While I still think this is Godot responsibility, this is quite a small QoL change that may not be worth extending the JSON spec, so we can handle derived virtual methods manually in godot-rust for now.

If you have some time, could you look if you find other such cases in Godot APIs and let me know? Just that we don't have to add 20 individual PRs everytime this comes up 😅

@djcsdy
Copy link
Author

djcsdy commented Apr 21, 2025

Thanks!

Now that I see how everything fits together I am pretty sure this applies to every class that extends AudioStreamPlayback. I'll verify this in the next few days.

@Bromeon
Copy link
Member

Bromeon commented Apr 27, 2025

@djcsdy did you confirm any more classes? Would be nice to merge #1136 soon.

@djcsdy
Copy link
Author

djcsdy commented Apr 29, 2025

Hey @Bromeon, I've been a bit busy. I'm going to try to check this right now, but may not get it done until the weekend.

@djcsdy
Copy link
Author

djcsdy commented Apr 29, 2025

I investigated and found that in practice Godot does not allow native code to extend most of the AudioStreamPlayback classes. I've added a list of the classes that it's illegal to extend to #1129.

The AudioStreamPlayback classes that it's legal to extend are:

  • AudioStreamPlayback itself
  • AudioStreamPlaybackResampled
  • AudioStreamPlaybackOggVorbis

AudioStreamPlayback

I don't see any problems with godot-rust's bindings to this class. Godot requires the programmer to implement mix, and godot-rust correctly enforces this requirement.

AudioStreamPlaybackResampled

As already stated, godot-rust requires an implementation of mix but Godot will never call that implementation, it always calls mix from the base class. Fixed by #1136.

AudioStreamPlaybackOggVorbis

Godot allows extending this class, but my brief investigation suggests that may be a bug in Godot. I don't think it's possible to extend this class safely in native code. So godot-rust might consider disallowing this.

godot-rust requires an implementation of mix but Godot will never call that implementation, it always calls mix from the base class. This may already be fixed by #1136 because AudioStreamPlaybackOggVorbis extends AudioStreamPlaybackResampled.

godot-rust also requires an implementation of mix_resampled and get_stream_sampling_rate but these are both provided by the base class. I am not sure if overrides of these functions will ever be called by Godot in practice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: engine Godot classes (nodes, resources, ...) quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants