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 GlobeRectangle::splitAtAntiMeridian #848

Merged
merged 13 commits into from
Apr 23, 2024
Merged

Conversation

kring
Copy link
Member

@kring kring commented Apr 5, 2024

This new method splits a GlobeRectangle that crosses the anti-meridian into two globe rectangles that touch but do not cross it.

This PR also uses this new method in RasterOverlayUtilities::createRasterOverlayTextureCoordinates in a slightly dodgy way. Consider a tile that touches the anti-meridian, such as a tile from Cesium World Terrain. Due to slight numerical noise in the model -> ECEF -> longitude/latitude transformation, vertices on that boundary may end up on the wrong side of it. For example, a tile that spans from 170 to 180 degrees longitude might have a vertex that's deemed to be at -179.99999 degrees longitude. And previously, when that happened, the results were fairly disastrous. projectRectangleSimple would produce an invalid rectangle and all of the generated texture coordinates would be invalid.

With the change in this PR, the portion of the rectangle on the "wrong" side of the anti-meridian is thrown away. Texture coordinates for any vertices that happen to land slightly on the wrong side get clamped to the valid range, which is exactly the right thing to do in this example where a Cesium World Terrain tile touches the anti-meridian.

Now, if a tile truly crosses the anti-meridian, this won't work quite as well. An actual meaningful portion of the tile's rectangle will be thrown away. But fixing that is pretty tricky, requiring that this function be able to produce multiple projected overlay rectangles for the input glTF. And in any case the end result is much better after this PR than it was before before it. After this PR, the larger portion of the tile, at least, will have correct raster overlay mapping. Previously none of it would have worked correctly.

@kring kring closed this Apr 5, 2024
@kring kring reopened this Apr 5, 2024
@timoore timoore self-assigned this Apr 6, 2024
@timoore
Copy link
Contributor

timoore commented Apr 6, 2024

Not sure if I should merge this myself, but it all looks pretty straightforward. It's interesting to note that any "noise" in coordinates getting the wrong longitude value must be coming from the conversion from local coordinates to ECEF, as the the atan function used to calculate longitude must choose the correct quadrant based on the sign of the ECEF y coordinate.

@csciguy8
Copy link
Contributor

csciguy8 commented Apr 8, 2024

Not sure if I should merge this myself,

@timoore I'd say go for it, if you feel it's ready to go.

If you think another reviewer could help, you can always tag others (like me) to give input too.

@csciguy8 csciguy8 self-requested a review April 17, 2024 16:40
@csciguy8
Copy link
Contributor

Tagging myself as a reviewer to help move this one along...

Copy link
Contributor

@csciguy8 csciguy8 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 @kring !

I added a couple more tests as I was reviewing, so may want to glance at those.

Now, if a tile truly crosses the anti-meridian, this won't work quite as well. An actual meaningful portion of the tile's rectangle will be thrown away.
Do you think it's possible for this to be an observable problem right now? (with raster overlays or other features). If so, sounds worth writing an issue.

Tested with cesium-unreal as a sanity check. All good.

@kring
Copy link
Member Author

kring commented Apr 23, 2024

Do you think it's possible for this to be an observable problem right now?

To observe this right now, you'd have to find a tile that crosses the anti-meridian. I don't know of any tilesets with tiles like that offhand.

@kring
Copy link
Member Author

kring commented Apr 23, 2024

Your new tests look good, thanks!

@csciguy8 csciguy8 merged commit cc27e7f into main Apr 23, 2024
14 checks passed
@csciguy8 csciguy8 deleted the split-at-antimeridian branch April 23, 2024 15:15
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