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

Attempt to preserve sample control point bank when encoding beatmap #32098

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Feb 25, 2025

RFC. Opening as draft as I'm not sure whether this is going to be accepted, and what the wider implications of this change may end up being.

This was reported internally in https://discord.com/channels/90072389919997952/1259818301517725707/1343470899357024286. The issue described was that sample specifications on control points in stable disappeared after the beatmap was updated from lazer.

The reason why the sample specifications were getting dropped is that they got lost in the logic that attempts to translate per-hitobject samples that lazer has back into stable "green line" type control points. That process only attempted to preserve volume and custom sample bank, but did not keep the standard bank - likely because it's kind of superfluous information for correct sample playback of the objects, as the samples get encoded again for each object individually. However dropping this information makes for a subpar editing experience.

The choice of which sample to pick the bank from is sort of arbitrary and I'm not sure if there's a correct one to pick. Intuitively picking the normal sample's bank (if there is one) seems most correct.

The reproduction case given was senya - Koakuma Ringo (EUROBEAT Remix).zip (included with permission), which after submission from lazer turned into 2225109 senya - Koakuma Ringo (EUROBEAT Remix).zip.

The changes in question:

[TimingPoints]  						[TimingPoints]
2320,382.165605095541,4,2,0,70,1,0      		     |	2320,382.165605095541,4,1,0,70,1,0
2320,-133.333333333333,4,2,0,70,0,0     		     |	2320,-133.333333333333,4,1,0,70,0,0
14549,-100,4,2,0,70,0,0 				     |	14549,-100,4,1,0,70,0,0
26778,-83.3333333333333,4,2,0,70,0,0    		     |	26778,-83.3333333333333,4,1,0,70,0,0
39007,-71.4285714285714,4,2,0,70,0,0    		     |	39007,-71.4285714285714,4,1,0,70,0,0
64230,-100,4,2,0,70,0,0 				     |	64230,-100,4,1,0,70,0,0
82192,-83.3333333333333,4,2,0,70,0,0    		     |	82192,-83.3333333333333,4,1,0,70,0,0
87925,-76.9230769230769,4,2,0,70,0,1    		     |	87925,-76.9230769230769,4,1,0,70,0,1
112383,-250,4,2,0,70,0,0				     |	112383,-250,4,1,0,70,0,0
124612,-71.4285714285714,4,2,0,70,0,0   		     |	124612,-71.4285714285714,4,1,0,70,0,0
149835,-100,4,2,0,70,0,0				     |	149835,-100,4,1,0,70,0,0
167415,-83.3333333333333,4,2,0,70,0,0   		     |	167415,-83.3333333333333,4,1,0,70,0,0
173530,-76.9230769230769,4,2,0,70,0,1   		     |	173530,-76.9230769230769,4,1,0,70,0,1
197988,-250,4,2,0,70,0,0				     |	197988,-250,4,1,0,70,0,0
210409,-133.333333333333,4,2,0,70,0,0   		     |	210409,-133.333333333333,4,1,0,70,0,0
222447,-90.9090909090909,4,2,0,70,0,0   		     |	222447,-90.9090909090909,4,1,0,70,0,0
246905,-50,4,2,0,70,0,1 				     |	246905,-50,4,1,0,70,0,1
247097,-55.5555555555556,4,2,0,70,0,1   		     |	247097,-55.5555555555556,4,1,0,70,0,1
249007,-50,4,2,0,70,0,1 				     |	249007,-50,4,1,0,70,0,1
257797,-50,4,2,0,70,0,1 				     |	269644,-50,4,1,0,70,0,0
269644,-50,4,2,0,70,0,0 				     |	270982,-83.3333333333333,4,1,0,70,0,1
270982,-83.3333333333333,4,2,0,70,0,1   		     |	271364,-71.4285714285714,4,1,0,70,0,1
271364,-71.4285714285714,4,2,0,70,0,1   		     |	295727,-71.4285714285714,4,1,0,70,0,0
295727,-71.4285714285714,4,2,0,70,0,0   		     |	295823,-71.4285714285714,4,1,0,70,0,1
295823,-71.4285714285714,4,2,0,70,0,1   		     |	318753,-100,4,1,0,70,0,0
318753,-100,4,2,0,70,0,0				     <
							     <

I can probably make a test case for this but it's a bit annoying. None of our existing conversion stability can really cover this because they either test that consecutive exports from lazer are stable, or if they test just normal stability, they don't exercise sample control points for obvious reasons.

I'm somewhat certain this shouldn't break stuff because the per-object samples should take precedence over control point specs, but I'm not super sure how to verify that in any robust way.

This was reported internally in
https://discord.com/channels/90072389919997952/1259818301517725707/1343470899357024286.
The issue described was that sample specifications on control points in
stable disappeared after the beatmap was updated from lazer.

The reason why the sample specifications were getting dropped is that
they got lost in the logic that attempts to translate per-hitobject
samples that lazer has back into stable "green line" type control
points. That process only attempted to preserve volume and custom sample
bank, but did not keep the standard bank - likely because it's kind of
superfluous information *for correct sample playback of the objects*, as
the samples get encoded again for each object individually. However
dropping this information makes for a subpar editing experience.

The choice of which sample to pick the bank from is sort of arbitrary
and I'm not sure if there's a correct one to pick. Intuitively picking
the normal sample's bank (if there is one) seems most correct.
@bdach bdach added the area:beatmap-parsing .osu file format parsing label Feb 25, 2025
@bdach bdach requested review from peppy and smoogipoo February 25, 2025 14:47
@bdach bdach self-assigned this Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:beatmap-parsing .osu file format parsing size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant