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 fixes and perf improvements #2143

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

jailln
Copy link
Contributor

@jailln jailln commented Jul 17, 2023

Some 3D tiles fixes:

Perf improvement:

  • Improve perfs for 3D tiles with bounding region by converting bounding regions to bounding spheres. Previously, bounding regions where converted to OBB and the AABB englobing the OBB was used for culling and SSE computation. Now, they are converted to bounding spheres and the sphere is used for culling and SSE computation. Depending on the cases it can be a bigger approximation than before but it is faster and I didn't see any visual discrepencies. I tested the perf differences on a 3D Tiles photogrammetry tileset (~500Go, 13732 tiles with only bounding regions) that I cannot share unfortunately. On the master, we spend most of our time initializing the bounding region:

before-PR-0

before-PR-1

After the PR, we spend most of the time in parsing and preparing for rendering:

after-PR-0
after-PR-1

I also tried to implement a conversion to threejs' OBB and to implement a culling algorithm on OBB (SSE computation for OBB is easy) but didn't succeed completely (can be seen on the following branch). Another way would have been to improve OBB.setFromExtent which is the culprit but I haven't found another way of implementing it yet. Propositions are welcome :)

BREAKING CHANGE: Remove region, box and sphere properties of C3DTBoundingVolume. They have been replaced by volume property which contains a THREE.Box3 (for box) or a THREE.Sphere (for sphere or region). Initial bounding volume type can be retrieved with the initialVolumeType property.

@jailln jailln requested a review from mgermerie July 17, 2023 16:08
Copy link
Contributor

@valentinMachado valentinMachado left a comment

Choose a reason for hiding this comment

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

This review does not treat the solution implemented (replacing C3DTBoundingVolume by sphere) and if this is actually working.. but is more about the code in itself.

What could I do to check if the solution is working ?

src/Core/3DTiles/C3DTBoundingVolume.js Outdated Show resolved Hide resolved
src/Core/3DTiles/C3DTBoundingVolume.js Show resolved Hide resolved
src/Core/3DTiles/C3DTBoundingVolume.js Outdated Show resolved Hide resolved
src/Core/3DTiles/C3DTBoundingVolume.js Show resolved Hide resolved
* Bounding Volume is a sphere.
* @property {String} initialVolumeType - the initial volume type to be able to dissociate spheres
* and regions if needed since both are converted to spheres (one of {@link C3DTilesBoundingVolumeTypes})
* @property {THREE.Box3|THREE.Sphere} volume - The 3D bounding volume created. Can be a THREE.Box3 for bounding volumes
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a poor pattern to store in a single variable different type of object (volume) then to use another variable (here initialVolumeType) to know how to manipulate volume.. (btw you could use instanceof instead of initialVolumeType)

you could create a wrapper abstract class called Volume implementing a method isVisible(camera, tileMatrixWorld) then specifying SphereVolume and BoxVolume (class extending Volume) the specific behavior.

you could also put two attributes in C3DTBoundingVolume instead of volume one called box and the other one sphere which is the solution I rather, since a C3DTBoundingVolume could have the both and it avoids to create an abstract class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last pattern was the previously used pattern. However, if we choose this one I would keep the initialVolumeType to be able to know if the sphere comes from a bounding sphere or from a bounding region, in case it may be useful for itowns' users in some way. The second pattern is also interesting, I actually thought about it when implementing this. I don't have time to implement it right now but I'll come back to it soon.

src/Core/3DTiles/C3DTBoundingVolume.js Show resolved Hide resolved
src/Core/3DTiles/C3DTBoundingVolume.js Outdated Show resolved Hide resolved
src/Core/3DTiles/C3DTileset.js Show resolved Hide resolved
src/Core/3DTiles/C3DTileset.js Outdated Show resolved Hide resolved
@jailln
Copy link
Contributor Author

jailln commented Sep 14, 2023

Hi @valentinMachado , thanks for your review and sorry for the late response.

What could I do to check if the solution is working ?

You can use a 3D Tiles dataset with bounding volumes of region type to test if it works. The only open source one and available online that I know is this one.

src/Core/3DTiles/C3DTBoundingVolume.js Outdated Show resolved Hide resolved
src/Core/3DTiles/C3DTBoundingVolume.js Show resolved Hide resolved
src/Core/3DTiles/C3DTBoundingVolume.js Show resolved Hide resolved
src/Core/3DTiles/C3DTBoundingVolume.js Show resolved Hide resolved
src/Process/3dTilesProcessing.js Show resolved Hide resolved
utils/debug/3dTilesDebug.js Outdated Show resolved Hide resolved
@jailln jailln force-pushed the fix/GEO-18291-3dtiles-mel branch from d9bc50b to 564f61d Compare October 25, 2023 10:36
@jailln jailln requested a review from mgermerie October 25, 2023 10:42
src/Core/3DTiles/C3DTBoundingVolume.js Show resolved Hide resolved
src/Process/3dTilesProcessing.js Show resolved Hide resolved
@mgermerie
Copy link
Contributor

Except from the conflict that should be easy to fix it's all good for me. Thanks !

fix(3dtiles): Fix a bug in 3d tiles bounding spheres subdivisions
fix(3dtiles): Fix 3D tiles debug bounding spheres

BREAKING CHANGE: Remove region, box and sphere properties of C3DTBoundingVolume.
They have been replaced by volume property which contains a THREE.Box3 (for
box) or a THREE.Sphere (for sphere or region). Initial bounding volume type
can be retrieved with the initialVolumeType property.
@jailln jailln force-pushed the fix/GEO-18291-3dtiles-mel branch from 564f61d to f888d36 Compare November 27, 2023 15:32
@jailln jailln merged commit f0eaf96 into iTowns:master Nov 27, 2023
9 checks passed
@jailln jailln deleted the fix/GEO-18291-3dtiles-mel branch November 27, 2023 15:58
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.

3D Tiles debug tools: bounding spheres are not displayed Unstable 3d tiles rendering in itowns
4 participants