osu
osu copied to clipboard
ppy
Fixed ModSelectOverlay not having info about room required mods
AccountForMultiplayerMods looks like a bad solution but I don't see other way to differentiate ModSelectOverlay during PlaylistItem select from overlay that room members have
Arguably - score multiplier update can be disabled to be considered as "relative to room"
Is this ready for re-review? I'm still waiting for an attempt at appending multiplayer required mods to the SelectedMods bindable instead of adding special code for that.
Is this ready for re-review? I'm still waiting for an attempt at appending multiplayer required mods to the
SelectedModsbindable instead of adding special code for that.
I'm sure that it will break something in the process. Just calculating it 2 times: for selected mods and for required mods seems stable and good enough for me. I moved all "required mods" calculations into separate class, so it shouldn't affect other ModOverlay-s at all. If this approach is unacceptable for you - I will try to make it by merging required and selected mods.
Unless you provide one reason why it breaks or post a diff that shows my proposal to be too complicated, I cannot simply imagine that the solution in this PR is better. I'll take a look myself if I get to it first.
Unless you provide one reason why it breaks or show a diff that it's too complicated, I cannot simply imagine that the solution in this PR is better. I'll take a look myself if I get to it first.
okay, i will try to make it in another way in another branch
I'm still waiting for an attempt at appending multiplayer required mods to the
SelectedModsbindable
I'm hoping this is meant in a "pass the combination of mods to the mod overlay specifically" sense, rather than in a sense of "redo the entire multiplayer screen to sorta-kinda keep track of both required and allowed at the same time in one bindable"... I could live with the former, definitely against the latter.
That said, I wouldn't be against an ad-hoc solution, but it cannot be written as it was initially written here. Inheritance is a thing, it should be used. Flags getting added to a base implementation solely for the sake of a single inheritor is just bad design.
I'm still waiting for an attempt at appending multiplayer required mods to the
SelectedModsbindableI'm hoping this is meant in a "pass the combination of mods to the mod overlay specifically" sense, rather than in a sense of "redo the entire multiplayer screen to sorta-kinda keep track of both required and allowed at the same time in one bindable"... I could live with the former, definitely against the latter.
That said, I wouldn't be against an ad-hoc solution, but it cannot be written as it was initially written here. Inheritance is a thing, it should be used. Flags getting added to a base implementation solely for the sake of a single inheritor is just bad design.
no flags, i'm using inheritance
After discussing in discord, it appears that the approach I proposed in https://github.com/ppy/osu/pull/27214#pullrequestreview-1887046024 is not as easy as it sounds, mainly because we rely on the state of the "selected mods" bindable in match mod select overlay to know what kind of free mods has the user selected in the match. The approach taken by this PR is therefore fine, but I'll go through and refactor it to at least satisfy https://github.com/ppy/osu/pull/27214#discussion_r1494475917.
After discussing in discord, it appears that the approach I proposed in #27214 (review) is not as easy as it sounds, mainly because we rely on the state of the "selected mods" bindable in match mod select overlay to know what kind of free mods has the user selected in the match. The approach taken by this PR is therefore fine, but I'll go through and refactor it to at least satisfy #27214 (comment).
I already fixed it though The only thing left to do is code duplicating, no?
The only concern I have with my version is that there are now two pathways for a side component like RankingInformationDisplay and BeatmapAttributesDisplay to read the mods list. Either it reads or DI's SelectedMods, causing it to ignore room required mods in multiplayer/playlists. Or, it overrides UpdateOverlayInformation to correctly factor in the room required mods. Summoning @bdach for feedback and a second review on this, as this looks good to my eyes for now.