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

Add AudioStreamPlayerInternal to unify stream players #87061

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jan 10, 2024

We have 3 AudioStreamPlayer classes and they share a lot of code. Code duplication, especially this big, is bad and that doesn't need explaining. Every fix or common feature has to be implemented 3 times and there is risk of code differences that result in bugs.

This PR tries to unify the code under a single class - AudioStreamPlayerInternal. It's an internal class used by the 3 audio players (not by inheritance). I delegated all common code and fields to the new class. The best indicator how big was the code duplication is that this PR has more deleted lines despite I added a whole new class. I also cleaned up some includes.

I tried to avoid introducing changes, but there are small differences from the original code, mostly in the order of some lines (some things were ordered differently in different players and common methods will obviously unify that). Probably needs testing to check whether all features are still working and there is no change in behavior. I did some basic tests and the audio still plays at least.

In the meantime I also came up with an alternate approach of taking advantage of C++ multiple inheritance. Maybe I'll try that too, as it should save some more code and possibly make it more readable.

}
internal.ensure_playback_limit();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The playback limit is applied each frame, unlike in AudioStreamPlayer, which does it only on play(). Maybe other players could do that too?

Copy link
Contributor

@MJacred MJacred Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The playback limit is applied each frame, unlike in AudioStreamPlayer, which does it only on play(). Maybe other players could do that too?

Sounds and looks reasonable to me. Then ensure_playback_limit can be removed altogether and its content moved into play_basic

} else {
stop();
}
internal.set_playing(p_enable);
}

bool AudioStreamPlayer::_is_active() const {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method code was the same as is_playing(). I unified it to do active.is_set() as other players, idk if it makes difference.

Copy link
Contributor

@MJacred MJacred Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method code was the same as is_playing(). I unified it to do active.is_set() as other players, idk if it makes difference.

Hm… for AudioStreamPlayer, the meaning changes from "AudioServer has my playback registered and setup, and started playing" to "I started being processed and the AudioServer is about to start setting up my playback".

But no idea if this has any tangible repercussions. _is_active is a Node thing afaik

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if relevant, but does animation use this for syncing the start? Audio already starts earlier than other things. For reference: #82698 (comment)

@Mickeon
Copy link
Contributor

Mickeon commented Jan 10, 2024

In the meantime I also came up with an alternate approach of taking advantage of C++ multiple inheritance. Maybe I'll try that too, as it should save some more code and possibly make it more readable.

I'm very interested. Readability seems to be the must. But, is something like the suggestion ever used across the rest of the codebase?

Whatever path is taken to unite the AudioStreamPlayers, it would be nice in the future to do the same for other classes with a lot of duplicated code.

@KoBeWi KoBeWi force-pushed the shadow_of_the_former_self branch from 65ee045 to 891598b Compare January 10, 2024 21:12
@ellenhp
Copy link
Contributor

ellenhp commented Jan 10, 2024

An important discussion point is whether this may interfere with people creating custom streams with gdextension. I think the answer is no because they can just inherit audiostream and continue duplicating all the code we're consolidating here. But that's a use case I want us to continue supporting so I want to bring it up :)

@Mickeon
Copy link
Contributor

Mickeon commented Jan 10, 2024

Compatibility is retained by doing this. From the outsider's perspective nothing should change.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 10, 2024

But, is something like the suggestion ever used across the rest of the codebase?

No, AFAIK every class is single inheritance.

I tried that approach and seems like you can't bind methods from another class (in that case AudioStreamPlayerInternal would be the secondary parent class), which makes it useless. Maybe there is a way to untangle the inheritance, but it's not that easy, so meh. The solution in this PR is better than what we have anyway.

EDIT:
Seems like there is a crash in CI, but I don't get what is it about.

@ellenhp
Copy link
Contributor

ellenhp commented Jan 10, 2024

Compatibility is retained by doing this. From the outsider's perspective nothing should change.

Yeah makes sense, I'm just trying to be thorough. I also think we should come up with a better name because we have a couple audio streams that won't end up subclassing this new internal class because they're dynamic (microphone, generator, probably others).

I misunderstood this PR actually, oops.

@Mickeon
Copy link
Contributor

Mickeon commented Jan 10, 2024

It could be called something like "Core", since it's the center of all AudioStreamPlayers. I had it for a bit and it's the only thing that comes to mind.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 10, 2024

I also think we should come up with a better name because we have a couple audio streams that won't end up subclassing this new internal class

The new class is for audio stream players (it's AudioStreamPlayerInternal), not audio streams. I think the name is fine.

@ellenhp
Copy link
Contributor

ellenhp commented Jan 10, 2024

Yeah sorry, neither of my comments make any sense for this. I got a covid vaccine about 36 hours ago and I'm still in the living dead phase of recovery. Didn't help that I hadn't looked at the code yet. I do want to refactor streams at some point too but there's a lot less we can do there without breaking stuff. As you noted, you can't create a new player in GDExtension yet so we really don't have to worry about breaking compat, and I'm not even aware of any custom modules that implement their own players without going through an entirely different middleware.

Copy link
Contributor

@MJacred MJacred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest should be fine, I'd say. Some processing order was changed, e.g. in _notification, but should make no difference (I think).

scene/3d/audio_stream_player_3d.cpp Outdated Show resolved Hide resolved
scene/audio/audio_stream_player_internal.cpp Show resolved Hide resolved
scene/2d/audio_stream_player_2d.cpp Show resolved Hide resolved
scene/3d/audio_stream_player_3d.cpp Show resolved Hide resolved
}
internal.ensure_playback_limit();
Copy link
Contributor

@MJacred MJacred Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The playback limit is applied each frame, unlike in AudioStreamPlayer, which does it only on play(). Maybe other players could do that too?

Sounds and looks reasonable to me. Then ensure_playback_limit can be removed altogether and its content moved into play_basic

} else {
stop();
}
internal.set_playing(p_enable);
}

bool AudioStreamPlayer::_is_active() const {
Copy link
Contributor

@MJacred MJacred Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method code was the same as is_playing(). I unified it to do active.is_set() as other players, idk if it makes difference.

Hm… for AudioStreamPlayer, the meaning changes from "AudioServer has my playback registered and setup, and started playing" to "I started being processed and the AudioServer is about to start setting up my playback".

But no idea if this has any tangible repercussions. _is_active is a Node thing afaik

scene/audio/audio_stream_player_internal.cpp Outdated Show resolved Hide resolved
scene/2d/audio_stream_player_2d.h Show resolved Hide resolved
scene/3d/audio_stream_player_3d.cpp Outdated Show resolved Hide resolved
@KoBeWi KoBeWi force-pushed the shadow_of_the_former_self branch from edfcf81 to cc7703c Compare January 16, 2024 13:59
@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 16, 2024

Rebased and updated. I had to change the AudioStreamPlayerInternal to Object, to handle the newly added parameter_list_changed signal. I also fixed the null issue from #86473.

Copy link
Member

@reduz reduz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I was hopeful it would save more code, but I guess its still a lot less error prone and easier to maintain. Great work!

@akien-mga
Copy link
Member

Failing test:

./tests/core/object/test_class_db.h:833:
TEST SUITE: [ClassDB]
TEST CASE:  [ClassDB] Add exposed classes, builtin types, and global enums
  [ClassDB] Validate exposed classes

./tests/core/object/test_class_db.h:376: ERROR: CHECK_FALSE( (p_arg.name.is_empty() || p_arg.name.begins_with("_unnamed_arg")) ) is NOT correct!
  values: CHECK_FALSE( true )
  logged: Unnamed argument in position 0 of method 'AudioStreamPlayer2D.set_pitch_scale'.

@KoBeWi KoBeWi force-pushed the shadow_of_the_former_self branch from cc7703c to 0c7db3c Compare January 17, 2024 12:12
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave it a quick look, and it looks good!

@akien-mga akien-mga merged commit 8f9c815 into godotengine:master Jan 18, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants