Data Exchange, GLTF - fix saving edges when Merge Faces is enabled#554
Merged
dpasukhi merged 6 commits intoOpen-Cascade-SAS:IRfrom May 23, 2025
Merged
Data Exchange, GLTF - fix saving edges when Merge Faces is enabled#554dpasukhi merged 6 commits intoOpen-Cascade-SAS:IRfrom
dpasukhi merged 6 commits intoOpen-Cascade-SAS:IRfrom
Conversation
elias-bananaz
commented
May 20, 2025
Comment on lines
-290
to
-301
| switch (theGltfFace.Shape.ShapeType()) | ||
| TopAbs_ShapeEnum shapeType = getShapeType(theGltfFace.Shape); | ||
|
|
||
| switch (shapeType) | ||
| { | ||
| case TopAbs_COMPOUND: | ||
| case TopAbs_COMPSOLID: | ||
| case TopAbs_SOLID: | ||
| case TopAbs_SHELL: | ||
| case TopAbs_FACE: | ||
| return true; | ||
| case TopAbs_WIRE: | ||
| case TopAbs_EDGE: | ||
| case TopAbs_VERTEX: | ||
| default: |
Contributor
Author
There was a problem hiding this comment.
When checking the face in theGltfFace, we shouldn't expect anything other than Face, Edge, Vertex or Compound, since these are what the specific iterators (Face, Edge, Vertex) are extracting. And Compound is only added in the case of merged faces.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with GLTF exports when merge faces is enabled by ensuring that edge data is saved correctly using Lines instead of LineStrip. Key changes include adjusting primitive mode checks in triangulation and JSON parsing, introducing a virtual getShapeType function, and updating index calculations for edges and vertices.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| RWGltf_TriangulationReader.cxx | Removed check for LineStrip mode to force use of Lines for merged faces |
| RWGltf_GltfJsonParser.cxx | Updated primitive mode checks by removing LineStrip references |
| RWGltf_CafWriter.hxx / RWGltf_CafWriter.cxx | Added getShapeType to correctly determine the underlying shape type from compounds and updated index calculation logic |
dpasukhi
approved these changes
May 23, 2025
elias-bananaz
added a commit
to Bananaz-ai/OCCT
that referenced
this pull request
May 25, 2025
…en-Cascade-SAS#554 Removed check for LineStrip mode to force use of Lines for merged faces Updated primitive mode checks by removing LineStrip references Added getShapeType to correctly determine the underlying shape type from compounds and updated index calculation logic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When using
SetMergeFaces(true)with edges, the GLTF exported is invalid. The edges are not saved. Using Draco compression fails as well.The Problem
When a document is saved with merged faces, a Compound is created to group together the different faces. But when checking the type of the shape in several places, it's checked for either edge, vertex or defaults to face. That means that a compound of edges is also treated using the FaceIterator.
The Solution
Look at the underlying shape of a compound. The only possibilities should be: Vertex, Edge, Face, or a Compound that contains exclusively one kind of those.
Note
This PR undoes the change introduced in #535. While exchanging
LinesforLineStripworks for unmerged faces, where each edge is added individually, this breaks when using merge faces. Since all edges are now in the same mesh, they are all joined together, resulting in extra edges appearing between the actual edges.Instead, the approach proposed here is to return to using Lines, but use the indices the fix the gap problem.