Add ability to toggle mods during replay #19925
Conversation
a poor workaround but I don't know how to solve it better
bdach
changed the title
osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs
Outdated
Show resolved
Hide resolved
my bad
| /// <summary> | ||
| /// A property to set whether the mod has been disabled. | ||
| /// </summary> | ||
| bool IsDisable | ||
| { | ||
| get; | ||
| } |
There was a problem hiding this comment.
Without digging too deep into this, this doesn't make sense. A get property to set?
| /// <summary> | ||
| /// A property to set whether the mod has been disabled. | ||
| /// </summary> | ||
| bool IsDisable |
| /// <summary> | ||
| /// Method called when mod toggle. | ||
| /// </summary> | ||
| void OnToggle(); |
There was a problem hiding this comment.
You may just want one single BindableBool in this interface, which can handle both value and event flow.
There was a problem hiding this comment.
Sure, but I'm a little confused where the event is registered
There are some mods that don't have a constructor
Define an event registration method in an interface?
There was a problem hiding this comment.
What "event"? You mean the BindValueChanged callback of the proposed bindable in question?
Mods that don't currently have a constructor can receive one. I'm not sure what the problem is here.
There was a problem hiding this comment.
I have decided to use a method to register events and this method will be called once on load
| // Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence. | ||
| // See the LICENCE file in the repository root for full licence text. | ||
|
|
||
| #nullable disable |
There was a problem hiding this comment.
Please no nullable disable on new classes
bring in `DisableToggleEvent` to register event handle for `BindableBool IsDisabled`
| /// <summary> | ||
| /// Display the specified mod at a fixed size. | ||
| /// </summary> | ||
| public class ClickableModIcon : ClickableContainer, IHasTooltip |
There was a problem hiding this comment.
Why is this just ModIcon but copy-pasted in its entirety? This is not okay. There are better methods of code re-use.
| /// <summary> | ||
| /// Displays a single-line horizontal auto-sized flow of mods. For cases where wrapping is required, use <see cref="ModFlowDisplay"/> instead. | ||
| /// </summary> | ||
| public class ClickableModDisplay : CompositeDrawable, IHasCurrentValue<IReadOnlyList<Mod>> |
There was a problem hiding this comment.
Same as above for this. This cannot/should not exist in its current form.
| @@ -371,6 +371,9 @@ private void applyRulesetMods(IReadOnlyList<Mod> mods, OsuConfigManager config) | |||
|
|
|||
| foreach (var mod in mods.OfType<IReadFromConfig>()) | |||
| mod.ReadFromConfig(config); | |||
|
|
|||
| foreach (var mod in mods.OfType<ICanBeToggledDuringReplay>()) | |||
| mod.DisableToggleEvent(); | |||
There was a problem hiding this comment.
Why is this named DisableToggleEvent()...? It reads completely wrong, like you are disabling the mod's effects or something.
Arguably method shouldn't even exist, as I said already. You can add ctors where you need them.
| @@ -87,13 +94,15 @@ public class DrawableOsuBlinds : Container | |||
| /// </summary> | |||
| private const float leniency = 0.1f; | |||
|
|
|||
| public DrawableOsuBlinds(CompositeDrawable restrictTo, Beatmap<OsuHitObject> beatmap) | |||
| public DrawableOsuBlinds(CompositeDrawable restrictTo, Beatmap<OsuHitObject> beatmap, BindableBool disabledBind) | |||
There was a problem hiding this comment.
Passing bindables like this is generally unsafe and shouldn't be done. Create a local bindable in DrawableOsuBlinds and bind to the mod's instance instead.
ppy#19925 (comment) Assuming changing the mod state in multiplayer, ModIcon will also update its color
frenzibyte
changed the title
Keep `NightcoreBeatContainer` as the same as stable do.
this problem cause disable freeze frame ignore beatmap AR
not sure if it will affect performance
It may should cause the replay to play in an unexpected way, example hit the circle before it arrive at location Maybe this mod is not suitable for turning it off.
close #8357
create a new interface named
ICanBeToggledDuringReplaySome annotations are missing, probably
add a test for mod can only toggle in replay
Maybe should handleHandle at ClickableModIcon.csReplayLoadedinHudOverlaymod list that I think can be implemented
[ ] HR(Unable to implement, need to reloadPlayer, also replay may broken)ModWithVisibilityAdjustment)[ ] ManiaModConstantSpeed(Unable to implement, need to reloadDrawableScrollingRuleset)