3d tiles height reference #11604

ggetz · 2023-11-03T19:57:07Z

To be merged after #11581

Building on this previous PR, this allows more user-friendly APIs for clamping to 3D Tiles as well as or instead of terrain by extending the HeightReference types:

CLAMP_TO_GROUND/RELATIVE_TO_GROUND now clamps to or is relative to terrain or 3D Tiles, whichever is higher
Added CLAMP_TO_TERRAIN/RELATIVE_TO_TERRAIN to clamp to or position relative to terrain only
Added CLAMP_TO_3D_TILE/RELATIVE_TO_3D_TILE to clamp to or position relative to terrain only

This was to align to the possible ClassificationType values, TERRAIN, CESIUM_3D_TILE, or BOTH.

I've replaced the sandcastle for the (now outdated) scene.clampToHeight method of clamping, which is now Clamp Model to Ground. I've also extended this and the Clamp Entities to Ground sandcastle a bit to showcase available options.

Testing Plan:

Make sure all options are working on the new sandcastles, "Clamp Model to Ground" and "Clamp Entities to Ground"
Confirm data sources with clampToGround work on 3D Tiles
Confirm nothing crashes when switching between 2D/3D/CV

TODO:

Unit tests
Update CHANGES.md
Confirm 2D/CV mode
Sandcastle thumbnails

cesium-concierge · 2023-11-03T20:00:16Z

Thanks for the pull request @ggetz!

❕ There was an error checking the CLA! If this is your first contribution, please send in a Contributor License Agreement.
- Maintainers, this was the error I ran into while attempting to process the CLA check. Please resolve it to continue CLA checking.
- You'll need to manually check for a submitted CLA
```
Error: The service is currently unavailable.
```
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.
❔ Unit tests were not updated.
- Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.

…hicsInc/cesium into 3d-tiles-height-reference

ptrgags · 2023-12-22T21:01:29Z

In #11655 it was pointed out that this PR might need to account for vertical exaggeration, see #11655 (comment)

ggetz · 2024-01-12T14:29:03Z

@jjspace Would you mind taking a pass on this PR? The 3D-specific work was done in #11581. This mostly wires up a new API for clamping entities to 3D Tiles, and updates or adds Sandcastles showing the functionality.

jjspace · 2024-01-12T21:11:19Z

For some reason the "Clamp Entities to Ground" example on the top of the mountain runs at 1-2 fps and even locks up the editor side. I only notice it in that example so far

jjspace

Had a couple comments in addition to the performance issue above.

I only got about halfway through the code and will finish up the rest on monday but wanted to leave these so you can start on them

CHANGES.md

packages/engine/Source/Scene/HeightReference.js

packages/engine/Source/DataSources/ModelVisualizer.js

ggetz · 2024-01-16T19:09:53Z

For some reason the "Clamp Entities to Ground" example on the top of the mountain runs at 1-2 fps and even locks up the editor side. I only notice it in that example so far

@jjspace This should be addressed. The root of the issue was a missing flag due to a bad merge. Additionally, I further optimized the 3D Tiles picking function by sorting the bounding sphere before reading any mesh data.

jjspace

I can confirm that the horrible framerate on the entities sandcastle seems fixed. I reviewed the rest of the code and think it all looks good apart from a few typos I spotted. I would appreciate a second glance through this though as I'm still newer to this area of the code.

I did re-run through the testing plan though and found an issue when changing view types. When switching between 2D and CV there's a "normalized result is not a number" error getting thrown. It doesn't happen when going from 3D to CV, only 2D -> CV

simplescreenrecorder-2024-01-16_17.17.02.mp4

packages/engine/Source/Scene/Cesium3DTileset.js

packages/engine/Source/Scene/Scene.js

Co-authored-by: Josh <[email protected]>

ggetz · 2024-01-17T15:34:29Z

I did re-run through the testing plan though and found an issue when changing view types. When switching between 2D and CV there's a "normalized result is not a number" error getting thrown. It doesn't happen when going from 3D to CV, only 2D -> CV

Good catch @jjspace. This is because an existing issue in the camera is causing the camera height to be set to NaN. That's outside of the scope of this PR, but we can apply a common workaround by temporarily skipping this scope while the scene is morphing.

Thanks for your review @jjspace!

@ptrgags Would you mind doing a final pass and merge if everything is good?

ptrgags · 2024-01-17T18:23:16Z

@ggetz I'm trying out the Sandcastles first before I look at the code. I'm noticing a couple things in the Clamp Entities to Ground Sandcastle:

When I switch the first dropdown from 3D Tiles -> Terrain, the drawn entity disappears. When I adjust the second dropdown, then it works again.
(a little tricky to reproduce) When I switch to Terrain, then select something else in the second dropdown (e.g. Draw Polyline), then slowly cycle through the dropdown options quickly (I was using the up arrow key), when I get back to "Draw Point with Label", the text doesn't line up with the bilboard, but then when I zoom out it lines up again. I'm guessing it's a race condition due to swapping assets in/out of the scene while they are still partially loaded?

.

ptrgags · 2024-01-17T19:14:14Z

I also tried using Terrain Exaggeration (Sandcastle)

It works, though the scene is very slow until everything loads.

ptrgags

@ggetz I had some minor comments and questions, but overall this is looking good

Apps/Sandcastle/gallery/Clamp Model to Ground.html

Apps/Sandcastle/gallery/development/Clamp to Ground Modes.html

packages/engine/Source/Scene/HeightReference.js

ptrgags · 2024-01-17T18:51:54Z

packages/engine/Specs/Scene/Model/ModelSpec.js

      });

-      it("changing height reference works", function () {
-        return loadAndZoomToModelAsync(
+      it("changing height reference works", async function () {


This test and another one in this file is failing locally:

Hm, I'm not able to reproduce this locally in the command line or in the browser.

packages/engine/Source/Scene/Model/Model.js

packages/engine/Source/Scene/Cesium3DTileset.js

packages/engine/Source/Scene/Scene.js

Co-authored-by: Peter Gagliardi <[email protected]>

…hicsInc/cesium into 3d-tiles-height-reference

ggetz · 2024-01-19T17:05:22Z

Thanks @ptrgags! Your comments should be addressed.

(a little tricky to reproduce) When I switch to Terrain, then select something else in the second dropdown (e.g. Draw Polyline), then slowly cycle through the dropdown options quickly (I was using the up arrow key), when I get back to "Draw Point with Label", the text doesn't line up with the bilboard, but then when I zoom out it lines up again. I'm guessing it's a race condition due to swapping assets in/out of the scene while they are still partially loaded?

From what I can tell, this is an existing issue with clamping to terrain and would be outside of the scope of this PR

ggetz · 2024-01-22T21:13:35Z

@jjspace I believe all the feedback here has been addressed. OK to merge?

ggetz · 2024-01-29T14:18:29Z

@jjspace Bump?

jjspace · 2024-01-29T19:53:29Z

@ggetz It mostly looks good, I did spot a small issue in the sandcastles

The Clamp Entities sandcastle isn't loading the google tiles correctly it seems. This then prevents switching between terrain and 3d tiles in the viewer.

Actually it seems like something else weird is going on here and all sandcastles seem to be running twice every time...? note the double options

I'm observing this on main too so maybe something on my end?

ggetz · 2024-01-29T20:09:53Z

@jjspace In this example?

ggetz · 2024-01-29T20:12:27Z

I'm also not seeing the double options.

jjspace · 2024-01-29T20:58:08Z

@ggetz it seems like the double run might be a problem with my extensions because running in the chrome guest profile doesn't make it happen. I have no idea why any of them would cause that but I'll have to investigate later. interestingly it only happens locally and in that ci link, not the deployed version of sandcastle...
(Edit: super weird but it seems to be a Bitwarden issue, will try and follow up with them)

I am seeing some test errors when running locally though. Not sure why CI didn't catch them.

I confirmed I see the fog one in main too so that's not related to this branch but the height reference one is only on this branch.
The tests also ran into that "error in afterAll" error after switching branches until I ran npm i again, not sure what that's about but will have to keep an eye on it

ggetz · 2024-01-31T15:21:26Z

Thanks @jjspace, that should fix the failing spec. I'll make sure an issue is open for the others, but there's no need for those to hold up this PR.

jjspace · 2024-01-31T23:03:25Z

Confirmed that test no longer fails on my system, thanks

Add height references for 3D tiles

8e271f1

Merge branch 'tileset-get-height' into 3d-tiles-height-reference

acf76ce

lilleyse mentioned this pull request Nov 30, 2023

Vertical exaggeration for 3D Tiles #11655

Merged

3 tasks

ggetz added 6 commits December 7, 2023 11:02

Merge branch 'tileset-get-height' into 3d-tiles-height-reference

38b6699

Tweak API, fix unit tests

3f35d9a

Merge branch 'tileset-get-height' into 3d-tiles-height-reference

4b1aaff

Update Sandcastle

4308e10

Update Sandcastle

6d7f114

Merge branch '3d-tiles-height-reference' of github.com:AnalyticalGrap…

3e7adc9

…hicsInc/cesium into 3d-tiles-height-reference

ggetz mentioned this pull request Dec 15, 2023

Erratic clampToHeight and heightReference behavior when toggling tileset on/off #9680

Closed

ggetz added 3 commits January 4, 2024 13:46

Merge branch 'tileset-get-height' into 3d-tiles-height-reference

8c645fd

Update model specs

36f3e7a

Error -> DevloperError

4fd8c5b

ggetz marked this pull request as ready for review January 4, 2024 20:31

ggetz mentioned this pull request Jan 9, 2024

3D Tileset getHeight, and keeping camera above 3D Tiles #11581

Merged

5 tasks

Merge branch 'main' into 3d-tiles-height-reference

20fd2be

jjspace reviewed Jan 12, 2024

View reviewed changes

Fix performance issue

fe87380

PR feedback

d0c2721

jjspace reviewed Jan 16, 2024

View reviewed changes

ggetz and others added 4 commits January 17, 2024 09:57

Update packages/engine/Source/Scene/Cesium3DTileset.js

9b95eac

Co-authored-by: Josh <[email protected]>

Update packages/engine/Source/Scene/Scene.js

0d5607f

Co-authored-by: Josh <[email protected]>

Fix comments

9591d82

Fix crash when transitioning

bcb793a

ggetz mentioned this pull request Jan 17, 2024

Add support for clamping Vector 3D Tile point features to Terrain or 3d Tiles #11710

Open

7 tasks

ptrgags reviewed Jan 17, 2024

View reviewed changes

ggetz and others added 3 commits January 17, 2024 15:07

Update packages/engine/Source/Scene/Cesium3DTileset.js

d489a76

Co-authored-by: Peter Gagliardi <[email protected]>

Cleanup

0f819cf

Merge branch '3d-tiles-height-reference' of github.com:AnalyticalGrap…

cb0ec02

…hicsInc/cesium into 3d-tiles-height-reference

Merge branch 'main' into 3d-tiles-height-reference

11251dd

ggetz mentioned this pull request Jan 19, 2024

clampToGround works poorly with Google Photorealistic 3D Tiles #11775

Closed

Merge branch 'main' into 3d-tiles-height-reference

dd3f773

ggetz mentioned this pull request Jan 29, 2024

Fix #11787 – Improve scene height picking performance #11792

Merged

7 tasks

ggetz added the priority - next release label Jan 31, 2024

Fix spec

6b5d19a

jjspace merged commit 8273efc into main Jan 31, 2024
9 checks passed

jjspace deleted the 3d-tiles-height-reference branch January 31, 2024 23:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3d tiles height reference #11604

3d tiles height reference #11604

ggetz commented Nov 3, 2023 •

edited

Loading

cesium-concierge commented Nov 3, 2023

ptrgags commented Dec 22, 2023

ggetz commented Jan 12, 2024

jjspace commented Jan 12, 2024

jjspace left a comment

ggetz commented Jan 16, 2024

jjspace left a comment

ggetz commented Jan 17, 2024

ptrgags commented Jan 17, 2024

ptrgags commented Jan 17, 2024

ptrgags left a comment

ptrgags Jan 17, 2024

ggetz Jan 18, 2024

ggetz commented Jan 19, 2024

ggetz commented Jan 22, 2024

ggetz commented Jan 29, 2024

jjspace commented Jan 29, 2024

ggetz commented Jan 29, 2024

ggetz commented Jan 29, 2024

jjspace commented Jan 29, 2024 •

edited

Loading

ggetz commented Jan 31, 2024 •

edited

Loading

jjspace commented Jan 31, 2024

3d tiles height reference #11604

3d tiles height reference #11604

Conversation

ggetz commented Nov 3, 2023 • edited Loading

cesium-concierge commented Nov 3, 2023

ptrgags commented Dec 22, 2023

ggetz commented Jan 12, 2024

jjspace commented Jan 12, 2024

jjspace left a comment

Choose a reason for hiding this comment

ggetz commented Jan 16, 2024

jjspace left a comment

Choose a reason for hiding this comment

ggetz commented Jan 17, 2024

ptrgags commented Jan 17, 2024

ptrgags commented Jan 17, 2024

ptrgags left a comment

Choose a reason for hiding this comment

ptrgags Jan 17, 2024

Choose a reason for hiding this comment

ggetz Jan 18, 2024

Choose a reason for hiding this comment

ggetz commented Jan 19, 2024

ggetz commented Jan 22, 2024

ggetz commented Jan 29, 2024

jjspace commented Jan 29, 2024

ggetz commented Jan 29, 2024

ggetz commented Jan 29, 2024

jjspace commented Jan 29, 2024 • edited Loading

ggetz commented Jan 31, 2024 • edited Loading

jjspace commented Jan 31, 2024

ggetz commented Nov 3, 2023 •

edited

Loading

jjspace commented Jan 29, 2024 •

edited

Loading

ggetz commented Jan 31, 2024 •

edited

Loading