-
Notifications
You must be signed in to change notification settings - Fork 82
[GEN][ZH] Unify code of GameAudio and MilesAudioManager #782
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
Conversation
f32af8c
to
eca7e23
Compare
Generals/Code/GameEngineDevice/Include/MilesAudioDevice/MilesAudioManager.h
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngineDevice/Include/MilesAudioDevice/MilesAudioManager.h
Outdated
Show resolved
Hide resolved
eca7e23
to
1e5bfa4
Compare
Updated and addressed the discussed points above, will push a small followup PR to remove the stragling bits of code associated with the removed interface functions from GameAudio. |
fa2370f
to
bf0a507
Compare
Had to make a small push again to include the removal of calls to There are so few instances where these functions are called that it might as well be a part of this PR. |
Is this change meant to be merged with rebase or not? Commit number 3 of this change appears to error:
|
The build of this PR should be fixed with the last commit i made to add removing those calls to since MilesAudioManager inherits from GameAudio, those two commits are kind of interdependent, so i thought it would be better to squash and merge this. But merging by rebase should be okay, i tweaked the PR description above to cover this. |
If commit number 3 fails to compile on its own, then it cannot be merged with rebase. |
bf0a507
to
0dbc5dc
Compare
Reordered the commits to break the problematic dependency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -152,10 +152,6 @@ class AudioManager : public SubsystemInterface | |||
virtual void resumeAudio( AudioAffect which ) = 0; | |||
virtual void pauseAmbient( Bool shouldPause ) = 0; | |||
|
|||
// device dependent stops. | |||
virtual void stopAllAmbientsBy( Object* obj ) = 0; | |||
virtual void stopAllAmbientsBy( Drawable* draw ) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are now 3 commits that remove pieces of stopAllAmbientsBy
in different places. How about put the removal for stopAllAmbientsBy
in one commit? I suggest create one new pull request where this stopAllAmbientsBy
stuff is removed first, then we see next steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seperated that out and into #793
0dbc5dc
to
5164284
Compare
This will may at the moment till #793 is merged before it. |
Can we merge this with Squash? |
Yeah should be fine, can encapsulate the changes in one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Adjusted the title of this change.
5164284
to
f027046
Compare
Rebased with main so good to go. |
Squash and merge
This PR makes the changes required to synchronize the audio implementations between Generals and Zero hour.
Most changes are backports to Generals, while some small tweaks are made to zero hour to match generals.