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

exclude audio from shuffle #2411

Merged
merged 32 commits into from
Feb 16, 2023

Conversation

briaguya-ai
Copy link
Contributor

@briaguya-ai briaguya-ai commented Jan 27, 2023

image

todo:

  • optimize (need the menu for excluding to not be slow even with hundreds of sequences loaded)
  • prevent hangs (it's currently possible to exclude everything and turn the randomize button into a "start an infinite loop of looking for a valid sequence when there are none" button)
  • figure out what happens when loading a new otr of sequences after already having an exclude list (this needs to be handled properly before this can be considered merge ready, losing an exclude list after updating to get a couple new sequences from DJ would be terrible UX)
  • add preview buttons to entries on exclude tab (probably worth moving the exiting preview button functionality out into a method that can be used in both places)
  • add "chip" style type (sfx/bgm etc.) tags for each entry in the list
  • add filters for type (sfx/bgm etc.)
  • use cvar-per-sequence/sfx for exclude list, save every time we update (this should prevent the laggy "make a giant string of excluded sequences" thing, which was the reason for not doing it every time in the first place. i think we use buffered file output so it shouldn't be an issue on PC (might be laggy on console))
  • pre-review cleanup
  • bulk include/exclude (ideally filter and then include/exclue all visible)
  • get it reviewed/address feedback

future things (make issues for these when this gets merged):

  • add presets that load from json so people can share their soundtracks (will check box once issue is made to do this)

Build Artifacts

@briaguya-ai briaguya-ai added the do not merge Not ready or not valid changes label Jan 27, 2023
@briaguya-ai briaguya-ai changed the title [WIP] exclude sequences from shuffle exclude sequences from shuffle Jan 28, 2023
@briaguya-ai briaguya-ai removed the do not merge Not ready or not valid changes label Jan 28, 2023
@aMannus
Copy link
Contributor

aMannus commented Jan 30, 2023

Is it possible to put the exclusions under their categories a-la excluded locations? Currently it's a bit hard to make out if something is a sound effect or a music track for example.

So have all SEQ_BGM_WORLD entries under a dropdown like:
image

etc etc.

@aMannus
Copy link
Contributor

aMannus commented Jan 30, 2023

You also named the tab "Exclude Sequences From Shuffle" but in reality, you can also exclude sound effects in the tab, so the name isn't very accurate.

Maybe "Exclude SFX when randomizing" is more accurate (albeit a bit long)?

@aMannus
Copy link
Contributor

aMannus commented Jan 30, 2023

Maybe out of scope of this PR, but a button to remove all the exclusions at once could also be useful (and excluded locations in the Randomizer menu could also use this if we had it). This really is more of a suggestion though.

@briaguya-ai briaguya-ai added the do not merge Not ready or not valid changes label Feb 6, 2023
@briaguya-ai
Copy link
Contributor Author

briaguya-ai commented Feb 14, 2023

@aMannus this is ready for another look

Maybe "Exclude SFX when randomizing" is more accurate (albeit a bit long)?

I don't like that because i think of SFX as sound effects, and separate from music. I ended up renaming the whole window the "Audio Editor" and changed the name of the tab from "Exclude Sequences From Shuffle" to "Audio Shuffle Pool Management"

it's wordy and i'm not the biggest fan of the name but it at least feels accurate

a button to remove all the exclusions at once could also be useful

i added that as a "future things" checkbox. i agree it'd be great but i also figure it can wait until after this is merged 2469dbc

@briaguya-ai briaguya-ai removed the do not merge Not ready or not valid changes label Feb 14, 2023
@briaguya-ai briaguya-ai changed the title exclude sequences from shuffle exclude audio from shuffle Feb 14, 2023
Copy link
Contributor

@dcvz dcvz left a comment

Choose a reason for hiding this comment

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

Overall changes look good! Definitely puts us in a place where we can better optimise audio editing down the line 👯

Left small comments here and there on things we could improve.

soh/soh/Enhancements/audio/AudioCollection.cpp Outdated Show resolved Hide resolved
soh/soh/Enhancements/audio/AudioCollection.cpp Outdated Show resolved Hide resolved
soh/soh/Enhancements/audio/AudioCollection.cpp Outdated Show resolved Hide resolved
soh/soh/Enhancements/audio/AudioCollection.cpp Outdated Show resolved Hide resolved
soh/soh/Enhancements/audio/AudioCollection.cpp Outdated Show resolved Hide resolved
soh/soh/Enhancements/audio/AudioCollection.cpp Outdated Show resolved Hide resolved
soh/soh/Enhancements/audio/AudioEditor.cpp Outdated Show resolved Hide resolved
soh/soh/Enhancements/audio/AudioEditor.cpp Outdated Show resolved Hide resolved
soh/soh/Enhancements/audio/AudioEditor.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@dcvz dcvz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@briaguya-ai briaguya-ai merged commit 1e6ec1b into HarbourMasters:develop Feb 16, 2023
@briaguya-ai briaguya-ai deleted the exclude-sequences branch March 1, 2023 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants