BPM display in mods overlay #19929
BPM display in mods overlay #19929
Conversation
|
Dunno about this from a design standpoint. I was hoping a design for this would arrive from @arflyte but no luck I guess. Also note that in this form, this display is potentially taking space away from a search box that I was going to add. I'll review code tomorrow. |
|
Mentioned in OP, but this is kinda useless as it stands: Uploading osu! 2022-08-24 at 05.27.25.mp4… @arflyte ? |
osu.Game/Overlays/Mods/BpmDisplay.cs
Outdated
| { | ||
| // when no map is selected, common length will be zero, producing infinity. | ||
| mostCommonBPM = 0d; | ||
| this.FadeOut(transition_duration, Easing.OutQuint); |
There was a problem hiding this comment.
I disagree with fading out the whole thing in such cases. Show a - instead.
Mods names are not so long. I think half of the screen will be enough for it. If we short the labels to just "BPM" and "Score", there will be space to add even more such displays.
My suggestion here is to move container with this displays to a higher level, forex. in |
| Anchor = Anchor.Centre, | ||
| Origin = Anchor.Centre | ||
| }, | ||
| new BpmDisplay(), |
There was a problem hiding this comment.
If the BPM display is to be steered by ShowTotalMultiplier then that property needs a rename. Maybe something like ShowModEffects or something, with associated proper xmldoc to explain what the "effects" are.
osu.Game/Overlays/Mods/BpmDisplay.cs
Outdated
| Height = DifficultyMultiplierDisplay.HEIGHT; | ||
| AutoSizeAxes = Axes.X; | ||
|
|
||
| InternalChild = new InputBlockingContainer |
There was a problem hiding this comment.
Having mixed feelings about all of this layout setup being essentially copy-pasted from DifficultyMultiplierDisplay. Is this really necessary?
osu.Game/Overlays/Mods/BpmDisplay.cs
Outdated
| double rate = 1; | ||
| foreach (var mod in mods.Value.OfType<IApplicableToRate>()) | ||
| rate = mod.ApplyToRate(0, rate); | ||
|
|
||
| double mostCommonBPM = Math.Round(Math.Round(60000 / beatmap.GetMostCommonBeatLength()) * rate); |
There was a problem hiding this comment.
Similarly mixed feelings about all this being copy-pasted from BeatmapInfoWedge. I initially thought this was a misguided buggy implementation due to the double rounding, until I blamed back the BeatmapInfoWedge copy and found #18345.
osu.Game/Overlays/Mods/BpmDisplay.cs
Outdated
|
|
||
| double mostCommonBPM = Math.Round(Math.Round(60000 / beatmap.GetMostCommonBeatLength()) * rate); | ||
|
|
||
| if (double.IsNormal(mostCommonBPM)) |
There was a problem hiding this comment.
What is this IsNormal check, even? Is this supposed to catch NaNs? When can this even occur? A comment below is saying something about "no map selected" - maybe the way there is to, you know, not divide by 0 in the first place?
| settingChangeTracker = new ModSettingChangeTracker(m.NewValue); | ||
| settingChangeTracker.SettingChanged += _ => refresh(); | ||
| }, true); | ||
| working.ValueChanged += _ => refresh(); |
There was a problem hiding this comment.
Nitpick, but why is one of these using BindValueChanged(...) and one is doing ValueChanged += ...?
|
Is this intended to be dependent on #19991 now? |
|
After same experiments, IMO the best way is just to duplicate effect displays to 2022-08-29.21-49-40.mp4Will this be accepted?
Yes. |
I'm not a fan of duplicating the displays, personally, from a functional standpoint. The visual look also leaves much to be desired for me (the alignment of the items seems really weird for me). For reference, some of the newer figma designs just get rid of the customisation panel at the bottom in favour of a dropdown, which would potentially solve this. That said we direly need input from @arflyte here whether that is something that's final or just a proposal (third time the charm, maybe?) |
|
I think the current problem with the design is that it's not suitable to display many types of info as it would clutter up everything (it's just bad to have a lot of boxes) So probably, as a long-term solution, I have to design a better information container that can house multiple info, which could also be future-proof. |
|
@Feodor0090 I think it's fine to duplicate the display as you proposed. Please apply this change and we'll take it from ther. |
This comment was marked as resolved.
This comment was marked as resolved.
|
Superseded by #24705. |
This PR adds BPM display next to difficulty multiplier display into mods overlay. This allows users to see final bpm after applying custom dt/nc/ht without closing the overlay.
Issues here:
ShowTotalMultiplieris true, because i share container withDifficultyMultiplierDisplay. I think that it's ok, because DMD is hidden only in freemods selection where this control is useless anyway.Test scene included.