-
Notifications
You must be signed in to change notification settings - Fork 191
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,8 +66,12 @@ export type Tileset3DProps = { | |
attributions?: string[]; | ||
headers?: any; | ||
loadTiles?: boolean; | ||
fetchOptions?: {[key: string]: any}; | ||
loadOptions?: {[key: string]: any}; | ||
updateTransforms?: boolean; | ||
maxRequests?: number; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could also supply a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But the only relevant request scheduler option is |
||
viewDistanceScale?: number; | ||
basePath?: string; | ||
contentLoader?: (tile: Tile3D) => Promise<void>; | ||
}; | ||
|
||
type Props = { | ||
|
@@ -85,8 +89,11 @@ type Props = { | |
attributions: string[]; | ||
headers: any; | ||
loadTiles: boolean; | ||
fetchOptions: {[key: string]: any}; | ||
loadOptions: {[key: string]: any}; | ||
updateTransforms: boolean; | ||
viewDistanceScale: number; | ||
basePath: string; | ||
contentLoader?: (tile: Tile3D) => Promise<void>; | ||
i3s: {[key: string]: any}; | ||
}; | ||
|
||
|
@@ -108,15 +115,22 @@ const DEFAULT_PROPS: Props = { | |
onTileUnload: (tile) => {}, | ||
onTileError: (tile, message, url) => {}, | ||
|
||
// Optional async tile content loader | ||
contentLoader: undefined, | ||
|
||
// View distance scale modifier | ||
viewDistanceScale: 1.0, | ||
|
||
// TODO CESIUM | ||
// The maximum screen space error used to drive level of detail refinement. | ||
maximumScreenSpaceError: 8, | ||
|
||
loadTiles: true, | ||
updateTransforms: true, | ||
viewportTraversersMap: null, | ||
|
||
headers: {}, | ||
fetchOptions: {}, | ||
loadOptions: {}, | ||
|
||
token: '', | ||
attributions: [], | ||
|
@@ -140,7 +154,7 @@ const TILES_GPU_MEMORY = 'Tile Memory Use'; | |
export default class Tileset3D { | ||
// props: Tileset3DProps; | ||
options: Props; | ||
fetchOptions: {[key: string]: any}; | ||
loadOptions: {[key: string]: any}; | ||
|
||
type: string; | ||
tileset: {[key: string]: any}; | ||
|
@@ -238,12 +252,18 @@ export default class Tileset3D { | |
this.refine = json.root.refine; | ||
|
||
// TODO add to loader context? | ||
this.fetchOptions = this.options.fetchOptions || {}; | ||
this.loadOptions = this.options.loadOptions || {}; | ||
if (this.options.headers) { | ||
this.fetchOptions.headers = this.options.headers; | ||
this.loadOptions.fetch = { | ||
...this.loadOptions.fetch, | ||
headers: this.options.headers | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we make a breaking change and drop the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is fine by me! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
}; | ||
} | ||
if (this.options.token) { | ||
this.fetchOptions.token = this.options.token; | ||
this.loadOptions.fetch = { | ||
...this.loadOptions.fetch, | ||
token: this.options.token | ||
}; | ||
} | ||
|
||
this.root = null; | ||
|
There was a problem hiding this comment.
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 itonTileContentLoad
or similar?There was a problem hiding this comment.
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
And only then those callbacks may be fired.
Without this solution there is no way to use
async
loaders such as the three.jsGLTFLoader
without experiencing a race condition in which a tile is selected before it is actually on the render graph.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.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 :)
There was a problem hiding this comment.
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.