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

HexagonLayer: Aggregate data using valid viewport. #2196

Merged
merged 2 commits into from
Aug 23, 2018

Conversation

1chandu
Copy link
Contributor

@1chandu 1chandu commented Aug 15, 2018

For #2111

Background

Re-aggregate and update hexbins when viewport is changed.

Change List

  • HexagonLayer: Re-aggregate data when viewport is changed.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

I suspect that this bug is a result of all the refactorings we have done to support multiple viewports.

I have a feeling that this PR mainly works around the real problem (viewport initially being undefined).

If that was fixed, would we still want to reaggregate when the viewport is changed? Wouldn't that turn this into a ScreenHexagonLayer?

Are the results of the aggregation actually viewport dependent (lng/lat/zoom) or do we just need a mercator projection? If the latter, can we just create a "working viewport" during aggregration (instead of depending on the passed in viewport)?

return (
oldProps.radius !== props.radius || oldProps.hexagonAggregator !== props.hexagonAggregator
changeFlags.dataChanged ||
changeFlags.viewportChanged ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

To get viewportChanged notifications, I think you need to define the shouldUpdateState method.

@1chandu
Copy link
Contributor Author

1chandu commented Aug 16, 2018

Are the results of the aggregation actually viewport dependent (lng/lat/zoom) or do we just need a mercator projection? If the latter, can we just create a "working viewport" during aggregration (instead of depending on the passed in viewport)?

It uses viewport for 1. calls projectFlat and unprojectFlat 2. uses viewport's distanceScales.

So if any change in above factors, we should re-aggregate.

@1chandu
Copy link
Contributor Author

1chandu commented Aug 16, 2018

Digged more into this

  1. During the layer initialization, the viewport available in layer.context is the default viewport (non geospatial)
  2. Actual webmercator viewport is created after layer is initialized and after GLContext is created, at this point it uses canvas width and height.
  3. During render time, each viewport is activated using _activateViewport, where layer's context.viewport is updated, viewportChanged flag is set.
  4. Layer gets chance to update its state (provided shouldUpdateState is overwritten)

For hexagon layers, we do need to re-project points at #4 , so we have to check for viewportChanged flag. But this might trigger aggregation un-necessarily for each pan/zoom events. So we need a new flag that is only set when mercator projection needs to be updated.

@1chandu
Copy link
Contributor Author

1chandu commented Aug 16, 2018

Update the PR with possible solution (couple of tests failing, will fix before landing them)

We need to project points the first time viewport changed to a geospatial (WebMercatorViewport) viewport, since initial viewport is not a WebMercatorViewport. Added changes to aggregate only when layer has geospatial viewport and shouldUpdate returns only when viewportChanged and data is not aggregated yet.

@1chandu 1chandu changed the title HexagonLayer: Re-aggregate data when viewport is changed. HexagonLayer: Aggregate data using valid viewport. Aug 20, 2018
@1chandu
Copy link
Contributor Author

1chandu commented Aug 21, 2018

@ibgreen created #2215

@oller oller mentioned this pull request Aug 22, 2018
@1chandu 1chandu merged commit 0df8caf into master Aug 23, 2018
@1chandu 1chandu deleted the HexagonAggregationIssue branch August 23, 2018 18:13
1chandu added a commit that referenced this pull request Aug 23, 2018
1chandu added a commit that referenced this pull request Aug 23, 2018
* Revert "HexagonLayer: Aggregate data using valid viewport. (#2196)"
* Comment out unused import
1chandu added a commit that referenced this pull request Aug 27, 2018
1chandu added a commit that referenced this pull request Aug 31, 2018
1chandu added a commit that referenced this pull request Aug 31, 2018
* Reenable "HexagonLayer: Aggregate data using valid viewport (#2196)"
1chandu added a commit that referenced this pull request Aug 31, 2018
* Reenable "HexagonLayer: Aggregate data using valid viewport (#2196)"
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

2 participants