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

Fix bbox map select feature #567

Merged
merged 3 commits into from
Jul 11, 2024
Merged

Conversation

vgeorge
Copy link
Member

@vgeorge vgeorge commented Jul 10, 2024

This is based on the WIP PR #417. As this introduces considerable changes, I wanted to discuss them first before adding them to the bbox-map-select branch.

My main goal in this PR is to fix the bbox drawing behavior. The current implementation relies on useState and useMemo, which are hard to get right due to racing conditions with the map state and do not scale well once we start working on other data management features on the map.

In this proposed approach, we use a reducer and selectors, decoupling the application logic from the component. While this introduces some boilerplate code, the Deck.gl map works more like a presentation layer, making it more maintainable and scalable in the long run.

The bbox drawing behavior seems to be working well. The workflow is similar: click on the top-right button to start selecting, then click on the map to define the start and end points.

It is possible to inspect the selected objects by opening the browser console and looking into the application state after the final click.

I haven't implemented the selected features exchange with the Python core because it is not clear to me what is the best approach. Deck.gl's pickObjects doesn't seem to pick all objects in the bbox. It might be because it doesn't pick occluded features. The function pickMultipleObjects seems to be the only method that picks occluded features, but the selection area must be a circle. If we find a way to get the feature IDs, we can implement the approach suggested by @kylebarron, but at the moment, the only approach seems to be passing the bbox to the Python code so it can query the features.

Other known issues in this PR that we can address in the base PR:

  • Map pan drag is not disabled during bbox drawing
  • Cursor should update when drawing the bbox
  • Reducer logger is always on; we need to figure out the best way to disable it when not in development mode

@kylebarron @batpad please let me know your thoughts on this.

Copy link
Member

@kylebarron kylebarron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about reducers kinda breaks my brain, but this lgtm. I'd love for @batpad to review this too

src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Show resolved Hide resolved
@vgeorge
Copy link
Member Author

vgeorge commented Jul 11, 2024

@kylebarron Thanks for the review! I agree the reducer approach can be a little hard to understand. Personally, I would prefer using XState for handling UI workflows. It uses state charts and makes it much easier to understand what is happening where. We can discuss introducing it if the reducer approach is too cumbersome. I'll wait for @batpad review on this before merging.

@batpad
Copy link
Member

batpad commented Jul 11, 2024

This most definitely looks like a much better approach than whatever I had in the base PR - handling the state in the component was getting overly messy and to me, this is definitely an improvement.

I have no strong feelings on what approach we use for state management - this Reducer approach is familiar to me and seems fine, but I can also see how it can make the brain hurt a bit.

I've not tested this, but I definitely think this is good to merge into the base branch.

@vgeorge I think what would be needed before merging to main is having some slightly better UI elements for the bbox picking? Maybe some icons or so to start / stop selecting?

I'm 👍 to merging this into the base branch.

@vgeorge
Copy link
Member Author

vgeorge commented Jul 11, 2024

@batpad thanks! Let's track the remaining work on the base branch.

@vgeorge vgeorge merged commit c4080fa into bbox-map-select Jul 11, 2024
5 checks passed
@vgeorge vgeorge deleted the bbox-map-select-with-reducer branch July 11, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants