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

Swapped lat long #10

Merged
merged 7 commits into from
Sep 23, 2024
Merged

Swapped lat long #10

merged 7 commits into from
Sep 23, 2024

Conversation

tfili
Copy link

@tfili tfili commented Sep 20, 2024

@keyboardspecialist I had to do this for some japan data too so I hijacked your branch. We should try and detect this but for now instead of using a branch, I think we should just add an envvar to the released version so we can use it when we need to.

I thinks I fixed it for real. See https://gdal.org/en/latest/tutorials/osr_api_tut.html#crs-and-axis-order

We need to test his a bunch before we can merge. I'll create a asset-pipeline PR with it so we can run some tests.

Testing

  • Run ./citygmlswayzetests in the asset-pipeline update-libcitygml branch - there was a test added to sets the env var
  • Verify everything passes
  • For extra validation you can run CITYGML_USE_SRS_ORDERING=1 ./citygmlswayzetests
  • Verify there are failures with the Lat/Lon being swapped

@keyboardspecialist
Copy link

Awesome, glad to see a real fix.

Here's the issue I opened earlier this year https://github.com/CesiumGS/asset-pipeline/issues/1528

I was on the right track, but never tried that flag.

@tfili
Copy link
Author

tfili commented Sep 20, 2024

@shehzan10 you know more about CityGML. Is there a reason that the incorrect ordering was specified on purpose? Should it be an option?

@shehzan10
Copy link
Member

@tfili We used the previous option based on the GDAL documentation. If you are confident that the new flag is the right one to use to always return [long, lat], then lets go with that.

It wasn't so much that the incorrect ordering was on purpose. The EPSG:6697 CRS is the only one I've seen that uses Lat, Long ordering, and we couldn't figure out a way until now to always make the function return long, lat regardless of input.

@tfili
Copy link
Author

tfili commented Sep 20, 2024

Looking into it a bit more, it seems like CityGML generally expects traditional GIS order. Looking at https://gdal.org/en/latest/drivers/vector/gml.html it is what GDAL defaults to when loading CityGML. I think the best option is to use an env var for us to swap it if needed for our tilesets. We may want to potentially expose it to customers at some point but I don't think there has been a need so far.

@tfili
Copy link
Author

tfili commented Sep 20, 2024

@keyboardspecialist This is ready for review.

@keyboardspecialist
Copy link

I'm getting a ton of test failures on my local linux build (release and debug)

[  FAILED  ] 24 tests, listed below:
[  FAILED  ] CityGMLSwayzeMainTests_GeoRES.CountModels
[  FAILED  ] CityGMLSwayzeMainTests_GeoRES.CountModelInstances
[  FAILED  ] CityGMLSwayzeMainTests_GeoRES.CountTextures
[  FAILED  ] CityGMLSwayzeMainTests_GeoRES.ReadProperties
[  FAILED  ] CityGMLSwayzeMainTests_GeoRES.CountBatchedAndInstanced
[  FAILED  ] CityGMLSwayzeMainTests_GeoRES.TotalPrimitivesAndTexels
[  FAILED  ] CityGMLSwayzeMainTests_GeoRES.BatchTablePropertiesAreSet
[  FAILED  ] CityGMLSwayzeMainTests_GeoRES_LOD2.ReadProperties
[  FAILED  ] CityGMLSwayzeMainTests_NYC12.CountModels
[  FAILED  ] CityGMLSwayzeMainTests_NYC12.CountModelInstances
[  FAILED  ] CityGMLSwayzeMainTests_NYC12.ReadProperties
[  FAILED  ] CityGMLSwayzeMainTests_NYC12.CountBatchedAndInstanced
[  FAILED  ] CityGMLSwayzeMainTests_NYC12.TotalPrimitivesAndTexels
[  FAILED  ] CityGMLSwayzeMainTests_NYCWTC.CountModels
[  FAILED  ] CityGMLSwayzeMainTests_NYCWTC.CountModelInstances
[  FAILED  ] CityGMLSwayzeMainTests_NYCWTC.CountBatchedAndInstanced
[  FAILED  ] CityGMLSwayzeMainTests_NYCWTC.TotalPrimitivesAndTexels
[  FAILED  ] CityGMLSwayzeMainTests_NYCWTC.GltfModelHasVertexColors
[  FAILED  ] CityGMLSwayzeMainTests_NYCWTCDisableColors.GltfModelDoesNotHaveColors
[  FAILED  ] CityGMLSwayzeMainTests_NYCWTCDisableVC.GltfModelDoesNotHaveVertexColors
[  FAILED  ] CityGMLSwayzeMainTests_GeoRESWithoutSRSWithSRSOption.CountModels
[  FAILED  ] CityGMLSwayzeMainTests_GeoRESWithoutSRSWithSRSOption.CountModelInstances
[  FAILED  ] CityGMLSwayzeMainTests_GeoRESWithoutSRSWithSRSOption.CountBatchedAndInstanced
[  FAILED  ] CityGMLSwayzeMainTests_GeoRES_Swapped.ReadProperties

@tfili
Copy link
Author

tfili commented Sep 23, 2024

@keyboardspecialist The tests seem to be passing here. Did you update submodules?

@keyboardspecialist
Copy link

@tfili For some reason my tests are failing when building through docker. Using my system's gcc works fine.

May be long overdue for a system clean.

Copy link

@keyboardspecialist keyboardspecialist left a comment

Choose a reason for hiding this comment

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

Looks good. Tests are passing for me now.

@keyboardspecialist keyboardspecialist merged commit 2df18a9 into master Sep 23, 2024
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.

3 participants