-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adjust osu!taiko and osu!mania editors to not visualise velocity changes by default #24550
Conversation
On the fence about this.... one of the goals of the editor was to be a layer on top of gameplay, and this strictly goes against that concept. Can understand it making editing easier, but like, previewing is no longer a seamless thing, and you have no idea how a beatmap is going to look / play without changing display modes via a new screen. I feel like this would need to be a very visible toggle, alongside a zoom control. Have you investigated how hard it would be to change the algorithm on the fly? |
From a technical perspective it isn't difficult to have it toggleable: smoogipoo@9ce84e6 (beyond it now being a simple-ish breaking change for scrollable rulesets) But I'm not sure about the UX in the above branch. I'd really like to not spend too much time on this, because the editor is pretty far down on my priority list - this change is 1 line of code that constitutes what I think is an MVP. Mappers have used constant scroll with the same UX in the osu!stable editor for about a decade by now. |
I guess the next question is: should this also be applied to taiko? catch? Should it even be applied to osu!mania when there hasn't been a single issue/discussion complaining yet? Would like to see a bit more discussion and have a feel of it myself before making any abrupt decisions. |
Consider this the discussion thread, and that I've done this only because it's been brought up to me.
Actually, it must be applied to taiko, as it's impossible to map with SVs there due to their overlapping algorithm. I thought it was already applied but it appears not... |
I'd definitely be more okay if it's applied everywhere, put it that way. Should be okay to get this in as a starting point, then follow up with zoom and visualisation modes. |
I'm not sure about this. On a brief check, it seems like the taiko editor handles quite nicely even with the algorithm set to overlapping. I still feel like we're losing a lot by setting all the editors to |
The timeline already acts as a preview of the constant timing layout. The main area is a preview of the gameplay. That was my intended direction forward. |
It depends on the map and if there are timing points with significant enough speed to overlap. For example (and this is not an extreme one): https://osu.ppy.sh/beatmapsets/1603159#taiko/3275829 2023-08-21.19-17-47.mp4 |
I've pushed changed to make this work across all rulesets, but taiko needs consideration. I may revert this change and keep it local to osu!mania if I can't figure a solution quickly. |
@smoogipoo I came up with a good in-between solution. Can you check and make sure it looks okay to your eye? |
Would otherwise trigger IDE0055, but that isn't resolveable without an inspection cycle with resharper, so just move in a more sane place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not @smoogipoo but figure I'll drop a review so that this can maybe get into release I guess
} | ||
} | ||
|
||
private void updateScrollAlgorithm() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of scope of this pull, but I majorly like this, since it will help fix issues like SVs not taking effect immediately pretty much instantly (just subscribe to ControlPoints
updates and call this).
|
||
namespace osu.Game.Rulesets.Edit | ||
{ | ||
public abstract partial class ScrollingHitObjectComposer<TObject> : HitObjectComposer<TObject> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This though I'm feeling kind of bearish about since it's starting to feel like a potential future inheritance finger trap. But I trust that we'll be able to recognise when it gets bad enough so probably fine for now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah.
The changes look good to me. |
Here's the breaking change in the IScrollingInfo: ppy/osu#24550 And should remove the LegacyBpmMultiplier: ppy/osu#24738
Stable never had this, and it's more natural to edit in linear speed.
I thought it would be interesting to have a menu item toggle for this but it appears like we can no longer change the visualisation method after a ruleset's been loaded.
2023-09-20_22-34-34.mp4