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

Tiles: options enhancements #1475

Merged
merged 1 commit into from Jun 16, 2021
Merged

Conversation

Avnerus
Copy link
Collaborator

@Avnerus Avnerus commented Jun 10, 2021

Updated options for tiles.
Here is the snippet from the revised documentation:

  • options.viewDistanceScale=1.0 (Number) - A scaling factor for tile refinement. A lower value would cause lower level tiles to load. Useful for debugging and for restricting resource usage.
  • options.updateTransforms=true (Boolean) - Always check if the tileset modelMatrix was updated. Set to false to improve performance when the tileset remains stationary in the scene.
  • options.loadOptions - loaders.gl options used when loading tiles from the tiling server. Includes fetch options such as authentication headers, worker options such as maxConcurrency, and options to other loaders such as 3d-tiles, gltf, and draco.
  • options.contentLoader = null (Promise) - An optional external async content loader for the tile. Once the promise resolves, a tile is regarded as READY to be displayed on the viewport.

Also fixed some missing examples in the website and upgraded dependencies.

@ibgreen
Copy link
Collaborator

ibgreen commented Jun 10, 2021

@Avnerus can you rebase this PR?
@belom88 Can you help @Avnerus land this PR?

@Avnerus Avnerus force-pushed the tiles-options-enhancements branch from f1d7521 to 6ed82aa Compare June 11, 2021 13:29
@Avnerus
Copy link
Collaborator Author

Avnerus commented Jun 11, 2021

Now rebased

this.fetchOptions.headers = this.options.headers;
this.loadOptions.fetch = {
...this.loadOptions.fetch,
headers: this.options.headers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make a breaking change and drop the headers option as it is now covered by loadOptions.fetch? This class is really complex, with so many options. Could certainly be a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is fine by me!

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK I created an API cleanup section here: #1245 where we can track proposals for API changes.

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.

Since we are about to launch 3.0, a major release we do have a rare opportunity to make breaking changes, so if we have ideas for how to clean up the sprawling Tileset3D API now is a good time.

};

this.content = await load(contentUrl, loader, options);

if (this.tileset.options.contentLoader) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have an onTileLoad callback. Do we need to add another callback? If we do, can we follow the convention and call it onTileContentLoad or similar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not the same as the onTile* callbacks which are essentially events.
The content loader promise must resolve before a tile can set

      this.contentState = TILE_CONTENT_STATE.READY;
      this._onContentLoaded();

And only then those callbacks may be fired.
Without this solution there is no way to use async loaders such as the three.js GLTFLoader without experiencing a race condition in which a tile is selected before it is actually on the render graph.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Understood, thanks for clearing up the use case.

Would it make sense if we could declare it as onTileContentLoad(tile: Tile3D): Promise<void> and we wait for it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am a bit hesitant because usually on* callbacks are not something you would expect would block execution. But I defer to you on this one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I can see your point. Thoughts:

  • contentLoader risk being confusing in a framework that is specifically built around a concept of "loaders". I would have expected an option with that name to accept a loaders.gl loader.
  • Perhaps another name, ideally indicating some new naming convention?
  • We could also allow all the on... functions to return promises though this might lead to unintended synchronization for current users, if they are using async functions as callbacks.

When working with a big documented API the API audit tends to be a big part of the effort. This is why it is often best to make one PR for every addition, instead of one PR with 4 new props :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps tileProcessingHook or something of sort?
Understood about the heavy PRs. Will work to make those leaner in the future.

fetchOptions?: {[key: string]: any};
loadOptions?: {[key: string]: any};
updateTransforms?: boolean;
maxRequests?: number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could also supply a requestSchedulerOptions object. It would avoid creating one more mapping of options and the documentation could be unified.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But the only relevant request scheduler option is maxRequests.
This line actually slipped in here from the traversal PR which I still need to rebase.

@ibgreen ibgreen mentioned this pull request Jun 11, 2021
24 tasks
@belom88 belom88 merged commit 108a67f into visgl:master Jun 16, 2021
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