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

Allow choosing different background/audio files for individual difficulties #30860

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Nov 24, 2024

RFC. I was going to start by following what the title of the issue thread states, which is to just not delete files that are used by other difficulties. However, the underlying discussion thread asks for more than that, and such a change does not really make much sense if the general code still assumes all beatmaps should use one background/audio file, so I took it one step further and changed the whole logic to properly allow difficulties to have their own background/audio.

Preview:

CleanShot.2024-11-27.at.05.36.41-converted.mp4

@frenzibyte frenzibyte requested a review from a team November 24, 2024 06:14
@Phippe
Copy link

Phippe commented Nov 25, 2024

I would've assumed that there would be something like a default image for all difficulties. Then each difficulty would have the option to overwrite the default image, but also to change back to it.

This would make the case easier where there are a large number of difficulties, but only a few of them have custom backgrounds. If you wanted to change the background for all the difficulties that share the same default background, you would have to do so for each difficulty separately.

Just my two cents though, I'm not a mapper.

@bdach
Copy link
Collaborator

bdach commented Nov 25, 2024

I would've assumed that there would be something like a default image for all difficulties. Then each difficulty would have the option to overwrite the default image, but also to change back to it.

I would also assume so yes.

This looks half-baked to me. There should be a way to replace the background on all difficulties at once, and it should be the default. Replacing on a single difficulty should be the exception. And maybe there should be a dropdown which allows picking from existing backgrounds from all the difficulties of the set.

@peppy
Copy link
Member

peppy commented Nov 25, 2024

Hmm, I dunno. I thought this PR is providing a pretty good UX until we actually have a concept of set level metadata. It meets existing users' expectations.

Adding a "this beatmap overrides the default" is going to be a pain to implement a good UX for. But I guess I'm all ears?

This would make the case easier where there are a large number of difficulties, but only a few of them have custom backgrounds. If you wanted to change the background for all the difficulties that share the same default background, you would have to do so for each difficulty separately.

This seems very edge case.

@bdach
Copy link
Collaborator

bdach commented Nov 25, 2024

Easiest would be to ask user on file selection whether to use it on all difficulties or just the one. Probably fade out the file picker and have 2 buttons in the same popover to choose.

@peppy
Copy link
Member

peppy commented Nov 25, 2024

Kinda the same thing really, just more explicit.

But it would solve my remaining issue with this PR, which is that it is applying the action to both background and audio at the same time (should really be per resource).

@frenzibyte
Copy link
Member Author

But it would solve my remaining issue with this PR, which is that it is applying the action to both background and audio at the same time (should really be per resource).

It is technically doing it per resource if you check the code. Is the frontend not conveying that well?

I would've assumed that there would be something like a default image for all difficulties. Then each difficulty would have the option to overwrite the default image, but also to change back to it.

I would also assume so yes.

This looks half-baked to me. There should be a way to replace the background on all difficulties at once, and it should be the default. Replacing on a single difficulty should be the exception. And maybe there should be a dropdown which allows picking from existing backgrounds from all the difficulties of the set.

The way it was implemented in this PR followed the natural storage of metadata in beatmaps, choosing a background/audio updates the metadata of the current difficulty, and the button changes all other difficulties to update as such.

I've also thought about a dialog after opening the PR but I wasn't sure if it's worth the effort. Now that it's brought up, and the button is already misleading in the idea of "does it update both background and audio or is it per-resource", so I will update the PR as such.

@peppy
Copy link
Member

peppy commented Nov 26, 2024

@frenzibyte to confirm we're all on the same page, I think spaceman is proposing that in the existing file chooser popup, after selecting a file, the file chooser content is replaced with two buttons (in the same popup) which let the mapper select [apply to this difficulty] [apply to all difficulties]. Should only show this if there's more than one difficulty already.

@frenzibyte
Copy link
Member Author

frenzibyte commented Nov 27, 2024

OP updated with preview of the new behaviour. This is ready for review.

Shaved off lots of copypasta so the test actually shows what it's testing.
@peppy peppy self-requested a review November 28, 2024 05:59
@peppy peppy force-pushed the editor-multiple-background-audio-files branch from d85585f to 4d9d5ad Compare November 28, 2024 06:14
{
var popover = new BeatmapFileChooserPopover(handledExtensions, current, chooserPath, beatmapHasMultipleDifficulties);

popover.ApplyToAllDifficulties.ValueChanged += v =>
Copy link
Member

Choose a reason for hiding this comment

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

What is this flow? Why isn't this a BindTo or something more standard?

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 idea was that the popover's bindable is nullable so that whatever the user chooses will always trigger a change in the bindable, but I don't want the general bindable to be nullable as well so there's a value changed event here.

Anyways I can understand why it looks odd, I've rewritten it in 4a1401a

Comment on lines 153 to 154
if (c.NewValue == null)
return;
Copy link
Member

Choose a reason for hiding this comment

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

How can this happen?

@frenzibyte frenzibyte force-pushed the editor-multiple-background-audio-files branch from a9197fa to 489d7a3 Compare November 28, 2024 23:37
@frenzibyte
Copy link
Member Author

frenzibyte commented Nov 28, 2024

In addition to the above comments, I've also looked back to how I was ensuring changes to metadata of other difficulties is being saved and I realised my logic was unnecessarily complicated.

Instead, I've updated the metadata of the other difficulties from beatmap.Value.BeatmapSetInfo (which is the detached beatmap set of the current difficulty), and I've forced a Save call after changing each resource for two reasons:

  1. EditorChangeHandler cannot be aware of changes to metadata of other difficulties, therefore if the user changes resource on a specific difficulty, then switches to another difficulty without saving, the user will not be prompted to save, and when they change the resource on the other difficulty, the state of the previous difficulty will be stale.
  2. If the user presses "Forget all changes" on a save dialog after changing a resource, then the difficulty/beatmap will become corrupted if the metadata was pointing to an old file that has been removed by the change logic.

Changing resources should be a rare operation anyway, so I don't think triggering a save will make anybody sad.

{
Caption = GameplaySettingsStrings.BackgroundHeader,
PlaceholderText = EditorSetupStrings.ClickToSelectBackground,
},
audioTrackChooser = new FormFileSelector(".mp3", ".ogg")
audioTrackChooser = new FormBeatmapFileSelector(beatmapHasMultipleDifficulties, ".mp3", ".ogg")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know that we should be encouraging usage of different tracks in a single beatmap set? Yes I know it happens in practice, but I don't know that we should be exposing that in the client.

Copy link
Member

Choose a reason for hiding this comment

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

I'm still of the thought that we should leave such things up to the ranking criteria. I'd rather have a defined flow for this, because if we don't, users will end up documenting weird and potentially incorrect ways of doing it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well in that case the same thing that applies in #30860 (comment) applies here, because the audio file is also encoded to .osu.

Comment on lines 110 to 121
if (applyToAllDifficulties)
{
newFilename = $"{baseFilename}{source.Extension}";

using (var stream = source.OpenRead())
foreach (var beatmap in set.Beatmaps)
{
if (set.GetFile(readFilename(beatmap.Metadata)) is RealmNamedFileUsage otherExistingFile)
beatmaps.DeleteFile(set, otherExistingFile);

writeFilename(beatmap.Metadata, newFilename);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic is missing a completely key step.

Recall that a beatmap's background filename is contained in the .osu file for the beatmap. None of this actually mutates the other .osu files. Which means that this breaks immediately on export and re-import; only the difficulty that was active at the point of replacing all backgrounds will have the backgrounds, and the other difficulties will point to dead assets that are no longer there because they were deleted at the point of replace. (I have tested this.)

In other words this logic needs to re-encode every single beatmap in the set to work correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ouch, good catch. Since I'm already triggering a save at the end of the change method I might as well perform a BeatmapManager.Save() of each difficulty in here to ensure it's re-encoded.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be addressed in 8e0f6fc, I don't like the part where I'm re-fetching each difficulty's skin in order to preserve it but can't see any other way around that.

@peppy peppy self-requested a review December 10, 2024 05:19
@peppy peppy force-pushed the editor-multiple-background-audio-files branch from c68d4af to bbaa542 Compare December 10, 2024 07:45
@peppy
Copy link
Member

peppy commented Dec 10, 2024

I rewrote the changeResource method to be better to my eye. @bdach would appreciate a one-over (and also @frenzibyte to check it's not worse than before).

The main goal here was that I wanted to avoid doing the per-difficulty save in the case where the filename didn't change. In testing, it can take a few seconds to run this as it triggers a full difficulty calculation via

public ProcessBeatmapDelegate? ProcessBeatmap { private get; set; }

I'm probably okay with this change now. We can revisit making the save more optimal in the future.

Copy link
Member Author

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

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

The main goal here was that I wanted to avoid doing the per-difficulty save in the case where the filename didn't change.

That should've been already correctly handled in my previous code, I've always checked if the filename of the other diff is different first before writing to it and triggering a save. If anything, your code is just emphasizing that part, which is good.

I'm not 100% sure how to feel about there being two small applyToAllDifficulties if conditionals with some shared code placed between them. It feels slightly awkward to me when reading the method from top to bottom but I can understand why it's done this way (to make the method look more like it's working step-by-step but each step behaves differently based on the condition). I can live with it.

@peppy
Copy link
Member

peppy commented Dec 10, 2024

That should've been already correctly handled in my previous

Check the older commit, it wasn't. I know because i debugged it and that's why I refactored everything.

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.

Adding a new background/audio file in the editor may remove file which is used in other difficulties
4 participants