Skip to content

Add way to add/remove custom beatmap samples to setup screen#36190

Merged
peppy merged 4 commits intoppy:masterfrom
bdach:add-custom-samples-via-setup
Jan 16, 2026
Merged

Add way to add/remove custom beatmap samples to setup screen#36190
peppy merged 4 commits intoppy:masterfrom
bdach:add-custom-samples-via-setup

Conversation

@bdach
Copy link
Copy Markdown
Collaborator

@bdach bdach commented Dec 31, 2025

2025-12-31.12-15-10.mov

Closes #35370.

I'm treating this as a RFC tier PR and not something that's necessarily fleshed out because this is treading a bunch of new ground with regards as to what the editor needs to handle and I'd like to see general opinions on where this has headed first.

Of note, the flow in RealmBackedResourceStore that was supposed to ensure that skin changed events fire when the skin's set of files changes was not correctly hooked up for beatmap skins and this PR attempts to hook it up.

Commit messages attempt to provide more insight to changes that you will likely find otherwise strange.

Among others, this features a scary-looking change wherein
`WorkingBeatmap` now exposes a `RealmAccess` via
`IStorageResourceProvider`. This is necessary to make
`RealmBackedResourceStore` actually start firing callbacks when the
edited beatmap's skin is modified by adding new samples to it.
This is invariably a test fix but also a valid change to make in
isolation.

In short, the problem is thus: Now that the beatmap skin is hooked up to
realm to receive notifications about changed files (which is required
for correctly handling custom samples), tests started failing, because
of the following sequence of events

- Saving a brand new `.osu` to a beatmap set causes
  `RealmBackedResourceStore` to fire subscription callbacks because a
  new file was added to the set
- This fires `RealmBackedResourceStore.CacheInvalidated`
- Which fires `EditorBeatmapSkin.BeatmapSkinChanged`
- Which would previously fire `EditorBeatmap.SaveState()` and as such
  mark the beatmap dirty / modified.

In this scenario this is gratuitous. There's no need to be raising save
states here, a new `.osu` was added to the set that is in a consistent
saved state and nothing actually changed in the beatmap.

However it does not appear sane to attempt to circumvent this with
conditional guards or something, because in cases where files are
added/removed from the set, *there isn't really any reason to take save
states anyway*. The change handler only deals with the `.osu`, any
modifications to any of the other files cannot be undone anyway.

Therefore, only keep the state save to the one change to beatmap skin
that *can* actually be sanely undone which is changing combo colours.
@bdach bdach force-pushed the add-custom-samples-via-setup branch from d1f2e39 to affe295 Compare December 31, 2025 12:03
@peppy peppy moved this from Inbox to Pending Review in osu! team task tracker Jan 7, 2026
@peppy peppy self-requested a review January 16, 2026 11:11
Comment on lines +104 to +105
if (beatmapSkin is LegacyBeatmapSkin skin)
BeatmapSkin = new EditorBeatmapSkin(this, skin);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The biggest issue I have with this whole change is this part here (the fact we're not using ISkin and always typing a LegacyBeatmapSkin for beatmaps).

It's not really the fault of this PR and it might even be resolved by a naming change. Just going to turn a blind eye here for now.

@peppy peppy merged commit a6489cd into ppy:master Jan 16, 2026
9 checks passed
@github-project-automation github-project-automation bot moved this from Pending Review to Done in osu! team task tracker Jan 16, 2026
@devkokooo
Copy link
Copy Markdown

Goated feature, been wanting this for a while

bdach added a commit to bdach/osu that referenced this pull request Jan 22, 2026
bdach added a commit to bdach/osu that referenced this pull request Jan 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Fix not being able to assign custom sample bank (numbers) in lazer editor

3 participants