Skip to content

Conversation

@bjornblissing
Copy link
Contributor

Correct S2 face range to 0–5 in MaxarValidatorCommon

@javagl
Copy link
Contributor

javagl commented Sep 4, 2025

Looks simple and reasonable, but I'd like to confirm: https://github.com/Maxar-Public/glTF/blob/6262a06b4b865c653bfbd274deb83dcb317e21ea/extensions/2.0/Vendor/MAXAR_image_ortho/schema/srs.schema.json#L30 looks like a different pattern. (It's late here, maybe I overlooked something obvious - in doubt, I'll take a closer look soon)

@bjornblissing
Copy link
Contributor Author

bjornblissing commented Sep 5, 2025

I'd like to confirm: https://github.com/Maxar-Public/glTF/blob/6262a06b4b865c653bfbd274deb83dcb317e21ea/extensions/2.0/Vendor/MAXAR_image_ortho/schema/srs.schema.json#L30 looks like a different pattern. (It's late here, maybe I overlooked something obvious - in doubt, I'll take a closer look soon)

Yes, 1-6 is regrettably wrong. This is one of the things we found during our review pass of our extensions.

As its written right now we would exclude face 0, i.e. most of Africa, the southern part of the Atlantic ocean, the entire Mediterranean Sea, etc...
http://s2geometry.io/resources/img/face0.jpg

@javagl
Copy link
Contributor

javagl commented Sep 5, 2025

I see. That's one of the points that are nearly impossible to detect during a review of the validator code itself (even when looking it up in the extension spec during the review). (And comparing it to the S2 spec may not be realistic either, because even though the faces there are numbered 0..5 in S2, there's nothing preventing an extension from serializing that as 1..6 into JSON (which would be dangerously confusing, but possible...))

(An aside: The current S2 Bounding Volume Validation is a bit shallow in this regard. It does a basic validation, but the main purpose of that class is for the validator to not generate an ERROR when an S2 bounding volume is encountered)

@javagl javagl merged commit ed028dd into CesiumGS:main Sep 5, 2025
1 check passed
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.

2 participants