-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Add support for GLTF extension: EXT_mesh_primitive_edge_visibility #12859
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
base: main
Are you sure you want to change the base?
Conversation
@ggetz @lilleyse I’ve fixed the edge depth bug that caused geometry can see through edges. The latest screenshot have been updated in the description. Demo link will update ASAP, since not sure why there are some unrelated timeout build errors in Github. This is now ready for the next round of review, thank you! |
Demo Link updated as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danielzhong Excellent job with this PR!
Just a few comments from me.
shaderBuilder.addVarying("vec3", "v_faceNormalBView", "flat"); | ||
|
||
// Add varying for view space position for perspective-correct silhouette detection | ||
shaderBuilder.addVarying("vec3", "v_positionView", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know offhand... does Model
already have a varying for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. Fixed!
packages/engine/Source/Scene/Model/EdgeVisibilityPipelineStage.js
Outdated
Show resolved
Hide resolved
packages/engine/Source/Scene/Model/EdgeVisibilityPipelineStage.js
Outdated
Show resolved
Hide resolved
* @returns {{edgeMap:Map<string, number[]>, faceNormals:Float32Array, triangleCount:number}} | ||
* @private | ||
*/ | ||
function buildTriangleAdjacency(primitive) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this function and others that are doing a lot of math see if you can use Cartesian3
types and Cartesian3
math operations (e.g. Cartesian3.cross
). Also try to use scratch variables wherever possible to avoid heap allocations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified, please review again! Thanks!
const positions = defined(positionAttribute.typedArray) | ||
? positionAttribute.typedArray | ||
: ModelReader.readAttributeAsTypedArray(positionAttribute); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you foresee any issues if positions are quantized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, please review.
const renderState = clone(command.renderState, true); | ||
edgeCommand.renderState = RenderState.fromCache(renderState); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the render state actually modified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to clean it up. Fixed!
// Use a very large bounding volume to avoid culling issues | ||
edgeCommand.boundingVolume = new BoundingSphere( | ||
Cartesian3.ZERO, | ||
Number.MAX_VALUE, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could have some performance side effects.
createPotentiallyVisibleSet
determines the near / far planes from the commands' bounding volumes and ideally we want these to be as tight as possible to avoid needing to render with multiple frustums.
I'm curious about the culling issues you were seeing, because it seems like it should be fine to use the original command's boundingVolume
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
// | ||
// CESIUM_REDIRECTED_COLOR_OUTPUT: A general-purpose flag to indicate that this shader | ||
// is a derived/modified version created by Cesium's rendering pipeline. | ||
// This flag can be used to avoid color attachment conflicts when shaders | ||
// need different output declarations for different rendering passes. | ||
// For example, MRT (Multiple Render Targets) features can check this flag | ||
// to conditionally declare their output variables only when not conflicting | ||
// with the derived shader's output layout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit rusty here, but is the problem that the edge visibility code, even though it's not executed in the OIT pass, has output declarations that are incompatible with OIT's output declarations?
If that's the case, this seems like an okay approach, short of other approaches that would require refactoring the model pipeline.
Co-authored-by: Sean Lilley <lilleyse@gmail.com>
Co-authored-by: Sean Lilley <lilleyse@gmail.com>
![]() @lilleyse Not sure why I can't reference or reply. But yes, that’s the root cause. Do you think I should include the refactor in this PR, or would it be better to open a new PR for it later? |
@danielzhong oh I think that comment is part of this comment thread: #12859 (comment) I would just open an issue for now. |
The
CesiumJS does not render anything for this model. (Opening and re-exporting it with gltf.report generates a model that can be rendered with CesiumJS. Whether or not that is related to the validation error remains to be investigated). The model uses
There should be test model that uses I did not peform a thorough review of this PR! It only caught my attention due to #12914 . And when I saw https://github.com/CesiumGS/cesium/pull/12859/files/1c9ba9d2415aa507cce107d91938d10c9e707cd1#diff-20964905fdcbaf8679c56d995e848ed68a3c0f08d868ad38c5d794145b0fbbc6R70, I thought: Wait, that can hardly be right. Edge visibility and feature IDs are totally unrelated. Maybe these |
Thanks for the feedback, it’s fixed |
Description
This PR implements the proposed GLTF extension
EXT_mesh_primitive_edge_visibility
, enabling edge rendering for glTF mesh primitives. It adds multi-pass rendering to avoid z-fighting, and implements decoding, creation, and rendering of both hard and silhouette edges:More details: Edge Visibility
Example

(green edge = hard edge; red edge = silhouette edge):
Issue number and link
#12765
Testing plan
Added Unit test for Edge Frame Buffer, MRT & Shader Color Attachments, Decoding logic, Shader Generation, etc.
Real Time Demo
Local Test File:
super_simple_3.bim-tiles.zip
How to Run:
http-server ./ --cors=X-Correlation-Id
Make sure change your local host address in the code!
((green edge = hard edge; red edge = silhouette edge):)

🔍Review Notes
getWebGLStub.js
to support MRT in coverage tests.Bug Fix Summary
modelFS.glsl
. The model runtime primitive couldn’t detect the render tag (it runs too early), so I introduced a uniform flag to detect when the pipeline stage is active.clearCommand
inEdgeFramebuffer.js
.getWebGLStub.js
to support MRT in coverage tests.Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change