-
-
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
Fix timeline objects disappearing prematurely on wide-screens #18462
Conversation
@@ -302,7 +302,7 @@ private void endUserDrag() | |||
/// <summary> | |||
/// The total amount of time visible on the timeline. | |||
/// </summary> | |||
public double VisibleRange => track.Length / Zoom; | |||
public double VisibleRange => (DisplayableContent / Content.DrawWidth) * track.Length; |
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.
I was very confused as to why this was apparently "more correct" than the old version. For all intents and purposes, DisplayableContent / Content.DrawWidth
should be precisely equal to 1 / Zoom
:
osu/osu.Game/Screens/Edit/Compose/Components/Timeline/ZoomableScrollContainer.cs
Line 131 in 83c2b27
private void updateZoomedContentWidth() => zoomedContent.Width = DrawWidth * currentZoom; |
I believe that the reason why 1 / Zoom
is not working correctly, is that the bug is actually in ZoomableScrollContainer
- the container is not responding to draw size invalidations, and therefore changing the window size is potentially inadvertently changing the zoom level, therefore breaking the VisibleRange
calculation too. Here's a video - captured on this branch - showing why I think this:
2022-05-29.17-47-53.mp4
Note how the zoom level snaps abruptly when I just nudge the buttons ever so slightly. The working theory is that when the window is being resized, the value of DisplayableContent
changes, but Content.DrawWidth
stays the same, which leads to Zoom
having a stale/incorrect value.
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.
Ah, good find. Should probably have it fixed in this PR instead, will take a look whenever I get the chance.
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.
Have reverted the current change and fixed ZoomableScrollContainer
to update its width on external size changes.
This reverts commit 02baf9a.
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.
Makes sense
Turns out there's already logic for expanding the lifetime of hitobjects, and zooming in/out actually work fine. But when resizing the window to a wide aspect ratio, the issue becomes clear.
Resolved by computing the visible range more properly, by taking the ratio of the timeline's displayed portion and factor it into the track length to get the "visible time range".
Have added test coverage to ensure this works as expected.