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

Replace Gltf3DTileContent with ModelExperimental3DTileContent #10055

Merged
merged 6 commits into from Feb 2, 2022

Conversation

j9liu
Copy link
Contributor

@j9liu j9liu commented Jan 31, 2022

Fixes #10049.

In this PR, all instances of Gltf3DTileContent have been replaced by instances of ModelExperimental3DTileContent. The corresponding specs were removed. Additionally, missing tests were added for the glb file format, and the 3D Tiles Next sandcastles were updated to no longer require the enableModelExperimental flag.

Implementing these changes causes three unit tests to fail:

image

Two of these failures are expected and will be addressed in a later issue. However, the color style test should be working, and @ptrgags mentioned he would look into this tomorrow.

@j9liu j9liu requested a review from ptrgags January 31, 2022 22:09
@cesium-concierge
Copy link

Thanks for the pull request @j9liu!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@j9liu
Copy link
Contributor Author

j9liu commented Feb 1, 2022

@ptrgags - Following up on your suggestion about the color style test, specifying the time doesn't fix the test failure.

image

If you want to replicate my test, I put this at the beginning of expectColorStyle and changed all instances of expect(scene) in the function to expect(renderOptions).

      const renderOptions = {
        scene: scene,
        time: new JulianDate(2457522.154792),
      };

@ptrgags
Copy link
Contributor

ptrgags commented Feb 1, 2022

@j9liu okay, I'll take a look at this later today

Copy link
Contributor

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

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

@j9liu this code is looking solid and the sandcastles are working fine.

Just a couple small things aside from that one test that's breaking (which I'll investigate more later today)

Specs/Scene/Cesium3DTilesetSpec.js Show resolved Hide resolved
Specs/Scene/Cesium3DTilesetSpec.js Show resolved Hide resolved
Source/Scene/Cesium3DTileContentFactory.js Show resolved Hide resolved
@j9liu
Copy link
Contributor Author

j9liu commented Feb 1, 2022

@ptrgags -- thanks for the feedback! Just implemented your changes, so all that's left is figuring out what's wrong with that last test.

@ptrgags
Copy link
Contributor

ptrgags commented Feb 2, 2022

@j9liu I made a simpler sandcastle to see what's happening with the style. And it's not handling the alpha correctly.

I'm going to investigate more today, There's a chance I could have introduced a regression in #10018, or the model coloring code isn't discarding when it finds alpha = 0

@ptrgags
Copy link
Contributor

ptrgags commented Feb 2, 2022

@j9liu I confirmed that the model color stage is where the problem is. Though the fix shouldn't happen in the shader, it should be the render state that's updated. I looked back at Model.js and when the model color is invisible (alpha === 0) it updates the render state to skip rendering color and depth (because that would just be wasted GPU cycles)

In Scene/ModelExperimental/ModelColorPipelineStage.js, in the code that checks the alpha, you'll want to add these lines:

const renderStateOptions = renderResources.renderStateOptions;
  if (color.alpha === 0.0) {
    // When the model is invisible disable color and depth writes but still write into the stencil buffer
    renderStateOptions.colorMask = {
      red: false,
      green: false,
      blue: false,
      alpha: false,
    };
    renderStateOptions.depthMask = false;
  } 

However, renderStateOptions right now only exists in PrimitiveRenderResources, so there's a few other small changes needed here:

  • ModelRenderResources - initialize renderStateOptions to an empty object
  • NodeRenderResources - initialize renderStateOptions to clone from modelRenderResources.renderStateOptions (see other examples in the same file)
  • PrimitiveRenderResources - instead of cloning, use combine() to combine the additional properties with nodeRenderResources.renderStateOptions

And update unit tests as necessary.

@j9liu
Copy link
Contributor Author

j9liu commented Feb 2, 2022

@ptrgags - Thanks for the guidance. I added your changes and the test passes now. Let me know if there are any concerns with the code I added.

Copy link
Contributor

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

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

@j9liu this is looking very close, just a few small tweaks before I merge it

@j9liu
Copy link
Contributor Author

j9liu commented Feb 2, 2022

@ptrgags - just pushed!

@ptrgags
Copy link
Contributor

ptrgags commented Feb 2, 2022

@j9liu changes look good, I'm merging!

@ptrgags ptrgags merged commit 676a5b2 into main Feb 2, 2022
@ptrgags ptrgags deleted the replace-gltf3dtilecontent branch February 2, 2022 21:47
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.

Replace Gltf3DTileContent with ModelExperimental3DTileContent
6 participants