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

Audio Editor Lock Button and Cleanup #3140

Merged
merged 7 commits into from
Sep 10, 2023

Conversation

Malkierian
Copy link
Contributor

@Malkierian Malkierian commented Aug 22, 2023

Adds a lock button to the audio editor to prevent "Randomize All" buttons from changing specific sounds.

Cleaned up the UI a bit with FontAwesome buttons instead of text buttons, and added tooltips for clarification.

Set all resets (individual reset buttons and "Reset All") to clear CVars instead of setting the value to default (settings .json cleanup).

https://youtu.be/_tRr-0-qeyI

Build Artifacts

Malkierian and others added 3 commits August 21, 2023 12:01
…, individual shuffle buttons bypass and the lock and unlock the sound.

Added tooltips to all the FA buttons.

Shrunk right side of Audio Editor window to match width of new buttons.

Unified all references to the randomized value and lock to two functions that automatically applied prefix and suffix.

Changed "Reset All" to clear the cvars instead of changing them to default.
@Malkierian
Copy link
Contributor Author

The one problem I couldn't figure out a solution for was the fact that using the "Reset All" or "Randomize All" buttons still restarted the currently playing background music, even if you had that locked, as well as the (I believe) already existing issue of an individual Randomize button on the active scene's BGM not changing the BGM currently playing.

Copy link
Contributor

@garrettjoecox garrettjoecox left a comment

Choose a reason for hiding this comment

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

using the "Reset All" or "Randomize All" buttons still restarted the currently playing background music, even if you had that locked,

This seems very minor, and unsurprising, fine for now

as well as the (I believe) already existing issue of an individual Randomize button on the active scene's BGM not changing the BGM currently playing.

This one sounds a bit worse, and undesirable. Could you confirm this is an existing issue? Approving with the assumption that it is, and wasn’t introduced with these changes.

@Malkierian
Copy link
Contributor Author

Yeah, just confirmed with a release build. The reason is because UpdateCurrentBGM doesn't even have anything to change the current one to a new one, only to reset if the new seqId was the same as the old one XD. I know how to fix it, should I put it in here?

@garrettjoecox
Copy link
Contributor

Yeah, just confirmed with a release build. The reason is because UpdateCurrentBGM doesn't even have anything to change the current one to a new one, only to reset if the new seqId was the same as the old one XD. I know how to fix it, should I put it in here?

if doing it separate will cause conflicts just throw it in this one

@Malkierian
Copy link
Contributor Author

Well, nevermind. Not only was the problem with the randomize button an extremely simple fix (simpler than I originally thought), it was actually pretty easy to put in the check for currently playing BGM having not changed during a Reset All or Randomize All to block the restart of the sequence. So everything should be good to go now.

@aMannus aMannus added the blocking This PR contains code that will be needed for another feature or bugfix to be PR'd later label Aug 26, 2023
@garrettjoecox garrettjoecox merged commit 3d0075e into HarbourMasters:develop Sep 10, 2023
8 checks passed
@Malkierian Malkierian deleted the audio-editor-lock branch September 10, 2023 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking This PR contains code that will be needed for another feature or bugfix to be PR'd later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants