ppy / osu

rhythm is just a *click* away!
https://osu.ppy.sh
MIT License
14.67k stars 2.17k forks source link

Implemented ability to adjust already-placed objects when changing timing offsets #28382

Open Hecatia-Lapislazuli opened 1 month ago

Hecatia-Lapislazuli commented 1 month ago

Closes #27915

UI could potentially be adjusted? Unsure exactly. Am copying the text from stable in this case for the button.

image

Will leave as draft PR for a bit, as I did want feedback on the above, but I also just want to make sure am not introducing any bugs first as I previously noticed some weird behaviour with sliders when adjusting BPM

Namely, they disappeared after the half way point passed when playing, as well as slider length not adjusting during BPM changes, until you hit the undo key which does reflect what should've happened.

Although currently I can't seem to replicate this behaviour, so may've been something else.

bdach commented 1 month ago

Visually immediately I think it's a very strange place to put the toggle in.

If anything I'd put it in the top menu somewhere. "Timing" menu most likely.

Hecatia-Lapislazuli commented 1 month ago

I think that should address the initial comments, for the most part anyways, besides for where this toggle should exist.

It could work here, although I don't feel as a user this would be where I'd expect this toggle to be.

image

Maybe here instead, below use current time?

image
bdach commented 1 month ago

It could work here, although I don't feel as a user this would be where I'd expect this toggle to be. Maybe here instead, below use current time?

I don't think that's better.

If you're insistent on keeping it where it is right now I will require a second opinion from someone from @ppy/team-client to resolve this because I do not consider the current placement to be correct.

Another point (and perhaps one that illustrates why I have the problem with the above) is the inconsistency of applying the toggle. There are four places on the timing sidebar where one can adjust timing point offset & BPM, but this setting only applies to half of them:

image

Is this expected? I would not expect that to be the case. If this is to work it should work for all four highlighted controls IMO.

Hecatia-Lapislazuli commented 1 month ago

Ah, I see. Misread the code and thought the other buttons used the same function to adjust these settings, hmm...

I think the easiest way to solve this for adjusting BPM is moving the code to TimingControlPoint.BeatLengthBindable, but not sure yet for timing... I'll look into it.

Have seen a bug with the current implementation though, if a user were to select more than one control points... but I feel resolving this will resolve that bug.

As for UI, I'm not held on to where I currently have it, is mostly just in the top menu is not where I'd expect it. In stable it was a fair bit more prominent. Although in truth, I feel the more I think about it in said top menu might look the best regardless... Anywho, a second opinion would be good still.

bdach commented 1 month ago

moving the code to TimingControlPoint.BeatLengthBindable, but not sure yet for timing...

No that is the wrong place. This logic should live in the editor specifically and not pollute timing points. Timing points should be dumb data.

TimingSection is probably where you want that to live. Or some static helper method.

Hecatia-Lapislazuli commented 1 month ago

This should be most things now - I only remembered that I shouldn't be placing editor stuff in non-editor namespaces until after pushing, so I'll do another commit later moving the functions AdjustHitObjectOffset and SetHitObjectBPM elsewhere.

Not sure where exactly. EditorBeatmap could make some sense, but feels bad for organization. I think best would be a new class in osu.Game.Screens.Edit.Timing somewhere.

I'll also work on some tests too.

Other than that, let me know if there's anything else to be done.

Hecatia-Lapislazuli commented 4 weeks ago

I'll mark this as ready for review now as I think all the major things should be addressed (bar maybe some oddity with function arguments and naming)

I'll mention feedback I did receive again, though, which is namely there should be a setting for future control points to be adjusted when you change either the timing or bpm. (such as by adjusting either the timing or bpm, the beats a given control point is away from the adjusted timing control point is maintained)

But this should probably be a discussion topic before implementing it, just going to mention it for now.