Add warning for when clampToGround is ignored in KMLDataSource

[OmarShehata]

This came up in #8404. The clampToGround option is ignored in KMLDataSource for lines that don't have tesselate to true. This isn't document and no warning is shown which makes this hard to figure out.

Here's a localhost Sandcastle to test with. Notice the line clamps fine, but if you change the tesselate in the KML doc to <tesselate>0</tesselate> you'll see the line doesn't clamp and, in master, no warning or anything is shown.

We could also document or warn about the case where a line that "can be extruded" are also not clamped, so it looks like the line just can't have an altitude mode of absolute, relativeToGround, or relativeToSeaFloor.

[cesium-concierge]

Thanks for the pull request @OmarShehata!

• ✔️ Signed CLA found.
• ❔ 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.
• Works (or fails gracefully) in IE11.

[mramato]

I'm not sure this warning makes sense. It's working exactly as expected by the spec, isn't it? If the behavior is different than Google Earth, we should fix that, but I don't think it is in this case.

[OmarShehata]

It's working exactly as expected by the spec, isn't it?

I have limited experience here, but I don't think it is. All I can find in the spec is that tesselate won't work unless altitudeMode is clamp, not the other way around:

To enable tessellation, the value for must be clampToGround or clampToSeaFloor.

I also just tested these two KML files in Google Earth, one has tesselate on and one has it off, both are set to clamp. Both do clamp in Earth but only the tesselated one clamps in CesiumJS. So it sounds like we should be changing the behavior here?

two_kml_lines.zip

I actually didn't realize that clampToGround inside the KML file meant the same thing as the clampToGround option you pass in to KMLDataSource.load (I thought the KML value just pushed all heights down instead of actually draping the geometry). Why do we have this option instead of just using the clamped value defined for each KML element?

[mramato]

Why do we have this option instead of just using the clamped value defined for each KML element?

Because there are too many caveats for us to just always use GroundPrimitive. Some of them are:

1. depthTestAgainstTerrain is required to be true for GroundPrimitive to work properly.
2. depthTestAgainstTerrain is required to be false for Primitive to work properly when draped on the ellipsoid.
3. GroundPrimitive outlines don't work
4. GroundPrimitive is considerably slower than Primitive.

It used to be worse and the goal is to do what you suggest, but we have a while to go. Some of this is captured in #5942 (since the real goal is terrain on by default)

[OmarShehata]

Because there are too many caveats for us to just always use GroundPrimitive

That's fair, thanks for the explanation.

It's working exactly as expected by the spec, isn't it? If the behavior is different than Google Earth, we should fix that, but I don't think it is in this case.

The behavior is indeed different from Google Earth here, which can be verified with the two KMLs I posted in #8428 (comment). It sounds to me like we should have CesiumJS always clamp regardless of whether tesselate is true or false, which would be a breaking change, but would match the behavior of Google Earth.

[cesium-concierge]

Thanks again for your contribution @OmarShehata!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[hpinkos]

@OmarShehata what's the status of this PR?

[OmarShehata]

Thanks for the bump @hpinkos . So I took another look here, I think I see why CesiumJS does not clamp lines that have tessellate set to false. We have no way to clamp something without tesselating.

In GE the untesselated clamped line will actually like clip through the terrain if there's not enough points originally. Furthermore, CesiumJS has no way to set a polyline's positions to be "relative to ground", so that behavior is different in GE vs CesiumJS too.

So if we just ignore the tesselate flag and clamp anyway that's technically incorrect. I think the warning here is the right behavior.

I just pushed a fix for the lint error. Let me know what you or @mramato think of this approach.

[hpinkos]

@OmarShehata something weird happened with the commits in this branch. Can you try resetting it or merging in master?

[OmarShehata]

@hpinkos sorry about that, not sure how that happened. Fixed now.

[hpinkos]

@mramato could you give this one a quick look?

[mramato]

While there's obviously room for improvement all around, the warning about current behavior looks fine to me. Thanks @OmarShehata

If you think it's worth it, please write up an issue for us to be able to match Google Earth, either exactly or in a way that makes more sense.

[OmarShehata]

It just came up on the forum again so I opened an issue to document my notes on this consider revisiting it: #8760.