-
Notifications
You must be signed in to change notification settings - Fork 27
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
WIP: Select by bounding box #417
base: main
Are you sure you want to change the base?
Conversation
@kylebarron I added a "selectionMode" and some ugly UI for a user to start selection mode and clear selection. As a bonus, when the user has a selection, it also displays the number of objects selected. There's a few things I feel a bit unsure about the logic, and also how the code is structured. We can look tomorrow - but I think this does roughly what we'd want. |
It probably makes sense to keep the bounding box state on the widget model so that Python has access to the same bounding box. So e.g. connecting to Mosaic you could pass the bounding box for them to do the same query in duckdb |
There would still a few things to be done here to finish, here's what it looks like so far: BboxScreengrab.mp4@kylebarron - Although, I'm wondering if this can be restructured to not be so tightly coupled to the main rendering code in |
src/index.tsx
Outdated
@@ -91,7 +98,7 @@ function App() { | |||
}); | |||
|
|||
const [mapId] = useState(uuidv4()); | |||
|
|||
let mapRef = useRef<DeckGLRef>(null); |
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.
nit: this and the useState above can all be const
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 think this is me writing too much Rust and always using let
...
src/index.tsx
Outdated
getPolygon: (d) => d.polygon, | ||
}); | ||
} else if (selectionStart) { | ||
// Show the selection start point (note this does not show the proposed bounding box, but could be done) |
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.
// Show the selection start point (note this does not show the proposed bounding box, but could be done) | |
// Show the selection start point |
Since a hover box is now shown
@kylebarron for now, this just sets As an approach, I think I prefer this than trying to send indexes for all the objects down from JS - can we not implement this as a method in python and just use the bbox sent by the frontend? So a I feel like this is almost ready for review -- not sure what else we should do here. @hanbyul-here would you be able to give this a look and point out anything obvious that you feel should be changed? There's possibly a few things that could maybe be done nicer?
Also this was my first time really writing TypeScript so if there's anything there that seems like it's not proper.. One thing to note:
|
I think there are pros and cons to each manner of implementation, but I think that there are some cases where maintaining the indexes on the model itself is valuable. It might also enable more client side use cases where you're syncing the data and the selection with another external widget through JS, avoiding the Python kernel. And not needing to do another search query on the Python side might be nice for performance as well. And if we're sending up to like 100k uint32s encoded in Parquet to the server, that's a pretty small file. Probably smaller than 50kb? |
Note that a user's selection on the screen is in pixels, and does not always align with an axis-aligned bounding box. E.g. if the map is tilted, the pixel bounding box will accurately choose items within that view because deck.gl uses the actual scene rendering for picking. But that won't be able to be picked from Python because you'd have to serialized the entire view and camera state and implement 3d picking from scratch. |
@kylebarron based on our conversation, I've made it now so that each layer has a |
I think this is getting closer; I would like to see if we can refactor the meat of the selection into a separate function so it doesn't bloat up the |
cc @LanesGood |
@batpad in response to your request for a design review of the bbox selection instructions/icon:
|
I don't think there's any way to use the ipyleaflet draw controls because we're not using leaflet under the hood. |
Hey folks, in #567 we fixed the bbox select drawing issues. Items moving forward:
|
Building on top of #412 with help from @batpad
Change list
Notes
pick
callback directly onto the models, and then iterate over each model, callingpick
which picks and then assigns directly onto that model's state?In testing, we seemed to get only 35k rows back when we got 143k rows checking the same bbox in geopandas with the input gdf. This is possibly related to x,y,width, height passed in toFixed with latest commitpickObjects
?