MeshTessellate : Store subdivision scheme as string #5787
Merged
Conversation
This file contains 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.
Switches our representation of subdivision scheme to just be a string rather than a custom enum. This is a bit simpler, and will be more consistent when other subdivision options are stored as strings ( the other options will come later, but we want to get this merged while 1.4 is still in beta, and it shouldn't cause problems to change this ).
All pretty straightforward - the only possible point of discussion I can think of is "bilinear" vs "linear". I think the approach of taken is correct - "bilinear" is the name of a subdivision scheme ( as is the convention in USD and OpenSubdiv ). "linear" is something we store in the interpolation property of a mesh, which means the mesh should be interpreted as simple polygons.
In the long term, we should support "bilinear" in the interpolation property of a mesh, to indicate a mesh which needs tessellation, but with a flat limit surface, and we should potentially rename "linear" to "none". I think this PR helps move us in that direction.
One tiny downside to this PR - in the UI, scheme = "" is represented as "From Mesh", so the UI is clear. But the API to MeshAlgo::tessellateMesh now just has scheme = "", which is a bit less clear that it's coming from the mesh than when it was scheme = SubdivisionScheme::FromMesh. There is maybe more of an argument now for naming it overrideScheme? But it's probably more important that the UI be nice and clear, and we want to match the name used in the UI, so this is probably fine?