Skip to content

Conversation

@hybridherbst
Copy link
Contributor

@hybridherbst hybridherbst commented Jul 16, 2020

Set subdivision scheme to "none" and explicity set normal interpolation to "vertex". This fixes #192 but will need additional changes for "USD roundtrip" scenarios where keeping the subdiv scheme is desired. I think that is a problem for later since such a round trip would also require proper support for faceVarying attributes being carried through / reconstructed on export if desired.

Current state of this should potentially be added to the docs to make it more clear what "roundtrip" means and what export currently does.

Image from iOS
Image from iOS (1)
Image from iOS

Copy link
Contributor

@jcowles jcowles left a comment

Choose a reason for hiding this comment

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

Thanks!

I think we should move subdiv scheme down to the base schema class, let me know if you want to do this or if you would prefer me to do it.

// for polygonal meshes, we do not want the default subdivision scheme of "catmull-clark" but instead "none"
// so that authored normals keep working.
var usdGeomMesh = new pxr.UsdGeomMesh(usdPrim);
usdGeomMesh.CreateSubdivisionSchemeAttr().Set(new pxr.TfToken("none"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should probably make this part of the schema class, in USD.NET.Unity.

usdGeomMesh.CreateSubdivisionSchemeAttr().Set(new pxr.TfToken("none"));

// explicitly set the normal interpolation here –
// default should be "vertex" anyways but seems some viewers don't respect the default
Copy link
Contributor

@jcowles jcowles Jul 16, 2020

Choose a reason for hiding this comment

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

Can you note the problematic viewers and versions here, for future reference?

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.

3 participants