ppy / osu

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

Spinner can be dragged past end time in editor #24950

Open Stoppedpuma opened 1 year ago

Stoppedpuma commented 1 year ago

Type

Game behaviour

Bug description

Spinner can be dragged past end time in the editor and will continue to add up until it gives bonus score and results in performance issues.

The meter fills slightly when rewinded but will instantly become full and start rewarding bonus score when I press space when it reaches the usual end time (The video doesn't show it adding bonus score because it's a bit annoying to reproduce)

Screenshots or videos

https://github.com/ppy/osu/assets/58333920/5cb0e5bf-873a-45a9-8933-32953fbc3ede

Version

2023.924.1

Logs

.

peppy commented 1 year ago

I'm pretty sure the game is supposed to handle this correctly (may be used in ranked beatmaps? not sure, but it's the same concept as virtual time in "keysounded" beatmaps with no audio track).

The editor is supposed to allow seeking beyond the end time, but that's not quite supported in lazer yet. Although I do believe stable limits that to design mode only.

Probably safe to fix this for now, though. It's an edge case which doesn't make much sense 99.999% of the time.

eoyoa commented 1 year ago

When looking to fix this issue, I found that this also applies to any TimelineHitObjectBlueprint like HitCircles and Sliders. HitCircles and Slider starts can be dragged way past the end time, while Sliders can be made to repeat past the end time as well.

I have observed that the Spinner bonus score not resetting and adding on each replay occurs consistently on every repeat after the playback has ended by means of hitting end time. EditorClock is observed to have playbackFinished be true, and the clock is stopped and the clock is seeked back to the end of the TrackLength.

For Sliders, if their end time is past the editor's end time, the audio track doesn't stop playing even though the timeline has already stopped. Pressing stop manually by clicking does stop the audio track though.

Without further investigation, I am assuming some sort of "finish HitObject" event is not called by the time playback is finished by means of hitting the end time, which is causing the bonus score to accumulate and the Slider's audio track to continue.

As such, should a HitObject be able to have any part of itself outside of the track?

I could imagine it would be less annoying to users to allow this, such as when making sliders, but I don't know if it is intended behavior.

If it is unintended behavior, you would need to fix Slider creation too.

If it is intended behavior, you would need to investigate why stopping by means of hitting the track's end time is different from manually stopping playback.

bdach commented 1 year ago

As such, should a HitObject be able to have any part of itself outside of the track?

I'd say no, full stop. As such the other issue you opened wouldn't be very much relevant because it shouldn't be possible to get in such a state to begin with.

That said there are annoying edge cases such as switching the underlying audio track for a shorter one so... not sure what to do with such cases.

eoyoa commented 1 year ago

That said there are annoying edge cases such as switching the underlying audio track for a shorter one so... not sure what to do with such cases.

Do you know what stable does with this edge case? I don't have easy access to a Windows machine right now.

A couple solutions I can think of are to invalidate bad HitObjects after a track switch (so either make the user move them after or grey them out so as to make them not playback) or just prevent track switches when bad HitObjects would exist after the switch. Not sure which would be better, so maybe checking stable before moving forward is best.

Regardless, to tentatively write a todo list for now:

I think it may still be worthwhile to find out why Slider audio doesn't end or why there is a freeze when Spinners end outside of track length, but since both won't occur if no HitObject can end outside of the track length, it might not be necessary.

I'll work on this soon.

peppy commented 1 year ago

Again, how is the virtual track case being handled? That should be of utmost importance before considering disallowing placement.

eoyoa commented 1 year ago

When you're referring to virtual tracks, you are talking about https://github.com/ppy/osu-framework/wiki/Playing-audio#virtual-tracks, right? If so, I have been exclusively working with virtual tracks when working on this issue; I don't start by putting a track in the beatmap since the original poster didn't.

Beatmaps with no track do have about a second of playback, so you are able to put down HitObjects and move them around.

Let me know if I'm misinterpreting you; I'm still not entirely sure what you're referring to.

peppy commented 1 year ago

There are many ranked beatmaps with no audio file. The game still handles this correctly, as mentioned in https://github.com/ppy/osu/issues/24950#issuecomment-1738543941. Would have to check how stable handles this, but to an extent, there needs to be a way to extend a track by placing hitobjects or otherwise.

bdach commented 1 year ago

Ok well that's also a direction one may take: permit placement of an object anywhere but ensure the underlying track is extended enough to be able to do anything to it.

Probably still needs stable crosscheck anyhow.

eoyoa commented 1 year ago

There are many ranked beatmaps with no audio file.

I tested https://osu.ppy.sh/beatmapsets/665562#osu/1408519 on stable, a ranked beatmap with no audio file (due to copyright). It seems that you are not able to place the start points of HitObjects past the end of the track, but you are able to drag HitObjects with duration (like spinners and sliders) as far as you can reach.

It seems to behave like my PR https://github.com/ppy/osu/pull/24969, but with certain differences: you can play past the end point of the track (but not manually seek), and you can grab HitObjects that end outside of the track length.

Permitting placement of an object anywhere and changing the track length to account for this therefore deviates from stable behavior. Should we go ahead with this?

Update: Editing the mentioned beatmap in lazer is bugged, the beatmap length is shorter than expected by about 30 seconds. HitObjects are therefore present past the end of the "track". Will consider making a separate issue once we resolve this.

peppy commented 1 year ago

Permitting placement of an object anywhere and changing the track length to account for this therefore deviates from stable behavior. Should we go ahead with this?

Well think of this from a mapper perspective. A mapper should not need to hack around things to get objects into their track.

I'm going to remove the "good first issue" since it's clear this will require some UX thinking/discussion.

eoyoa commented 1 year ago

Well think of this from a mapper perspective. A mapper should not need to hack around things to get objects into their track.

I do agree. It also does adequately remove the edge case of switching audio tracks for a shorter one, as the underlying track will be guaranteed to be long enough.

I feel that it would be a good idea to implement, but I do understand that deviating from stable is a rather contentious topic. That's mostly why I'm asking before I write up a PR.

peppy commented 1 year ago

I think it's fine to go ahead with a PR and we'll take discussion from there.