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

AudioCommon: Move sound stream variables to Core::System. #11026

Merged
merged 1 commit into from Sep 2, 2022

Conversation

AdmiralCurtiss
Copy link
Contributor

Global state elimination, part 1 of too many.

I'm not 100% sure if this is the correct approach? It does require later refactoring out all the Core::System::GetInstance() calls into passing the class around, but that seems easier than just going directly there from the current state.

@iwubcode
Copy link
Contributor

iwubcode commented Sep 1, 2022

What you have LGTM.

I might be completely off on this, as I haven't looked at the audio stuff or the general Dolphin initializaton/deinitalization flow. But I figured now is the time to bring this up.

A possible alternative would be to introduce a new SoundSystem class. This class wraps most (all?) those free-form functions in AudioCommon. Instead of InitSoundStream you'd just have the constructor. Instead of ShutdownSoundStream you would just have the destructor. So some nice RAII. Then instead of the System having to know about multiple pieces of state for sound (and therefore sound stuff having to call back into System), it would just have this SoundSystem class as a member. A SoundSystem would own a SoundStream and have details on whether the audio dump was running, etc. Other subsystems like Inputs, Graphics, etc would also have a system class and all these would belong under System. You could possibly even take it a step further by having the owner System be passed to the 'children' so that if they needed to access other subsystems they could.

Apologies if there's some flaws in my recommendation. Regardless, I think what you have is great. You're probably bringing a tear to @lioncash 's eye :)

@lioncash
Copy link
Member

lioncash commented Sep 1, 2022

Oh nice! I completely forgot to continue work on deglobalizing everything after introducing the scaffolding for it.

Refactoring from GetInstance() usages to passing in the class instance directly is how we migrated all the global core state out in yuzu. Now the frontends control the lifecycle of the system instance rather than it sort of... just existing haphazardly all over the place. So I think that approach is totally fine

@AdmiralCurtiss
Copy link
Contributor Author

@iwubcode Yes, that would definitely be nicer... but it's also significantly more refactoring work. What I did here was as straightforward as possible to minimize any possibility of breaking anything. Definitely something to keep in mind for the future though!

@lioncash Alright, cool. In that case I think this is good to go then, and I'll try to move more stuff in the same way in separate PRs.

@lioncash
Copy link
Member

lioncash commented Sep 2, 2022

Sounds good!

@lioncash lioncash merged commit 23902f9 into dolphin-emu:master Sep 2, 2022
11 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the sound-stream branch September 2, 2022 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants