-
Notifications
You must be signed in to change notification settings - Fork 39
wip: Define models for Views #908
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
base: main
Are you sure you want to change the base?
Conversation
_esm = bundler_output_dir / "index.js" | ||
_css = bundler_output_dir / "index.css" | ||
|
||
_has_click_handlers = t.Bool(default_value=False, allow_none=False).tag(sync=True) |
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.
This diff is from moving view_state
lower so that it's immediately after the new views
attribute; nothing was changed except for adding views
basemap_style = BasemapUrl(CartoBasemap.PositronNoLabels) | ||
""" | ||
A URL to a MapLibre-compatible basemap style. | ||
basemap = t.Instance(MaplibreBasemap, allow_none=True).tag( |
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.
This allows users to customize the basemap, and to turn off the basemap altogether by passing None
.
Before merging we may keep basemap_style
around to not break backwards compatibility, but I think documenting that the new API is
Map(basemap=MaplibreBasemap(basemap_style=basemap_style))
isn't too bad
mode: Literal[ | ||
"interleaved", | ||
"overlaid", | ||
"reverse-controlled", | ||
] = "reverse-controlled", | ||
basemap_style: str | CartoBasemap, |
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.
Here the user can explicitly choose a rendering mode between interleaved
(mapbox overlay set to interleaved), overlaid
(mapbox overlay not using interleaved), or reverse-controlled
, the existing behavior, which doesn't use mapbox overlay
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.
This file implements Python models for deck.gl Views, allowing a definition of a globe view and of non-geospatial views.
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.
This defines the TS/JS side of the view models, with the ability to create deck View
objects
@kylebarron The general approach seems quite comprehensive. I’d suggest implementing the different modes more progressively, as this will likely require some refactoring on the JS side. In hindsight, adding XState wasn’t the best choice on my part, it makes state management a bit harder to evolve. I’ll open a ticket to propose an alternative. I think we can still address this technical debt while it’s not too complex, but not on this PR. |
I think despite what are a bunch of changes here on the Python side, the JS side only needs to change to support two modes: One with mapbox overlay and one without. We can also merge this PR or something similar without exposing the classes to users, so we can keep developing on the JS side |
@kylebarron sounds good. Now that we merged the Playwright PR we can add specs to ensure JS features continue to work as expected. |
…lay (#921) As described in the [upstream deck.gl docs](https://deck.gl/docs/developer-guide/base-maps/using-with-maplibre), there are three ways to support using deck.gl with Maplibre: _interleaved_, _overlaid_, and _reverse-controlled_. The first two are supported via `MapboxOverlay` with a prop `interleaved: true|false`, while the latter is implemented by having Maplibre be a child of the deck.gl Map. There are worthwhile reasons to support all of these modes. We need to use interleaved or overlaid to support globe view, while reverse-controlled better supports multiple deck.gl views. This PR refactors the map component in `index.tsx` into two separate React components: a deck.gl-first renderer (i.e. "reverse-controlled") and a MapboxOverlay renderer. The idea is that this will pair with #908 to give users more control over various ways of rendering maps. This is backwards-compatible because we default to reverse-controlled, the existing default. Closes #890, closes #437, for #886, for #718
A small refactor to make it easier to read `_map.py` by removing the HTML export functionality, moving that to `_html_export.py`. This was extracted from #908, see https://github.com/developmentseed/lonboard/pull/908/files#r2414276602
In ipywidgets, a "model" is the core of reactivity. In particular, a Lonboard `Map` is reactive, but every underlying layer is _also_ reactive, so that when a user changes any property on a layer, it automatically gets updated on the map. By extension, our [`Extension`](https://developmentseed.org/lonboard/latest/api/layer-extensions/) classes are also themselves models. These are separate Python classes that get serialized as their own reactive models on the JS side. Soon we'll have more types of widget models that we want to be reactive, including views and basemap styles (#908). Right now, model initialization is slightly hard-coded to handle layer initialization. This PR refactors the model initialization to be fully generic.
The basemap refactor was extracted to #935 |
@vgeorge @felixpalmer this is progress on the Python side to be able to serialize
In particular, #906 passes a bare
projection="globe"
parameter through. I think this is too simplistic because there are too many other things to consider when switching to globe view.This PR defines all the data models for deck.gl views and maplibre basemap information so that on the JS side we should be able to precisely map the input into deck.gl calls.
This PR also sets the stage for non-geospatial map rendering and for multi-view support.
With this PR, to generate a map with globe view, someone would call
Perhaps we could add something like
projection="globe"
toviz
to permit a less-verbose way for users to define a map with a globe, but that would call the above code internally on the Python side.Relates to
ipywidgets
#905 (multi-views),TODO:
before_id
attribute to layers