cesium icon indicating copy to clipboard operation
cesium copied to clipboard

Adjust defaults for 3D Tiles collisions

Open ggetz opened this issue 2 years ago • 1 comments

Description

Given feedback about 3D Tiles camera collision performance and confusion around the API flag, this PR:

  1. Adjusts the default for tilesets to have collisions disabled rather than enabled
  2. Renames the relevant property from disableCollision to enableCollision
  3. For Google P3DT, which has been tested for sufficient performance, sets enableCollision to true by default.

The reason for making this the new default is that, after investigation, I'm simply not confident we'll be able to ensure performance for every tileset case, particularly for tilesets containing large tiles with few primitives and many vertices.

We still will address https://github.com/CesiumGS/cesium/issues/11824 as well as potential performance improvements, but I just wanted to get on the same page about the API beforehand to reduce churn.

Issue number and link

Closes https://github.com/CesiumGS/cesium/issues/11811

Testing plan

  • [ ] Verify the Google P3DT examples still have the same behavior with camera collisions as main
  • [ ] Ensure the "Clamp Models to Ground" and "Clamp Entities to Ground" examples have the same behavior as main
  • [ ] Ensure other tilesets, especially OSM Buildings and the "3D Tiles Interior" example, have collisions disabled by default and the expected performance using
viewer.scene.debugShowFramesPerSecond = true;

Author checklist

  • [x] I have submitted a Contributor License Agreement
  • [x] I have added my name to CONTRIBUTORS.md
  • [x] I have updated CHANGES.md with a short summary of my change
  • [x] I have added or updated unit tests to ensure consistent code coverage
  • [x] I have update the inline documentation, and included code examples where relevant
  • [x] I have performed a self-review of my code
  • [ ] Open issue for deprecation

ggetz avatar Feb 14 '24 16:02 ggetz

@jjspace Can you take an initial pass on this?

ggetz avatar Feb 14 '24 16:02 ggetz

Bump @jjspace

ggetz avatar Feb 19 '24 15:02 ggetz

Before we merge, @mramato would you mind doing a quick pass on the API here and advise if it's workable in Cesium ion?

ggetz avatar Feb 19 '24 19:02 ggetz

@ggetz Thrilled to see this change being made. At far as defaults go, I only have one thought: Since we're renaming, would enableCameraCollision be better/more specific than enableCollision? Are there any uses for this outside of camera movement?

It's outside the scope of this PR, but in addition to performance, the other requested behavior from the ion side would be to make the camera behave the same way it does with terrain; which unless something has changed means: "The camera can go insight/under a tileset if it's loading, but the moment the full LOD is loaded it will "pop" to be on top of the tileset. The reasons for this are

  1. It's hard to prevent the camera from going under during the load process anyway, so better to just allow it until the full detail is loaded for the current location.
  2. Saving/restoring camera views is really hard if you don't factor in "is the data still loading" before preventing the camera from going somewhere.
  3. Similar to 2, camera flights are really hard to do otherwise because the camera usually slams into a lower LOD and stops and you don't end up with the actually desired location for the flight.

I can write up an additional issue for the above collision feedback if you want.

mramato avatar Feb 19 '24 21:02 mramato

Thanks @mramato!

We went with enableCollision rather than enableCameraCollision as the same flag is need to enable clamping to 3D Tiles.

Besides the performance itself, I believe your additional requests for camera collision are covered by https://github.com/CesiumGS/cesium/pull/11837, which is a follow up PR to this one. If you want to test out the behavior in that PR and leave any additional feedback, we'd be happy to incorporate it there.

ggetz avatar Feb 19 '24 21:02 ggetz

Awesome, thanks @ggetz. Looking forward to given all changes a spin when they are ready.

mramato avatar Feb 19 '24 22:02 mramato

Thanks all! @jjspace I just merged in main. Please merge when happy.

ggetz avatar Feb 20 '24 13:02 ggetz

Sorry for forgetting this yesterday, good to go, thanks!

jjspace avatar Feb 21 '24 17:02 jjspace