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

Add clampToGround to kml #6952

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add clampToGround to kml #6952

wants to merge 6 commits into from

Conversation

nf-s
Copy link
Contributor

@nf-s nf-s commented Oct 25, 2023

Add clampToGround to kml

Fixes #6799

Related to

Terria used to clamp 2D and 3D KML polygon features to ground, and there was no way to turn it off. I have removed Terria's clamping, added a clampToGround trait, and now we pass that into KmlDataSource.load() - where cesium will now handle clamping

clampToGround will default to true - but Cesium will respect altitudeMode in KML - so it won't clamp 3D polygons that have altitudeMode set to absolute or relativeToGround.

There is a weird behaviour where KML polygons will only be clamped if in 3d (cesium) mode, this is because when loadMapItems is loaded, cesium terrain is sampled to clamp polygon features. This is now fixed.

Test me

{
  "name": "3D KML Test clampToGround = false",
  "type": "kml",
  "url": "https://gist.githubusercontent.com/nf-s/cfa170f067c8c462467558e9971f811d/raw/acf861a79fd206770029bb739fb76b5b84080e98/test-polygon.kml",
  "clampToGround": false
},
{
  "name": "3D KML Test clampToGround = true",
  "type": "kml",
  "url": "https://gist.githubusercontent.com/nf-s/cfa170f067c8c462467558e9971f811d/raw/acf861a79fd206770029bb739fb76b5b84080e98/test-polygon.kml",
  "clampToGround": true
},
{
  "name": "2D KML Test clampToGround = false",
  "type": "kml",
  "url": "https://gist.githubusercontent.com/nf-s/e0174e6f4114b9bab01307fe183be1e2/raw/7c2e5c210d8b7f0120d961d4e6086ad92b0caf44/test-2d-polygon.kml",
  "clampToGround": false
},
{
  "name": "2D KML Test clampToGround = true",
  "type": "kml",
  "url": "https://gist.githubusercontent.com/nf-s/e0174e6f4114b9bab01307fe183be1e2/raw/7c2e5c210d8b7f0120d961d4e6086ad92b0caf44/test-2d-polygon.kml",
  "clampToGround": true
}
  • Before link
    • Notice that all polygons are clamped to the ground
  • After link
    • Notice that all 3D polygons are not clamped to the ground (regardless of clampToGround value)
    • Notice that 2D polygons are clamped when clampToGround is true (set mode to 3d terrain)

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

@imakihi
Copy link
Contributor

imakihi commented Nov 5, 2023

@nf-s
"clampPolygonsToGround": false worked!!!
Thank you for making this work.
Best.
Hiroo

@imakihi
Copy link
Contributor

imakihi commented Nov 10, 2023

@nf-s
Is it possible to set clampPolygonsToGround = false as a default?
I wanted to locally import KML files and overlay those in the 3D mode (not being clumped).
What do you think?

Hiroo

@nf-s nf-s changed the title Add clampPolygonsToGround to kml Add clampToGround to kml Nov 10, 2023
@nf-s
Copy link
Contributor Author

nf-s commented Nov 10, 2023

Hi @imakihi

I looked into this a bit more and I have removed the polygon clamp feature. Cesium now has a clampToGround flag that applies its own logic to KML files.

This means that KML files with 3D polygons that have altitudeMode set to absolute or relativeToGround will never be clamped - regardless of clampToGround.

We still need to do some testing - but I think this is a better way forward than before, where Terria was manually overriding coordinate heights.

What do you think @steve9164 ?

@imakihi
Copy link
Contributor

imakihi commented Nov 13, 2023

Hi @nf-s
Thank you for looking into this. Your logic makes sense to me.
Please let me know if I can test this new implementation.

@nf-s
Copy link
Contributor Author

nf-s commented Nov 13, 2023

Hi @nf-s Thank you for looking into this. Your logic makes sense to me. Please let me know if I can test this new implementation.

Hi @imakihi

That would be great, thanks!
If you have a few different sources of KML, can you please drag them into the test deployment (http://ci.terria.io/kml-clamp-polyhon) and let us know if you see any weird behavior with the new clampToGround (this will be set to true by default)?

@imakihi
Copy link
Contributor

imakihi commented Nov 13, 2023

@nf-s
I tested 5 kml files with different altitude modes.

  • absolute
  • clump to ground
  • clump to seafloor
  • relative to ground
  • relative to seafloor

Except relative to ground, I think, it worked. When I drag and drop my test file for the relative to ground, it doesn't clump, I think.
I created my sample KML file from here.
https://developers.google.com/kml/documentation/altitudemode

These are my test files.
altitudemode.zip

@nf-s nf-s mentioned this pull request Nov 15, 2023
Copy link
Member

@steve9164 steve9164 left a comment

Choose a reason for hiding this comment

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

The code changes look good. Have you had time to test the KMLs?

@nf-s
Copy link
Contributor Author

nf-s commented Dec 4, 2023

I tested the KMLs, but couldn't find a difference between clampToGround = true or false

To be honest, I find it quite difficult to figure out exactly how clampToGround works in cesium - the code path is pretty annoying to follow.

Basically - if clampToGround is true

Anyway, I think we should just merge this in. This respects KML features (and altitude mode), and correctly passes clampToGround into cesium.

@nf-s nf-s mentioned this pull request Dec 7, 2023
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.

Losing Z values when multiple KML files are overlaid
3 participants