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

TileLayer improvements #4139

Merged
merged 2 commits into from
Feb 4, 2020
Merged

TileLayer improvements #4139

merged 2 commits into from
Feb 4, 2020

Conversation

Pessimistress
Copy link
Collaborator

Resolves #3511

This PR refactors BaseTileLayer/TileCache classes to function more similar to Tile3DLayer/Tileset3D. Traversal now reruns every time any tile finishes loading, instead of waiting for the entire viewport to load. The logic that determines tile visibilities is entirely moved into TileCache. It also uses a more sophisticated strategy (configurable by the user) to pick placeholders when a tile is pending.

The new API needs audit.

Change List

  • Add strategy prop to BaseTileLayer (see documentation)
  • TileCache rewrite
  • Unit tests

@coveralls
Copy link

coveralls commented Jan 12, 2020

Coverage Status

Coverage decreased (-2.4%) to 80.872% when pulling 720f23d on x/tile-strategy into 49aac07 on master.

@Pessimistress Pessimistress marked this pull request as ready for review January 16, 2020 21:58
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.

Got interrupted, will add more comments soon...

docs/layers/base-tile-layer.md Outdated Show resolved Hide resolved
docs/layers/base-tile-layer.md Outdated Show resolved Hide resolved
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 feel that having classes named "Base..." should be avoided if at all possible. To me exposing abstract base classes is very technical, and associated with object oriented programming practices that are currently out-of-style (particularly in the JS community).

My preference would be to avoid the Base name and call this TileLayer instead of BaseTileLayer. Assuming that creates an unacceptable naming collisions (a layer with the same name in two different modules), happy to work on finding a more suggestive name.

I think this should still be in geo-layers - just because we make layers work with non-geospatial coordinate systems does not mean they automatically get moved out of geo-layers, at least this should not be in the core layers module.

If the 2D loader is getting this complicated, I am starting to feel it could make sense to put the TileCache code (which I want to call Tileset2D) in loaders.gl. We can expand the 3d tiles category to tiles handle both 2D and 3D tiles...

There seems to be a lot of overlap in logic, support classes, testing logic etc.

As one example, we are going to need to add support for the RequestScheduler from loaders.gl to this class as well, so there could be synergies by having this in loaders.

We just need to find a sub-module structure that makes sense to everyone.

modules/layers/src/base-tile-layer/base-tile-layer.js Outdated Show resolved Hide resolved
| | | | | | |
+-----------+ +-----+ +-----+-----+

show cached children tiles when parent is loading
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two way reuse, both up and down... This is not used by 3d tiles. Does this complicate things much?

Copy link
Collaborator Author

@Pessimistress Pessimistress Jan 27, 2020

Choose a reason for hiding this comment

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

3d-tiles loads everything from the root to the current level, so the parent is always available when you zoom out. 2d only loads tiles at the current level. When you zoom out and no ancestors are available, the map will flash white and you lose all visual reference. Yes it adds complexity, but it's also needed for usability.

modules/layers/src/base-tile-layer/utils/tile-cache.js Outdated Show resolved Hide resolved
@Pessimistress
Copy link
Collaborator Author

Regarding the BaseTileLayer class: we need an API audit before 8.1 release for sure. If we are not moving it into the core layers, then there will be only one TileLayer as before, just exposing a few new props to enable the new use cases.

I'm ok with this arrangement for now, given this non-geospatial support is new. In the long term, do we want to investigate non-geospatial support for 3d tiles (e.g. Potree is technically not bound to geospatial)? If so, we may want to move these two layers to a new module @deck.gl/tile-layers and drop any specific format dependency?

docs/layers/base-tile-layer.md Outdated Show resolved Hide resolved
@Pessimistress Pessimistress merged commit ff6f9ff into master Feb 4, 2020
@Pessimistress Pessimistress deleted the x/tile-strategy branch February 4, 2020 18:38
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.

TileLayer: multiple zoom level tiles rendered at the same time
3 participants