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

3d tiles migration clean #2365

Merged
merged 6 commits into from
Jul 19, 2024

Conversation

jailln
Copy link
Contributor

@jailln jailln commented Jul 19, 2024

  • clean / document OGC3DTilesLayer
  • share cache and queues amongst 3D tiles layers
  • Propagate 3d-tiles-renderer events through OGC3DtilesLayer
  • Attaching / dettaching layer
  • Register supported 3D tiles extensions

@jailln jailln requested a review from Desplandis July 19, 2024 10:56
Copy link
Contributor

@Desplandis Desplandis left a comment

Choose a reason for hiding this comment

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

Sounds good to me, just have some suggestions. We'll have to wait for a new release on 3d-tiles-renderer. =)

@@ -10,6 +10,50 @@ import ReferLayerProperties from 'Layer/ReferencingLayerProperties';
// Internal instance of GLTFLoader, passed to 3d-tiles-renderer-js to support GLTF 1.0 and 2.0
// Temporary exported to be used in deprecated B3dmParser
export const itownsGLTFLoader = new iGLTFLoader();
// TODO: waiting for https://github.com/NASA-AMMOS/3DTilesRendererJS/pull/626 to be merged to enable this extension
Copy link
Contributor

Choose a reason for hiding this comment

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

Merged, waiting for a release.

Suggested change
// TODO: waiting for https://github.com/NASA-AMMOS/3DTilesRendererJS/pull/626 to be merged to enable this extension

* @event OGC3DTilesLayer#load-model
* @type {Object}
* @property {THREE.Group} scene - the model (tile content) parsed in a THREE.GROUP
* @property {Object} tile - the tile metadata from the tileset
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @property {Object} tile - the tile metadata from the tileset
* @property {Tile} tile - the tile metadata from the tileset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no Tile class, I sticked with Object

* @event OGC3DTilesLayer#dispose-model
* @type {Object}
* @property {THREE.Group} scene - the model (tile content) that is disposed
* @property {Object} tile - the tile metadata from the tileset
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @property {Object} tile - the tile metadata from the tileset
* @property {Tile} tile - the tile metadata from the tileset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here

* @event OGC3DTilesLayer#tile-visibility-change
* @type {Object}
* @property {THREE.Group} scene - the model (tile content) parsed in a THREE.GROUP
* @property {Object} tile - the tile metadata from the tileset
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @property {Object} tile - the tile metadata from the tileset
* @property {Tile} tile - the tile metadata from the tileset

* @type {Object}
* @property {THREE.Group} scene - the model (tile content) parsed in a THREE.GROUP
* @property {Object} tile - the tile metadata from the tileset
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing parameter in event.

Suggested change
*/
* @property {boolean} visible - the tile visible state
*/

Comment on lines +154 to +170
_setupCacheAndQueues() {
if (lruCache === null) {
lruCache = this.tilesRenderer.lruCache;
} else {
this.tilesRenderer.lruCache = lruCache;
}
if (downloadQueue === null) {
downloadQueue = this.tilesRenderer.downloadQueue;
} else {
this.tilesRenderer.downloadQueue = downloadQueue;
}
if (parseQueue === null) {
parseQueue = this.tilesRenderer.parseQueue;
} else {
this.tilesRenderer.parseQueue = parseQueue;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion. You could add lruCache, downloadQueue and parseQueue in a single object and only do a single null check?

@jailln jailln merged commit 6e0d5ef into iTowns:3d-tiles-migration Jul 19, 2024
1 check passed
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.

2 participants