magland / figurl-timeseries-views

0 stars 0 forks source link

TimeScrollView panning problem with large dataset #2

Closed magland closed 1 year ago

magland commented 1 year ago

See https://figurl.org/f?v=gs://figurl/spikesortingview-10&d=sha1://dfb82b18910ba97aa30ef299cc5fd2cd0f9dbe08&label=Timeseries%20graph%20example

Zoom in and then start panning quickly in one direction. You should notice choppiness (plot moves back and forth even though panning is only one direction). It might be a bug with throttling. I'm not so concerned about the rendering speed - this example was intentionally chosen to have slow rendering. The problem is with the back-and-forth choppiness.

@jsoules

jsoules commented 1 year ago

As best as I can currently determine, this is down to the rendering speed for the visualization being close to the throttle delay.

As a reminder, throttling works to decrease the rate of user inputs changing program state. When the user first tries to interact, we set up a holding pen for what they want the new state to look like, and we arrange to wait (e.g.) 100 ms and then change the state to match whatever is in the holding pen at that time. Any user input before the scheduled commit event occurs just replaces what's in the holding pen. (Or metaphorically: the waiter starts asking your party what they're ordering. You pick something in your mind, but you can keep changing your mind as much as you want; it's only when it is your turn that you say your final choice and that's what the waiter writes down.)

If you look closely at the linked example, when you start panning, there's usually a small jump in the data right after the first mouse movement. This is the first render. Once that render starts, we are into a new throttling cycle--based on the visible window that's currently displayed. But if that render is slow enough, it's possible we will queue another render from the original displayed data before the new one finishes. That one starts rendering, but in that lag time, the first update completed, moving the base window a little bit. Now the same mouse location corresponds to a slightly smaller time delta than it did under the first render. So even the slightest mouse movement will schedule another render, only it will be targeting a time point that's a little bit closer to the original time point (a little bit of rolling back).

More concretely:

Suppose we have a window of 20 seconds, centered at 1:14. User clicks on 1:20 to start dragging the mouse to the left in order to advance the window. User moves the mouse slowly from 1:20 to 1:15 for a total expected movement of 5 seconds.

A few different things have to line up for this to be a problem:

So I don't expect that this would show up all the time or anything. But it should be fixed.

Overall, I think the best way to resolve this is to switch the throttling paradigm of the drag action from a throttle (act X seconds after first input) to a debounce (act X seconds after the last input). The drawback of this is that you won't get as many intermediary states if a user is steadily dragging--the user has to stop moving the mouse for a little bit before the render will happen--but I think this should ensure that scheduled renders always happen against what's actually displayed on the screen.

magland commented 1 year ago

I don't think we should move to debounce, because it's important for user to see the panning as they move. There must be a way to solve this without switching to debounce.

jsoules commented 1 year ago

To be clear, the debounced version would still schedule a rerender whenever the user goes 50 ms without moving the mouse. So they'd still likely see changes during the course of a drag.

jsoules commented 1 year ago

It's a pretty straightforward change that I've already coded up, so if you'd like to see it in action and see if the behavior meets your expectations, just remind me how to publish it and I can push it somewhere.

magland commented 1 year ago

Just make a devel/deploy_dev.sh as in the other projects

jsoules commented 1 year ago

I've played around with this some more, and while it does solve the problem and perform acceptably, I agree that it doesn't provide feedback as readily in the (hopefully) modal case of preformant renders.

Thing is, without it I'm a little stuck about how to fix. I'd like to avoid over-complicating what I think is a pretty clean implementation of throttling/debouncing; but at the risk of doing so, one thought that comes to mind is to implement some sort of refraction condition--part of the state that will prevent further state updates until it gets reset. In this instance, we could have part of a useEffect on the TSV that ticks this over, so the drag-panning becomes available again only after the prior render is complete. But that might also be a bad user experience in other ways, most notably that the UI is going to appear (or, well, be) unresponsive for even longer.

On a related note, we could increase the delay on the throttle. But that won't necessarily fix a sufficiently long-running render, while it will make things less responsive in the general case.

magland commented 1 year ago

Sorry for not following all the details, but I don't understand... are the renders playing in chronological order from where the mouse position was? If so, we wouldn't have this problem.

Am I simplifying too much?

jsoules commented 1 year ago

The renders play in chronological order from where the mouse position was at the time the render was requested. But that mouse position is (by necessity) relative to what's in the window. And subsequent renders can change the time represented by any particular point in the window.

So if the renders are slow enough that you can complete the throttled render-request cycle before the render completes, you can have a render that's queued based on the earlier window state, then the window moves underneath the mouse, and the next requested render (in chronological order) no longer makes sense.

To try to clarify further: the renders are played in the order in which they're received, I assume this is what you meant (there isn't a mechanism to try to reorder them later just because a subsequent render requested to pan toward an out-of-order point, and it isn't really clear that you'd want to do that anyway since moving back and forth is a legitimate user action)

jsoules commented 1 year ago

see https://www.figurl.org/f?v=gs://figurl/timeseries-views-1dev&d=sha1://dfb82b18910ba97aa30ef299cc5fd2cd0f9dbe08&label=Timeseries%20graph%20example&s={} for the debounced version--it fixes this problem but may introduce worse UX for other cases

magland commented 1 year ago

The renders play in chronological order from where the mouse position was at the time the render was requested. But that mouse position is (by necessity) relative to what's in the window. And subsequent renders can change the time represented by any particular point in the window.

I think this is the problem. The mouse position should not be relative to what's in the window -- it's just the mouse position in pixels. The current time shift should only be dependent on (a) x1 = the current mouse position - in pixels - not relative to what's in the window (b) x0 = the anchor mouse position - also in pixels - the position of the mouse when drag was started (c) t0 = the anchor timepoint - the timepoint of the mouse when the drag was started

Then the shift should be computed based on these three quantities. (the formula is a bit tricky)

Note that the computed shift should always be relative to when the drag started, not the last rendered position.

jsoules commented 1 year ago

I see what you mean. It can be simpler--if we know x1 (pixel current position), x0 (pixel anchor position), and the ratio of pixels to time, we can compute the time delta from the original anchor time, and don't need to worry about updates. I'm not sure why this wasn't written this way originally--maybe something to do with keeping the RecordingSelectionContext from having any dependency on the window size?

In any case, I'll code up something like this and see how that works.

magland commented 1 year ago

Fixed by #4