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

GPM sandcastle #12232

Merged
merged 14 commits into from
Nov 22, 2024
Merged

GPM sandcastle #12232

merged 14 commits into from
Nov 22, 2024

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Oct 5, 2024

Edit: This has been updated to build upon #12237 , which therefore should be merged first

Description

Support for the NGA_gpm_local extension has recently been added to Cesium JS, via #12204. This (indirectly) includes the possibility to pick metadata values from the GPM PPE (Per-Point Error) textures, by treating them as property textures and apply metadata picking.

This PR adds a Sandcastle that demonstrates the GPM visualization.

For now, this Sandcastle is in the /Development section, but can easily be moved elsewhere. It includes the option to visualize property texture values, and therefore might eventually replace https://sandcastle.cesium.com/index.html?src=Custom%20Shaders%20Property%20Textures.html

The current state is a draft. It already replicates most of the functionality from an existing demo. But unfortunately, there seems to be an issue that is related to scaling and picking the metadata values from property textures for this data set: The picking functionality currently returns wrong values. The reason for this still has to be investigated.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

github-actions bot commented Oct 5, 2024

Thank you for the pull request, @javagl!

✅ We can confirm we have a CLA on file for you.

@lilleyse
Copy link
Contributor

lilleyse commented Oct 5, 2024

For now, this Sandcastle is in the /Development section, but can easily be moved elsewhere

I think the 3D Tiles folder would be better, more visible and easier to access from sandcastle.cesium.com.

@javagl
Copy link
Contributor Author

javagl commented Oct 5, 2024

I can move it there with the next update.

About the wrong metadata values:

This is related to the handling of offset/scale. The ppeTexture contains offset/scale. And there is no concept of normalized in the PPE Textures. But when translated to property textures, they have to be normalized in order to even be able to apply offset/scale. So in the property texture, normalized has to be set to true, and then taken into account in the scale factor. This was anticipated in the GPM part (EDIT: Fixed link). But on the metadata picking side, every normalized/offset/scale basically has to be "undone", to fully exploit the 256 values of the picking frame buffer. Right now, it only took normalized into account, but it essentially has to do what apply/unapplyValueTransform are doing. Things might become more tricky for more complex cases, where the 'class property' defines some offset/scale, and the 'property texture property' overrides the offset/scale. Care has to be taken to get the forward- and backward conversions right...

I do have a fix for that locally that "works" for the GPM data. But I'd like to test (and describe/document!) it more thoroughly.

@ggetz
Copy link
Contributor

ggetz commented Nov 21, 2024

@javagl I've merged #12237, and subsequently merged main into this PR. Is it ready to be moved out of Draft and be reviewed?

As an aside, it looks like this PR has two copies of the Sandcastle, one in Developement/ and one at the gallery root. Is that intentional?

@javagl javagl marked this pull request as ready for review November 21, 2024 19:46
@javagl
Copy link
Contributor Author

javagl commented Nov 21, 2024

As an aside, it looks like this PR has two copies of the Sandcastle, one in Developement/ and one at the gallery root. Is that intentional?

No, let me check...

@javagl
Copy link
Contributor Author

javagl commented Nov 21, 2024

Indeed, the one in development was the initial version - this should have been removed when adding the one in the (non-development) 3D Tiles section ( b4e5af4 ), but it was removed now.

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Overall this Sandcastle is looking and working great @javagl!

I have a few nitpick-y code comments.

I did notice that lower-resolution LODs of the tilesets, seen when zoomed out, weren't getting the Custom Shader applied, nor any ellipsoid anchor points, and returned "unknown" values when picked.

image

This seems to be expected behavior, but let me know if it's not.

javagl and others added 3 commits November 22, 2024 19:02
Co-authored-by: Gabby Getz <[email protected]>
Co-authored-by: Gabby Getz <[email protected]>
Mainly defined checks

Co-authored-by: Gabby Getz <[email protected]>
@javagl
Copy link
Contributor Author

javagl commented Nov 22, 2024

This seems to be expected behavior, but let me know if it's not.

It is expected. There has been some discussion about how this level could be identified to begin with. When you're loading the initial tileset, you don't know how deeply you have to zoom to have that information (i.e. you don't know in which level these things will be available). That's one reason why the initial view configurations for the sample data sets has been chosen carefully to be zoomed in closely enough.

I also thought about how that could be made more apparent. Indicating it visually could be difficult. There might not be a silver bullet in general. But maybe mentioning this in the "info box" text might be one way...?

@ggetz
Copy link
Contributor

ggetz commented Nov 22, 2024

Thanks @javagl! I mostly just wanted to confirm from a testing testing standpoint.

At least at this point, I don't think further action is needed to indicate this.

@ggetz ggetz enabled auto-merge November 22, 2024 18:16
@ggetz ggetz added this pull request to the merge queue Nov 22, 2024
Merged via the queue into main with commit bd0f24f Nov 22, 2024
5 checks passed
@ggetz ggetz deleted the gpm-sandcastle branch November 22, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants