Fix "omit barline" sometimes not applying correctly #22754
Conversation
These changes were taken from ppy#22582. Minor adjustments were applied to match stable expectations, which is to say there cannot be an inherited control point with omit barline specification (in the editor the setting is greyed out when inheritance is turned on).
|
@sw1tchbl4d3r Please take a look at this and see how it looks to you. During RL discussion it was touched on that the previous PR was very complicated to understand and review. I think this approach feels better (and is a better direction in terms of exposing the setting to the user in the editor). |
|
This is definitely the better approach, was worried a sudden change of a property like this might break older lazer-made maps that rely on the property being part of the effect point, but this still needs some work in the editor, in particular this: The full info "badge" gets too long to fit in the section and gets moved downwards, which results in this weird displaying of the property. |
|
I've fixed the layout issue with this one, so it should be good to go. |
There was a problem hiding this comment.
Definitely think this is much better (read: easier to digest) than the alternative PR.
Seeing that IRL discussions were mentioned above, I'm unsure if I'm allowed to just merge this or if I should wait for another opinion. I will say that I am relying on unit tests here and have not done much further testing on the ranked beatmap corpus (if that is something you want me to do, then let me know).
| @@ -230,10 +231,10 @@ void outputControlPointAt(double time, bool isTimingPoint) | |||
| LegacyEffectFlags effectFlags = LegacyEffectFlags.None; | |||
| if (effectPoint.KiaiMode) | |||
| effectFlags |= LegacyEffectFlags.Kiai; | |||
| if (effectPoint.OmitFirstBarLine) | |||
| if (timingPoint.OmitFirstBarLine) | |||
There was a problem hiding this comment.
Of note, this is a subtler-than-apparent change which makes it so that if the problematic beatmap mentioned in the issue is re-exported using lazer's editor, it will auto-fix itself, as in the problematic inherited point will have the omit bar line flag set after export. Which is neat, but also technically breaks import-export stability. But I think that's fine.
|
@bdach if you're confident it's likely fine to go ahead and merge. the RL discussion was mostly very shallow smoogi:"other PR is complex" me:"yeah but i think i have a cleaner alternative let me do that instead". |
This is an alternative to #22582, which increases the scope of change but reduces the complexity.
Taken from that PR:
By moving the specification to the more correct location of
TimingControlPoint, we can resolve this issue without too much extra thought.