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 debug wireframe to ModelExperimental #10332

Merged
merged 15 commits into from
Apr 29, 2022
Merged

Conversation

j9liu
Copy link
Contributor

@j9liu j9liu commented Apr 27, 2022

Closes #9066. This PR adds the debugWireframe option to ModelExperimental. The current wireframe code in Model leads to tangled / incorrect wireframes, but ModelExperimental implements it properly.

Model `Model Experimental
image image
image image
image image

Here's a modified version of the Model Experimental sandcastle that allows you to toggle the wireframe.

This should also work for tilesets with Cesium.ExperimentalFeatures.enableModelExperimental = true. For example, here's the result of the changes in the 3D Tiles Inspector sandcastle (localhost)

With Model With ModelExperimental
image image

Note: this required changes to GltfIndexBufferLoader because implementing the wireframe required the index buffer to be available on the CPU. Index buffers are now loaded as typed arrays only in WebGL1 since it's not possible to get data from the buffer directly. With WebGL2, either buffers or typed arrays are fine.

@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.

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 for gl.TRIANGLES, this is working very nicely! I had some comments about making this more robust to handle triangle strips/fans

Source/Core/PrimitiveType.js Show resolved Hide resolved
Source/Scene/ModelExperimental/buildDrawCommands.js Outdated Show resolved Hide resolved
Specs/Scene/ModelExperimental/ModelExperimentalSpec.js Outdated Show resolved Hide resolved
@j9liu
Copy link
Contributor Author

j9liu commented Apr 28, 2022

@ptrgags updated, here's a Sandcastle with TRIANGLE_STRIP and TRIANGLE_FAN models to demonstrate that they work

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 code updates look solid, and the new models are working properly.

One thing I noticed is that for some reason the GroundVehicle model doesn't seem to render properly when switched to wireframe mode. Any idea why this might be?

image

Source/Core/WireframeIndexGenerator.js Outdated Show resolved Hide resolved
Source/Core/WireframeIndexGenerator.js Outdated Show resolved Hide resolved
@j9liu
Copy link
Contributor Author

j9liu commented Apr 29, 2022

@ptrgags I can't replicate the Ground Vehicle bug. It looks fine for me:
image

@j9liu
Copy link
Contributor Author

j9liu commented Apr 29, 2022

Turns out on Linux, normal mapping doesn't work with lines because it handles the dFdx, dFdy functions differenly. I added a define to make sure the code doesn't try to use normal maps if it is rendered in wireframe.

@ptrgags let me know if this solves the issue for you.

@ptrgags
Copy link
Contributor

ptrgags commented Apr 29, 2022

@j9liu that still didn't work unfortunately... Even when I inspect the normals with a CustomShader, I get a black wireframe mesh.

However, I also tried forcing it to render UNLIT via CustomShader, that's working:

Sandcastle

image

@ptrgags
Copy link
Contributor

ptrgags commented Apr 29, 2022

@j9liu oh wait I think I fetched without pulling 😅, let me try again

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 sorry for the false alarm, the changes are working:

image

had one small comment before I merge

Source/Shaders/ModelExperimental/MaterialStageFS.glsl Outdated Show resolved Hide resolved
@j9liu
Copy link
Contributor Author

j9liu commented Apr 29, 2022

@ptrgags updated!

@ptrgags
Copy link
Contributor

ptrgags commented Apr 29, 2022

@j9liu looks good, I'll merge once CI passes!

@ptrgags ptrgags merged commit 57d2118 into main Apr 29, 2022
@ptrgags ptrgags deleted the model-experimental-debug-wireframe branch April 29, 2022 15:58
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.

3D tile wireframe display is tangled
3 participants