Hiviexd / MVTaikoChecks

A set of osu!taiko Mapset Verifier checks
3 stars 2 forks source link

Remove break length BPM scaling from `RestMomentCheck` #12

Closed hongaaronc closed 1 year ago

hongaaronc commented 1 year ago

So going through the code for RestMomentCheck again, I realized this is how we are handling BPM scaling for the check:

  1. Scale the minimum gap for what's an acceptable rest moment (lower BPM => more forgiving)
  2. Scale the amount of continuous mapping w/o break (lower BPM => less forgiving)

I think # 1 makes sense, but do we actually want to be doing # 2?

I feel like often when I'm manually checking for rest moments, I think not scaling the amount of continuous mapping, but only scaling the acceptable break gap, makes sense since song structures still follow conventions like 1 bar = 4 measures, which is 16/1. This seems to be what the RC is built around. I think this is more intuitive so that break structure lines up with song structure.

So I think we should just change this part to use non-normalized msPerBeat, but wanted to confirm this makes sense.

Hiviexd commented 1 year ago

from my 3 years of modding i can confidently say that option 1 is the much more common approach (i.e at 125BPM, 1/1 would be a sufficient break for muzus for example), i rarely see option 2 get applied at all

our current approach is good and aligns with what people expect

hongaaronc commented 1 year ago

wait but right now we're doing both..

so like:

  1. 125bpm, 1/1 is a sufficent break (this is good) but also...
  2. We're saying we need breaks every 32/3 (about 11/1) of continuous mapping at 125bpm (seems questionable)

so we dont want 2 right?

Hiviexd commented 1 year ago

correct, doesn't make sense to apply 2, especially when we're already applying 1

hongaaronc commented 1 year ago

Alright thanks, will update the check then