-
Notifications
You must be signed in to change notification settings - Fork 16
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
[RFC] Spatial Filter #482
Comments
Hi @aaranadev, thx for the proposal. @bbecquet did a first review and we checked some aspects of it together, to see technical feasibility. We think expanding the Spatial Filter options make sense, at least partially, but we still have some questions to ask, to clarify RFC. First some technical comments, then questions. Technical feasibility
Questions
cc/ @moimart |
Hi @VictorVelarde , thx for the answer. With my POC, I've been able to check the technical questions and State and deck.gl are working ok. About UX/UI, I think that is the more complex piece too. Questions
If the inclusive/exclusive switch is too complex to add in the UX/UI of cc: @efernandezleon |
Hi all, thanks for the proposal, this looks like a great feature. Some thoughts below: Performance The more complex the mask gets the slower widgets will run. For points the performance is generally OK, but for lines it is slower and for polygons even slower. As a rough rule of thumb, the runtime will scale linearly with the number of vertices the mask polygon has. This performance hit comes from C4R not deck.gl. A way to improve this would be to not store circular masks as polygons, but rather as points with a radius. Rendering this with deck is straightforward via a Separately, from a UX point of view, it may be worth adding a limit on the mask complexity - or at least a warning. exclusive/inclusive modes Regarding exclusive/inclusive: the naming seems unclear to me. Instead I think of it as:
From a deck.gl perspective, the Intersect mode is not supported and the way to implement it would be to compute the overlapping region in C4R prior to passing this off to deck.gl. Agree with @VictorVelarde that this mode is more complex & risky to implement |
Ok, now I think I understand better your proposal @aaranadev. I think you want a couple of quite different things…
(a) SINGLE MASK. I think that both from [deck.gl](http://deck.gl) & c4r, we see this first piece as a feasible change, not requiring too big changes in the architecture of spatialFilter. (b) MULTIPLE MASKS. In this case, the impact is potentially quite bigger and we need more info. Let me add some comments / questions:
Perf note: from Felix’s feedback… adding complex geometries would have a clear impact over perf, so yes I also think displaying a warning over XXX vertices would be required (and then we might consider the complexity of using custom structure for Circles in a second step). cc/ @bbecquet |
Great! I think that the (a): SINGLE MASK, all agree, but I don't understand
Can you explain me? On another side (b) MULTIPLE MASK Yes, the impact is bigger because you need to do an intersection by every position of the array.
I have a POC but is not accessible because I've copied several parts of c4r and I have done the hack for |
There can be max four independent masks. Think of each of these as a single color bitmap that contains a rendered deck.gl layer. This layer can be a composite layer, which contains collection of sublayers, so in practice you can render as many layers as you want into a mask. You will have to pay the cost of them being rendered any time the mask changes, which can often be every frame. Standard (non-mask) layers can specify one (and only one) mask for masking. It is not possible to read two masks and apply the result of both to a layer.
@aaranadev it would be good to see your PoC, to see how you've implemented the Intersect mode |
@felixpalmer @VictorVelarde, I've uploaded the code to public repo. https://github.com/aaranadev/c4r-compare-mode You can see the code and test it. When |
@aaranadev this code https://github.com/aaranadev/c4r-compare-mode/blob/main/src/components/layers/MaskLayer.ts#L16 in your example is not doing an intersect. I mean, the |
Yes, it's the same code that C4R has but changing an array of one element by n elements |
Before entering in the technical discussion I'd go a step back and getting more info why do you need this feature. I'm asking because it looks like it's related to comparison mode and I think there are approaches far simpler creating a new widget.
Looks like a good feature for CARTO for React. Agree on this
Not agree with this approach. There are simpler ways of implementing it. Like creating a new widget that can ask a source to get the data filtered by a custom geom. I think using the FeatureSelection Widget for this is making it far more complicated I'll propose a solution early next week |
I've setup a meeting next Monday to discuss it and we'll post the conclusion here. We've proposed this feature because we want to have a comparison mode in site selection. This proposal works but requires a huge effort that it's not required to accomplish the comparison mode in site selection.
I think it could be addressed with a simpler approach.
|
Background
We need the functionality to add more than one geometry to allow filtering or drawing on the map.
Problem
Implementing his feature right now is not possible without ejecting it from the Carto4React completely and rebuilding them all from scratch. The current widget
FeatureSelectionWidget
available in the Carto4React library does not provide this functionality.Why do we need that
That could be used to share the same implementation across many projects and reduce the time of development for the applications, focussing on providing real value for our customers.
Proposal
The proposed implementation is divided into two different implementations.
FeatureSelectionWidget
The widget must allow the show of various geometries together and if the union between them is inclusive or exclusive.
Spatial Filter
The spatial filter feature must allow saving multiple geometries and show them in
FeatureSelectionLayer
andMaskLayer
POC
The text was updated successfully, but these errors were encountered: