Highlight features spread across tiles #4365

jesusbotella · 2020-03-09T11:06:09Z

Background

Right now, only one feature is highlighted when autoHighlight property is enabled and the feature is split into several tiles. We would need to highlight the whole feature instead of only the hovered part.

I misunderstood the object index and returned the index within the provided data array instead of the picking index. Is there a way to get the picking index from a given property?

Change List

Add highlightedObjectIndex to rendered sublayers in TileLayer.
Implement getHighlightedObjectIndex in MVTTileLayer.
Save hoveredFeatureId into MVTTileLayer state.

coveralls · 2020-03-09T11:20:20Z

Coverage decreased (-2.6%) to 80.599% when pulling 8e7b97f on CartoDB:jb/mvttilelayer-highlight into 849a8b9 on uber:master.

Pessimistress · 2020-03-09T22:30:24Z

I think one fix that needs to happen is in GeoJsonLayer - pickingInfo returned by GeoJsonLayer should contain the index of the overall feature array, not the index in the sublayer. I will look into this.

VictorVelarde · 2020-03-16T17:31:45Z

Hi @Pessimistress will that fix be available for the next 8.1.0 release?

Pessimistress · 2020-03-16T18:15:37Z

will that fix be available for the next 8.1.0 release?

We are releasing soon - the fix will land as a patch.

VictorVelarde · 2020-03-17T12:54:43Z

Great, ping us if you need any help or review on that!

Pessimistress · 2020-03-18T16:14:04Z

@jesusbotella Landed #4403, this PR should have been unblocked.

jesusbotella · 2020-03-19T18:11:41Z

Thank you @Pessimistress! That was fast!

I have just tested my code after the merge and it works but I have some problems with particular polygons. Here's a GIF:

Particularly with the top big polygon, it works to highlight most of it but there's a part that remains unhighlighted.

I've uploaded the example that I'm using so that you can see what happens. Some features are successfully highlighted but others don't.
highlightExample.zip

I need to tweak the code a bit so that features stop being highlighted, but that'll come later.

Thank you!

Pessimistress · 2020-03-27T03:30:43Z

modules/geo-layers/src/mvt-layer/mvt-layer.js

@@ -9,6 +9,11 @@ import ClipExtension from './clip-extension';

 const WORLD_SIZE = 512;

+const defaultProps = {
+  ...TileLayer.defaultProps,


defaultProps of the base class is automatically merged.

jesusbotella · 2020-03-27T14:31:08Z

I've pushed commits to make it work properly with @Pessimistress changes.

First of all, I've tweaked the code to unhighlight features when mouse is moved off the feature.

Another important thing is that I've changed MVT custom highlighting to only work when autoHighlighting is active, mimicking its functionality but selecting the whole feature instead of the split parts of the whole feature.

And I noticed that this change broke highlightedObjectIndex in GeoJSON layer and MVT layer, so I returned highlightedObjectIndex value in getHighlightedObjectIndex if present.

Let me know what you think, and if you like it, I'll add docs and tests.

AdriSolid · 2020-04-14T19:49:16Z

Hi! Thanks for working on the MVTLayer, awesome!

I'm faced the splits parts once a feature is highlighted, on version 8.1.1

VictorVelarde · 2020-04-22T14:25:28Z

@AdriSolid That "partial highlighting" should be fixed when this PR gets merged.

Regarding to that, @Pessimistress do you think that we should go on and create docs & tests for it with the current approach for unhighlighting?

coveralls · 2020-04-29T09:27:53Z

Coverage increased (+2.7%) to 83.203% when pulling f3e09e8 on CartoDB:jb/mvttilelayer-highlight into 1cecdca on visgl:master.

jesusbotella · 2020-05-06T08:44:24Z

Hey @Pessimistress! I have just set this PR as ready for review. It would be great if you (or any other contributor) could give it a look and tell us your thoughts.

Thank you very much!

docs/layers/mvt-layer.md

modules/geo-layers/src/mvt-layer/mvt-layer.js

Pessimistress · 2020-05-06T21:33:19Z

modules/geo-layers/src/tile-layer/tile-layer.js

@@ -20,7 +20,8 @@ const defaultProps = {
  maxCacheSize: null,
  maxCacheByteSize: null,
  refinementStrategy: STRATEGY_DEFAULT,
-  zRange: null
+  zRange: null,
+  getHighlightedObjectIndex: {type: 'function', value: () => -1, compare: false}


This is not documented.

If this is going to be a public API, we need to look into whether it's sufficient to just pass it the tile argument.

I think it's ok to not expose this until we find other use cases.

Yeah, makes sense. Let's remove it and wait until we find some other use cases.

modules/geo-layers/src/tile-layer/tile-layer.js

modules/geo-layers/src/mvt-layer/mvt-layer.js

Pessimistress · 2020-05-06T21:37:57Z

modules/geo-layers/src/mvt-layer/mvt-layer.js

+  const hasFeatureId = Boolean(feature.id);
+  const hasUniquePropertyId = Boolean(uniquePropertyId);
+
+  if (hasFeatureId) {


I'd expect the user-specified hasUniquePropertyId to override the default

Sure, makes more sense. I have just changed it.

jesusbotella · 2020-05-07T16:41:56Z

I have just changed the suggestions mentioned in the review.

I have also had to remove the interaction test because it was fragile. I guess that I have not been able to guess a proper wait time for firing the hover events and the property checking. I don't know if you have some ideas to make it work (besides increasing timeout).

jesusbotella · 2020-05-25T08:05:51Z

Hi @Pessimistress! Sorry for disturbing, I performed the review changes, and I would like some validation whenever you can. Thank you!

Pessimistress · 2020-05-26T20:30:38Z

docs/layers/mvt-layer.md

+
+An string pointing to a tile attribute containing a unique identifier for features across tiles.
+
+##### `highlightedFeatureId` (Number)


Does the feature id have to be a number? Is string accepted? If so, the default should be null for clarity?

As per MVT docs, feature ID can be only a number. But it's true that if anyone uses uniqueIdProperty to reference an ID within feature properties, it could be a string.

I have just changed it to support strings as well in the code. Let me know if you see something weird.

modules/geo-layers/src/mvt-layer/mvt-layer.js

Co-authored-by: Xiaoji Chen <Pessimistress@users.noreply.github.com>

jesusbotella · 2020-05-27T13:25:31Z

I see that coverage has decreased. I tried to make a module test for highlighting to improve it, but I don't know how to make it pass because fetch is not supported in Node and the only way for MVT Layer to load data is through a URL.

ibgreen · 2020-05-27T14:23:04Z

I see that coverage has decreased. I tried to make a module test for highlighting to improve it, but I don't know how to make it pass because fetch is not supported in Node and the only way for MVT Layer to load data is through a URL.

I am not up-to-date on the test setup in deck.gl, but the @loaders.gl/polyfills module installs a polyfill of fetch under Node.js. This is used for all Node.js tests in loaders.gl, so it is pretty well tested.

jesusbotella · 2020-05-27T15:08:17Z

I am not up-to-date on the test setup in deck.gl, but the @loaders.gl/polyfills module installs a polyfill of fetch under Node.js. This is used for all Node.js tests in loaders.gl, so it is pretty well tested.

Thank you @ibgreen! I tried modifying this file (https://github.com/visgl/deck.gl/blob/master/test/modules/geo-layers/mvt-layer.spec.js) but I believe that the output already prints a fetch not available in node message for those tests as far as I see.

jesusbotella · 2020-05-27T17:52:54Z

Ok, I have been able to get more insight on those test issues.

First of all, I think I have found a bug because we're just passing a simple template string to the sample data in the MVT test, but we're not considering it as valid in getURLFromTemplate and the tests print a Invalid URL message. But those tests were passing regardless of that.

And then, when I change it to an array, I think the helper is not passing the proper data value to Layer should have sublayers tests because these errors pop up due to the failing tiles as you can see in the image below:

Below those errors, I still see some fetch not available in node messages, but I cannot see where those errors come from because it seems like they come from the same test in the image as there are no other test case name below.

Going to debug for a bit and see if I can fix them.

jesusbotella · 2020-05-28T07:39:11Z

I kinda found a fix for those errors, that is checking if our template value type has a replace method. That way we avoid passing unwanted values like { length: 1 } to MVTLoader and it doesn't throw an error about selectedTiles anymore.

About fetch, there is this line in Node tests setup:

deck.gl/test/node.js

Lines 3 to 5 in 583cde5

    
           // Polyfill for loaders 
        
           // TODO - @loaders.gl/polyfills seems to freeze the tests 
        
           global.fetch = () => Promise.reject('fetch not available in node');

It seems difficult for me to test highlighting without tile data, so I don't know if you have a clue for me to inject that data into the layer and if you have some scheduled work to see why using @loaders.gl/polyfills froze the tests or not. I could give it a try and see the cause when I have time.

VictorVelarde · 2020-06-03T17:00:54Z

@ibgreen @Pessimistress we're a bit stuck on this... any advice on how we could move on?

Pessimistress · 2020-06-03T17:59:41Z

@jesusbotella Since we are not testing the loader here, you can bypass the default loading routine with this:

class TestMVTLayer extends MVTLayer {
  getTileData(tile) {
    // return parsed MVT data directly
  }
}

ibgreen · 2020-06-03T18:01:28Z

@VictorVelarde Apologies the issue is not clear to me, is it still a matter of the tests? The CI tests do seem to be passing...

We could certainly get fetch working in Node tests similar to how it works in loaders.gl if @Pessimistress thinks that makes sense.

You could also just disable your tests when running under Node.js and only do these tests in the browser.

Pessimistress · 2020-06-03T19:12:12Z

We could certainly get fetch working in Node tests similar to how it works in loaders.gl

Loaders has a quite complex system to reroute URLs in both node and browser. I prefer not to add that extra layer of complexity if we don't have to.

jesusbotella · 2020-06-05T14:35:59Z

Thank you both for your comments!

@ibgreen @Pessimistress There's no need to make fetch work with those tests. I think that it's better to override getTileData and return a plain data object as @Pessimistress said.

The PR is ready if we forget about coverage decrease, but not feeling happy about that 😓
That said, I think I am hitting a rock with the tests 😓 .

The thing is that the method that I added in this PR (getHighlightedObjectIndex) to return the actual index of the feature in the data array rather than feature ID is not working properly on the tests.

The method needs the actual data to find the feature within the tile data array, but the tile data that I get when the method is called in the tests is a promise. I understand that it gets wrapped in a promise in case getTileData returns a promise, so no problem. But I don't know how to test the code that I added.

When using the layer in a normal environment, I'm relying on a layer re-render that triggers on every tile load to find the index, and here's not happening.

So, I don't know what to do now, to be honest :(

Here's the test that I did, and wanted to check highlightedObjectId within all sublayers, but I have not been able to do that:

test('MVT Highlight', t => {
  class TestMVTLayer extends MVTLayer {
    getTileData() {
      return geoJSONData;
    }
  }

  const testCases = [
    {
      props: {
        data: ['https://a.basemaps.cartocdn.com/rastertiles/voyager_labels_under/{z}/{x}/{y}.png'],
        id: 'mvt-highlight-test',
        filled: true,
        pickable: true,
        autoHighlight: true,
        highlightedFeatureId: 1
      },
      onAfterUpdate: ({ subLayers }) => {
        for (const layer of subLayers) {
          t.comment(layer.props.pickable);
          t.comment(layer.props.autoHighlight);
          t.comment(layer.props.highlightedObjectIndex);
          t.comment(layer.props.highlightedFeatureId);
        }
      }
    }
  ];

  testLayer({ Layer: TestMVTLayer, testCases, onError: t.notOk });

  t.end();
});

Pessimistress · 2020-06-05T17:15:25Z

It's a long due backlog item to support asynchronous checks in testLayers. :( I suggest we land this PR with this test skipped (test.skip) and I will look into fixing it.

FYI if you rebase master the coverage should go up.

jesusbotella · 2020-06-05T17:24:37Z

It's a long due backlog item to support asynchronous checks in testLayers. :( I suggest we land this PR with this test skipped (test.skip) and I will look into fixing it.

Oh, didn't know about that, but don't worry about fixing it for this. It sounds as a relief because I was kinda hitting my head against the wall 😁

Ok, so I am gonna commit it and skip that test. I'm not worried about this particular test because there's another one with a render image that works perfect, so we are good with this.

jesusbotella added 2 commits March 9, 2020 11:34

Add highlightedObjectIndex method to TileLayer

0e02049

Handle getHighlightedObject in MVTTileLayer

a2ace35

Pessimistress mentioned this pull request Mar 18, 2020

Fix GeoJsonLayer's handling of highlightedObjectIndex #4403

Merged

Merge branch 'master' into jb/mvttilelayer-highlight

8e7b97f

Pessimistress mentioned this pull request Mar 27, 2020

Fix GeoJSON multi-geometry highlighting #4426

Merged

Pessimistress reviewed Mar 27, 2020

View reviewed changes

jesusbotella added 3 commits March 27, 2020 13:44

Handle unhighlighting

46426e0

Highlight MVT features only on autoHighlight

056baa5

Return highlightedObjectIndex in getHighlightedObjectIndex if present

8ec5756

Merge branch 'master' into jb/mvttilelayer-highlight

bcbc750

jesusbotella added 3 commits April 29, 2020 13:53

Add render tests

e4d922a

Add interaction test

dd700a5

Add uniqueIdProperty and highlightedObjectIndex properties to docs

b7b134a

jesusbotella marked this pull request as ready for review May 6, 2020 08:42

Pessimistress reviewed May 6, 2020

View reviewed changes

jesusbotella added 2 commits May 7, 2020 15:11

Address review comments

d237c98

Remove MVT Layer picking test

99a94f4

Pessimistress approved these changes May 26, 2020

View reviewed changes

jesusbotella and others added 3 commits May 27, 2020 13:00

Set highlightedFeatureId to null by default and add robust check

839a186

Check feature id with in keyword

1f56d74

Co-authored-by: Xiaoji Chen <Pessimistress@users.noreply.github.com>

Change MVT layer docs to reflect property changes. Linter changes.

a1549aa

Consider as invalid any template data without replace method

60cc865

jesusbotella added 2 commits June 5, 2020 19:30

Add MVT highlight node test

96a3351

Merge branch 'master' into jb/mvttilelayer-highlight

f3e09e8

Pessimistress merged commit d52a63f into visgl:master Jun 5, 2020

Pessimistress mentioned this pull request Jun 7, 2020

[test-utils] Add testLayerAsync #4659

Merged

Pessimistress mentioned this pull request Jun 25, 2020

Fix bundle config #4725

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Highlight features spread across tiles #4365

Highlight features spread across tiles #4365

jesusbotella commented Mar 9, 2020

coveralls commented Mar 9, 2020 •

edited

Pessimistress commented Mar 9, 2020

VictorVelarde commented Mar 16, 2020

Pessimistress commented Mar 16, 2020

VictorVelarde commented Mar 17, 2020

Pessimistress commented Mar 18, 2020

jesusbotella commented Mar 19, 2020

Pessimistress Mar 27, 2020

jesusbotella commented Mar 27, 2020

AdriSolid commented Apr 14, 2020

VictorVelarde commented Apr 22, 2020

coveralls commented Apr 29, 2020 •

edited

jesusbotella commented May 6, 2020

Pessimistress May 6, 2020

jesusbotella May 7, 2020

Pessimistress May 6, 2020

jesusbotella May 7, 2020

jesusbotella commented May 7, 2020

jesusbotella commented May 25, 2020

Pessimistress May 26, 2020

jesusbotella May 27, 2020

jesusbotella commented May 27, 2020

ibgreen commented May 27, 2020

jesusbotella commented May 27, 2020 •

edited

jesusbotella commented May 27, 2020 •

edited

jesusbotella commented May 28, 2020 •

edited

VictorVelarde commented Jun 3, 2020

Pessimistress commented Jun 3, 2020

ibgreen commented Jun 3, 2020

Pessimistress commented Jun 3, 2020

jesusbotella commented Jun 5, 2020 •

edited

Pessimistress commented Jun 5, 2020 •

edited

jesusbotella commented Jun 5, 2020


		An string pointing to a tile attribute containing a unique identifier for features across tiles.

		##### `highlightedFeatureId` (Number)

Highlight features spread across tiles #4365

Highlight features spread across tiles #4365

Conversation

jesusbotella commented Mar 9, 2020

Background

Change List

coveralls commented Mar 9, 2020 • edited

Pessimistress commented Mar 9, 2020

VictorVelarde commented Mar 16, 2020

Pessimistress commented Mar 16, 2020

VictorVelarde commented Mar 17, 2020

Pessimistress commented Mar 18, 2020

jesusbotella commented Mar 19, 2020

Pessimistress Mar 27, 2020

Choose a reason for hiding this comment

jesusbotella commented Mar 27, 2020

AdriSolid commented Apr 14, 2020

VictorVelarde commented Apr 22, 2020

coveralls commented Apr 29, 2020 • edited

jesusbotella commented May 6, 2020

Pessimistress May 6, 2020

Choose a reason for hiding this comment

jesusbotella May 7, 2020

Choose a reason for hiding this comment

Pessimistress May 6, 2020

Choose a reason for hiding this comment

jesusbotella May 7, 2020

Choose a reason for hiding this comment

jesusbotella commented May 7, 2020

jesusbotella commented May 25, 2020

Pessimistress May 26, 2020

Choose a reason for hiding this comment

jesusbotella May 27, 2020

Choose a reason for hiding this comment

jesusbotella commented May 27, 2020

ibgreen commented May 27, 2020

jesusbotella commented May 27, 2020 • edited

jesusbotella commented May 27, 2020 • edited

jesusbotella commented May 28, 2020 • edited

VictorVelarde commented Jun 3, 2020

Pessimistress commented Jun 3, 2020

ibgreen commented Jun 3, 2020

Pessimistress commented Jun 3, 2020

jesusbotella commented Jun 5, 2020 • edited

Pessimistress commented Jun 5, 2020 • edited

jesusbotella commented Jun 5, 2020

coveralls commented Mar 9, 2020 •

edited

coveralls commented Apr 29, 2020 •

edited

jesusbotella commented May 27, 2020 •

edited

jesusbotella commented May 27, 2020 •

edited

jesusbotella commented May 28, 2020 •

edited

jesusbotella commented Jun 5, 2020 •

edited

Pessimistress commented Jun 5, 2020 •

edited