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 Sandcastle for glTF PBR extensions #12012

Merged
merged 4 commits into from
Jun 3, 2024
Merged

Add Sandcastle for glTF PBR extensions #12012

merged 4 commits into from
Jun 3, 2024

Conversation

jjhembd
Copy link
Contributor

@jjhembd jjhembd commented Jun 3, 2024

Description

Adds a Sandcastle to demonstrate the new glTF PBR extensions.

Testing plan

Load the glTF PBR Extensions Sandcastle and verify the different models load and render, and try the two lighting options.

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 update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

github-actions bot commented Jun 3, 2024

Thank you for the pull request, @jjhembd!

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

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.

Thanks @jjhembd! Do we still need to maintain the dev sandcastle examples now that we have this example?

content="Use Viewer to start building new applications or easily embed Cesium into existing applications."
/>
<meta name="cesium-sandcastle-labels" content="Showcases" />
<title>Cesium Demo</title>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the title and description tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the description. I think the title is auto-replaced elsewhere. All the other Sandcastles have "Cesium Demo" here.

});

const { scene, camera, clock } = viewer;
const {
Copy link
Contributor

@ggetz ggetz Jun 3, 2024

Choose a reason for hiding this comment

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

This is pretty atypical when compared with how we handle imports in other Sandcastle examples. I know it's a bit more verbose, but for more consistency can we use Cesium.Cartesian3, Cesium.DirectionalLight, etc?

let model;
const modelOptions = [
{
text: "Specular Test",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we start with the Clear Coat Wicker model? It matches the thumbnail, and also provides a more "real life" use case as compared with the test models.

@jjhembd
Copy link
Contributor Author

jjhembd commented Jun 3, 2024

Do we still need to maintain the dev sandcastle examples now that we have this example?

Good point, I removed the dev sandcastles. This should be ready for another look.

Co-authored-by: Gabby Getz <gabby@cesium.com>
@ggetz
Copy link
Contributor

ggetz commented Jun 3, 2024

Thanks @jjhembd!

@ggetz ggetz merged commit b4dabd4 into main Jun 3, 2024
9 checks passed
@ggetz ggetz deleted the pbr-sandcastle branch June 3, 2024 16:10
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.

2 participants