Add progressive coverage for mania's Hidden and FadeIn mods #27068
Conversation
smoogipoo
changed the title
|
There was a PR for this already: #22591 Am I to understand that this one is to supersede it? |
|
I didn't see that PR... I think this PR should be treated independently and supersede if possible. I think min/max coverage can probably be left to a later date. |
| { | ||
| } | ||
|
|
||
| public ScoreRank AdjustRank(ScoreRank rank, double accuracy) | ||
| public virtual ScoreRank AdjustRank(ScoreRank rank, double accuracy) |
There was a problem hiding this comment.
The distinction between what should and shouldn't be virtual with mods isn't yet clear to me, especially those in the base osu.Game project.
There was a problem hiding this comment.
What I was getting at is that it does not appear to be required for this diff at present. So wondering if there was anything in particular that this was angling at.
|
Was this visually tested against stable or was this written just by plugging the stable numbers in? Because as far as I can see, visually, stable seems to hide more of the playfield even if the numbers match up.
Red line marks where the cutoff is. Also in stable the cover goes away during break and is faded back in on break end: https://github.com/peppy/osu-stable-reference/blob/013c3010a9d495e3471a9c59518de17006f9ad89/osu!/GameModes/Play/Rulesets/Mania/Stage/StageMania_Calculations.cs#L286-L316. I'm not seeing that happen in this implementation, so not sure if that is an executive decision to not bother, or an omission. |
|
I mostly transferred things across and eyeballed it. The issue is that the cover is applied from the hit position, so for example to get a cover that's covers everything but 10px you have to do something like Here are my results after the changes (left =
|
| if (DrawHeight == 0) | ||
| return base.GetHeight(coverage); | ||
|
|
||
| return base.GetHeight(coverage) * reference_playfield_height / DrawHeight; |
There was a problem hiding this comment.
At "normal" resolutions this is fine, but I'm not sure using DrawHeight here is the best idea because it appears to break down on very narrow screens, giving them unfair advantage. See the following screenshots:
 |
 |
This is the same instant on the same map, same scroll speed, just different aspect ratios. For better comparison here's them next to each other, lined up to the hit position:
The time range is the same, all objects line up to each other, but the effective coverage on the narrow window is considerably less.
|
In addition to the above:
I don't see a response to this anywhere.
What does this mean? In my testing the old setting value appears to be effectively ignored? Is that what's deemed "working" there? |
Yeah sorry, that's what I meant. The concern was for if it crashes/throws some sort of exception. |
|
We probably want to get this in the next release. Seems like some feedback still needs to be applied? |
|
Yeah I'll try to find a solution for the feedback. |
|
All should be fixed now... |
bdach
added
the
next release
|
Aside from one UI nit (4b21970), I think this is good enough to go with now. I'm gonna admit that I mostly did visual comparisons but the coverage seems to check out. The fade in after break is a bit different: Untitled.mp4but I don't think that's significant enough to run this through the wringer any longer (if anything lazer is harsher which seems fine to do). @peppy not sure if you wanted to give this one a once-over? |
Resolves #22355
Supersedes / closes #22591
This restores progressive coverage that HD and FI had in osu!stable, and adds a new mod
ManiaModCover/COwhich allows setting a custom cover.I've tested that importing an old score that previously had
Coverageset to a custom value works.Not sure whether we want/need to do a score reset at this point.