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

AudioStream/Playback only exposes virtual methods #1328

Open
stechyo opened this issue Dec 5, 2023 · 3 comments
Open

AudioStream/Playback only exposes virtual methods #1328

stechyo opened this issue Dec 5, 2023 · 3 comments
Labels
enhancement This is an enhancement on the current functionality
Milestone

Comments

@stechyo
Copy link

stechyo commented Dec 5, 2023

Godot version

4.2

godot-cpp version

4.2

System information

Linux

Issue description

AudioStream and AudioStreamPlayback only expose virtual methods. This is sufficient for creating total overrides of existing stream/playbacks, but not for e.g. intercepting an existing stream playback's mixed audio frames and changing them. Calling a native stream playback's _mix() returns 0 mixed frames, presumably because of the GDVIRTUAL macros used in the engine code (forgive any misunderstanding, but I presume that when those macros are removed what gets generated into extension bindings is the non-virtual method, similar to what's exposed on GDScript/C#).

How feasible would it be to allow direct use of audio methods?

Thank you!

Steps to reproduce

Attempt to call _mix for a native stream playback and check that zero frames have been mixed.

// stream_playback is a Ref<AudioStreamPlaybackMP3> in this case
AudioFrame input[frames];
int mixed = stream_playback->_mix(input, rate_scale, frames);
// mixed == 0, all audio frames in `input` are zero

Minimal reproduction project

N/A

@dsnopek
Copy link
Contributor

dsnopek commented Dec 7, 2023

Thanks for the report!

So, you want to be able to get an instance of, say, AudioStreamPlaybackMP3 and call it's mix() method (NOTE: without the leading _ score)? I think that's doable. Someone would need make a PR changing AudioStreamPlayback::_bind_methods() to bind the mix() method. Does it make sense to bind the other non-virtual methods too?

Is that a PR that you're interested in making? It'd be a fairly simple contribution to the engine :-)

@dsnopek dsnopek added the enhancement This is an enhancement on the current functionality label Dec 7, 2023
@dsnopek dsnopek added this to the 4.x milestone Dec 7, 2023
@stechyo
Copy link
Author

stechyo commented Dec 7, 2023

Yeah, I can do that. Just wanted to understand if there wasn't any opposition to exposing the method to GDExt. I'll see if I can publish a PR today, thanks!

@dsnopek
Copy link
Contributor

dsnopek commented Dec 7, 2023

I don't think there'll be any opposition from the GDExtension team - the audio team may have reasons they don't want those exposed, but they don't tend to see godot-cpp issues. :-) You'll probably need their approval on the Godot PR, so you could discuss it with them there, or if you want to discuss before writing code, you could make a Godot issue or proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality
Projects
None yet
Development

No branches or pull requests

2 participants