Fix camera collision on tileset load

[ggetz]

Description

Merge #11829 first!

This ensures the scene is subscribed to tileset height updates, ie. when a tile loads. The subscription requires the current position, so it should only check for updates when a tile above or below the provided position loads in, and we reset the subscription when the camera position changes.

This is a similar workflow to how clamp to ground works, which is why these functions already exist.

Issue number and link

Fixes #11824

Testing plan

1. Go to the Google 3D Tiles Sandcastle
2. Use the geocoder to navigate to the Grand Canyon
3. Quickly move the camera down and underground before the view is stable. The camera should no longer ever get "stuck" under terrain as it does in main.
4. Ensure consistant FPS with Google P3DT
5. Repeat the above steps with CWT

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have update the inline documentation, and included code examples where relevant
• I have performed a self-review of my code
• Adjust defaults for 3D Tiles collisions #11829 is merged

[ggetz]

@jjspace Can you please take a look at this once #11829 is merged?

[jjspace]

@ggetz It's tricky but I was still able to get the camera stuck below the google 3d tiles around the Grand Canyon even when they were still loading.

I didn't find a 100% reproducible set of steps but these seem to produce a higher chance

• Rotate the camera with middle click fast
• Sliding in on the side of a mountain/cliff seems to increase the likelyhood
• It seems more likely when the mouse crosses the horizon line to the sky mid-drag. like the border that changes left click between move and rotate (which is why it's then harder to get un-stuck)
• I had the most success by positioning the camera right above a cliff, middle clicking halfway across the canyon then flipping fast so the camera was in the opposite side

simplescreenrecorder-2024-02-21_12.12.33.mp4

[ggetz]

@jjspace I believe this is actually par with the behavior of existing terrain collision. This is a fairly reproducible way I've found of reproducing:

1. Navigate to the Grand Canyon using the geocoder
2. Orient the camera to be at the bottom of the canyon, and drag it along the ground a bit
3. Turn off the tileset or globe
4. Swing the camera into an area of higher elevation
5. Turn the globe or tileset back on

In either case, you will find the camera is underground until the camera is moved. Though, this is more obvious with Google P3DT due to back face culling and skirts.

CWT Sandcastle (1.114 deployed)

3D Tiles (current branch)

I think the case here is that no new tiles are actually being loading in in this case, meaning there's no event to trigger the height update.

Something I want to highlight here is that the globe height is updating via the same mechanisms for both terrain tiles and 3D Tiles. Any difference here I believe is coming down to differences in the mesh construction itself between CWT and Google P3DT.

[ggetz]

Bump @jjspace

[jjspace]

Just a small comment for some code clarification. I had observed the desired behavior snapping the camera outside the special case discussed above. Please open an issue if that should be considered a bug not addressed by this fix

[jjspace]

This line seems confusing from a types perspective. Especially when paired with the line below where updateHeight sets it to a function.
Also I'm a little confused why we're setting the value here and then setting it again below? Is it used in the getGlobeHeight function? or is this trying to call it if it exists and set it below regardless?

[ggetz]

What is is doing is that, if this._removeUpdateHeightCallback is a defined function, execute the function (therefor removing the callback) and reassign the value to false.

It's removing the old listener (if it exists) before creating a new listener. This is a code pattern which is used commonly in the project.

[jjspace]

So it's equivalent to this?

if (defined(this._removeUpdateHeightCallback)) {
  this._removeUpdateHeightCallback();
  this._removeUpdateHeightCallback = false;
} else {
  this._removeUpdateHeightCallback = false;
}

// or more concise

if (defined(this._removeUpdateHeightCallback)) {
  this._removeUpdateHeightCallback();
}
this._removeUpdateHeightCallback = false;

1. We normally avoid trying to do truthy/falsey checks, why not use defined() in the first &&?
2. Why assign to false as a boolean when it's also a function or undefined? Even if it's a common pattern this seems like asking for issues? Is false ever actually checked for? (I didn't see it anywhere)
   • Would it be better to do delete this._removeUpdateHeightCallback?

[ggetz]

I'll cede that this isn't the best pattern. I've updated it to be type consistant and more readable.

[ggetz]

I had observed the desired behavior snapping the camera outside the special case discussed above. Please open an issue if that should be considered a bug not addressed by this fix

I did confirm that loading new tiles was not actually being accounted for in the existing collision code. I made a small tweak here to account for that– both with 3D Tiles and terrain.

Likely the reason it was not enabled in the previous code was because it's possible to pop the camera up really far while lower-level tiles are being loaded. Here, I accounted for that by ensuring we only adjust the camera when the height is fairly stable between frames (no great than 10% change).

When the camera moves though, we use what was the previous behavior regardless.

[ggetz]

I found that this is a helpful sandcastle for testing.

1. Navigate to the grand canyon using the geocoder
2. With the globe on, orient the camera to ellipsoid height
3. Disable the globe
4. Enable the tileset

You will likely start underground while the tileset loads in, and then you should now pop to the surface as the loaded tiles stabilize.

[ggetz]

@jjspace This should be ready for another look.

[jjspace]

Thanks @ggetz I think all the updated code looks good now. I believe I tested as you outlined and was not able to get stuck under the ground. I did trigger the issue I pointed out above once or twice but that's harder and a slight edge case.

Did you test this for performance? I noticed it felt kinda sluggish for a bit but went away after refreshing and I can't consistently reproduce. I've been having intermittent issues with getting locked to CPU rendering so it might be on my end.

Also did you want @mramato to take a peak at this before merging?

[ggetz]

Thanks @jjspace!

I did not see any impact on FPS.

I also tested time to first render, throttled to average US network speeds:

	branch	main
cached	5.95	5.0145
	5.8841000000238415	5.010599999964237
	6.122299999952316	5.890199999988079
average	5.98	5.3
uncached	6.185800000011921	7.366
	6.7466999999880795	6.9873000000119205
	6.41	6.614699999988079
average	6.45	6.99

While there may have been a slight impact on time to first render (at least when cached), it appears to be small.

[ggetz]

@mramato Would you mind taking this for a spin and letting us know if you have any feedback?

[mramato]

@ggetz The good news is that I was not able to get the camera to go below or stay below the Google tileset. So that's a huge improvement and navigating around was much better. 🎉

I did run into a (related?) issue when it comes to camera movement near the ground. If you use the GP3DT Sandcastle example and there is a tennis court just off to the right, when you are high up, you can man with the mouse left-button no problem. however the moment you are close to the ground, the camera seems to be "stuck". I made a video to show what I mean. When you see me wiggling the mouse pointer and nothing is happening, that's me actually holding down the left button and the camera not really responding. I can show you in person tomorrow if you can't reproduce.

2024-02-29.14-46-43.mp4

[mramato]

@ggetz Just realized the above happens even when enableCollision is false. Could this be another symptom of #4368 or #7092? (I didn't check to see if this area is below the ellipsoid)

[ggetz]

Thanks @mramato!

I can replicate the same issue you describe in 1.113, before any of the 3D Tiles camera modifications went in. So I'm chalking it up to the previously existing picking issues below ellipsoid.

Since it's a valid complaint, I'll add your report to the relevant issue. But I think it's outside the scope of this PR.

[ggetz]

@jjspace I think we're good to merge assuming everything is good to go from your POV.

[mramato]

@ggetz fair enough. Are we planning to tackle that issue anytime soon? It's becoming a more common problem given data look Google and CWB.

[ggetz]

Are we planning to tackle that issue anytime soon? It's becoming a more common problem given data look Google and CWB.

No concrete plans at the moment, but seeing if we can get it prioritized soon in planning.