Skip to content
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

Sync view state between Python and JS #112

Closed
Tracked by #313
kylebarron opened this issue Oct 17, 2023 · 2 comments · Fixed by #448
Closed
Tracked by #313

Sync view state between Python and JS #112

kylebarron opened this issue Oct 17, 2023 · 2 comments · Fixed by #448
Labels
javascript Relates to JS bindings

Comments

@kylebarron
Copy link
Member

kylebarron commented Oct 17, 2023

Right now we include an _initial_view_state that lets Python set the initial view state.

deckgl allows you to pass in an initialViewState param which then lets deck manage the internal view state. Or you can manage the view state independently from deck, which you update with onViewStateChange and pass into deck's viewState parameter.

  • Set the state from python but allow the JS side to vary independently (otherwise you couldn't pan)
  • Debounce for messages from JS -> Python to not clog the web socket
  • not debounce for setting the view state from onViewStateChange (because we don't want to slow the deck updates)

The existing implementation of useModelState (in anywidget/react) is:

export function useModelState(key) {
  let model = useModel();
  let [value, setValue] = React.useState(model.get(key));
  React.useEffect(() => {
    let callback = () => setValue(model.get(key));
    model.on(`change:${key}`, callback);
    return () => model.off(`change:${key}`, callback);
  }, [model, key]);
  return [
    value,
    (value) => {
      model.set(key, value);
      model.save_changes();
    },
  ];
}

We probably want something like useModelStateDebounced which returns a callback that immediately calls model.set(key, value) but debounces for model.save_changes().

Note to self: https://www.joshwcomeau.com/snippets/javascript/debounce/ for implementation of debounce + note to use useMemo in react. It's unclear if we do want useMemo because we seemingly do want to re-render the react component on every view state change, because deck is reactive and won't re-render the full map

@manzt
Copy link

manzt commented Oct 22, 2023

FWIW, I think this would be a really useful feature to add to @anywidget/react (as in a new hook or extending the existing hook).

@batpad
Copy link
Member

batpad commented Mar 27, 2024

@kylebarron I see what you're trying to do in #113 - yea, the debounce is not going to work defined where you have it - I think there's roughly two options here if we want to try with this approach:

  • Put the debounce where you have the callback for onViewStateChange: https://github.com/developmentseed/lonboard/pull/113/files#diff-c2f8ec75103ae661341f239c22352142d1d5e965cad55b19cfb978902500106cR77 . But you don't want to do that because you want to update the state and have a single source of truth - here I still think what we could have is a useModelState and useModelStateUnsynced or so - where calling useModelStateUnsynced just never calls the model.save_changes() method. And that should be okay? (And then be more intentional about when you call useModelState to actually sync with python). I can try sketching this out.
  • If you really want something like a useModelStateDebounced where that method "debounces" the calls to .save_changes(), you'd need to have a way within that function to store state of when the last call to .save_changes() had been made, and conditionally make that call - if this was a traditional REST API, I'd imagine one could create some queue system and be intelligent about it based on the response callback from the server, and cancel previous calls, etc. Would have to think about this, but could also potentially sketch something out.

@kylebarron do you know if this all works if you comment out the model.save_changes() line? I'll probably try and fix up your branch and give this a whirl - we should definitely be able to get this to work.

kylebarron added a commit that referenced this issue Apr 2, 2024
Closes #112



In #444 @batpad
mentioned

> It does seem to make the map a bit slow for me, and we should
investigate that, but in principle, this for me works as expected

While playing around with that PR, I also saw some view state lagging at
low zooms with lots of data rendered. In this screencast I'm smoothly
zooming in, but the rendering is very jagged, and sometimes zooms back
out slightly. **This screencast is of the previous PR**


https://github.com/developmentseed/lonboard/assets/15164633/bbe0a168-b76e-4563-bc30-828d648210db

My hypothesis of what's happening is that when rendering lots of data
there's a slight lag for deck to call the `onViewStateChange` handler.
And so deck will occasionally call the handler with "old" data and
that's where you sometimes get the "snap back" in the scrolling.

I think this is a really bad UX, and it seems not to happen when deck
manages the view state internally. So I think potentially the best UX
and performance solution is to decouple the view state from the model,
while still syncing from deck to Python in the debounced model handler.

This PR tries to implement this.

---------

Co-authored-by: Sanjay Bhangar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Relates to JS bindings
Projects
None yet
3 participants