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: fix a bug and improve perfs #2266

Merged
merged 2 commits into from
Feb 14, 2024
Merged

Conversation

jailln
Copy link
Contributor

@jailln jailln commented Feb 8, 2024

Description

This PR introduces two changes:

  • Fixes BatchID in 3DTiles are float and negative #2074 (that also caused tiles to be removed and never loaded again when removed from the cache): the issue came from accessing threejs buffer attributes array directly instead of using the recommended getX() method (which is not the same in the case of InterleavedBufferAttribute.

  • Greatly improves 3D tiles perfs (see screenshots below) by reducing loading time overhead due to internal structures pre-filling (mainly for 3D tiles picking and style) -> part of these structures are now filled on first demand.

Screenshots

Performance profiling on 3dtiles_ion.html example before the second commit:

before-PR-getInfoById

Performance profiling on 3dtiles_ion.html example after the second commit:

afterPR-getInfoById

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.

Nice PR! I added some comments about a few odd things but they are minor. =)

Nice perf gain BTW.

src/Core/3DTiles/utils/3DTilesUtils.js Outdated Show resolved Hide resolved
src/Core/3DTiles/C3DTFeature.js Outdated Show resolved Hide resolved
src/Core/3DTiles/C3DTFeature.js Outdated Show resolved Hide resolved
src/Core/3DTiles/C3DTFeature.js Show resolved Hide resolved
src/Layer/C3DTilesLayer.js Show resolved Hide resolved
src/Layer/C3DTilesLayer.js Show resolved Hide resolved
@jailln jailln requested a review from Desplandis February 12, 2024 16:41
@jailln jailln force-pushed the fix/2074-3dtiles branch 3 times, most recently from c51b51c to 5c424d0 Compare February 12, 2024 21:13
…re-filling

BREAKING CHANGES:
* C3DTFeature constructor parameters changed from
(tileId, batchId, groups, info, userData, object3d) to
(tileId, batchId, groups, userData, object3d)
* C3DTilesLayer.findBatchTable() is not exposed in the API anymore
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.

Previous comments were all resolved. Ready to merge!

@jailln jailln merged commit 5d2f384 into iTowns:master Feb 14, 2024
9 checks passed
@jailln jailln deleted the fix/2074-3dtiles branch February 14, 2024 13:02
@jailln jailln mentioned this pull request Aug 8, 2024
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.

BatchID in 3DTiles are float and negative
2 participants