USD: optionally author subdivision schema on export #113267

Merged
Jesse Yurkovich merged 9 commits from wave/blender_wave_Apple:dev/author_subdivs_v2 into main 2023-12-15 21:11:50 +01:00

This PR adds support for exporting USD assets which have the subdivision schema applied. Current behavior prior to this PR is that the effects of Subdivision Surface modifiers are always applied to their mesh prior to export, such that it is not possible to recover the original base mesh.

In this PR we provide three options for the subdiv schema type:

None - Base mesh only (Export base mesh with subdiv type set to None)
None - Subdivided mesh (Export subdivided mesh with subdiv type set to None)
Best match (Export base mesh with subdiv schema type set to Catmull-Clark) <-- this is the default
"Best match" here means that Blender will set a subdiv scheme type in the exported USD asset when it is possible to closely match the subdivision surface type that was authored in Blender.
At this time Blender provides two subdivision types: Catmull-Clark and Simple.

Because Simple does not have a corresponding subdivision type in USD, we do not attempt to convert or represent it, and instead the Simple subdiv modifier will be evaluated and applied to the mesh during export.

Whenever a Catmull-Clark Subdivision Surface modifier is applied to an object, and is the last modifier in the stack, it is possible to set the subdiv scheme to Catmull-Clark for the respective prim in the exported USD file. In all other cases, any subdiv modifier will be evaluated and applied to the mesh prior to export -- unless, the user chooses the "None - Base mesh only" option in the export dialog.

Authored by Apple: Matt McLin

This PR adds support for exporting USD assets which have the subdivision schema applied. Current behavior prior to this PR is that the effects of Subdivision Surface modifiers are always applied to their mesh prior to export, such that it is not possible to recover the original base mesh. In this PR we provide three options for the subdiv schema type: None - Base mesh only (Export base mesh with subdiv type set to None) None - Subdivided mesh (Export subdivided mesh with subdiv type set to None) Best match (Export base mesh with subdiv schema type set to Catmull-Clark) <-- this is the default "Best match" here means that Blender will set a subdiv scheme type in the exported USD asset when it is possible to closely match the subdivision surface type that was authored in Blender. At this time Blender provides two subdivision types: Catmull-Clark and Simple. Because Simple does not have a corresponding subdivision type in USD, we do not attempt to convert or represent it, and instead the Simple subdiv modifier will be evaluated and applied to the mesh during export. Whenever a Catmull-Clark Subdivision Surface modifier is applied to an object, and is the last modifier in the stack, it is possible to set the subdiv scheme to Catmull-Clark for the respective prim in the exported USD file. In all other cases, any subdiv modifier will be evaluated and applied to the mesh prior to export -- unless, the user chooses the "None - Base mesh only" option in the export dialog. Authored by Apple: Matt McLin
Michael B Johnson added the
Interest
Pipeline, Assets & IO
Interest
USD
labels 2023-10-05 00:58:33 +02:00
Member

See attached subdivs.blend scene that we used for testing, and screenshots showing result of doing export for each of the three subdiv scheme options followed by importing back into Blender.

See attached subdivs.blend scene that we used for testing, and screenshots showing result of doing export for each of the three subdiv scheme options followed by importing back into Blender.
Brecht Van Lommel was assigned by Matt McLin 2023-10-05 01:57:16 +02:00
Michael Kowalski was assigned by Matt McLin 2023-10-05 01:57:16 +02:00
Member

Also screenshot of the export UI:

Also screenshot of the export UI:
Matt McLin changed title from dev/author_subdivs_v2 to USD: optionally author subdivision schema on export 2023-10-05 02:02:48 +02:00
Matt McLin added this to the USD project 2023-10-05 02:03:30 +02:00
Matt McLin requested review from Michael Kowalski 2023-10-05 02:12:27 +02:00
Brecht Van Lommel was unassigned by Matt McLin 2023-10-05 02:12:36 +02:00
Michael Kowalski was unassigned by Matt McLin 2023-10-05 02:12:37 +02:00
Michael Kowalski reviewed 2023-10-05 20:03:16 +02:00
@ -477,0 +495,4 @@
WM_reportf(RPT_WARNING, "USD export: Simple subdivision not supported, exporting subdivided mesh at render quality");
}
}

I wonder whether exporting normals should be skipped if the subdivision scheme is not set to "none". The USD docs seem to suggest this, if I'm reading them correctly:

Normals should not be authored on a subdivision mesh, since subdivision algorithms define their own normals. They should only be authored for polygonal meshes (subdivisionScheme = "none").

Please see https://openusd.org/dev/api/class_usd_geom_mesh.html#details.

If that's the case, then maybe the code can be reordered to create normals only if needed:

  /* Default to setting the subdivision scheme to None. */
  pxr::TfToken subdiv_scheme = pxr::UsdGeomTokens->none;
  
  if (subsurfData) {
    if (subsurfData->subdivType == SUBSURF_TYPE_CATMULL_CLARK) {
      if (usd_export_context_.export_params.export_subdiv == USD_SUBDIV_BESTMATCH) {
        /* If a subdivision modifier exists, and it uses Catmull-Clark, then apply Catmull-Clark SubD scheme. */
        subdiv_scheme = pxr::UsdGeomTokens->catmullClark;
      }
    } else {
      /* "Simple" is currently the only other subdivision type provided by Blender, */
      /* and we do not yet provide a corresponding representation for USD export. */
      WM_reportf(RPT_WARNING, "USD export: Simple subdivision not supported, exporting subdivided mesh at render quality");
    }
  }

  usd_mesh.CreateSubdivisionSchemeAttr().Set(subdiv_scheme);

  if (usd_export_context_.export_params.export_normals &&
      subdiv_scheme == pxr::UsdGeomTokens->none) {
    write_normals(mesh, usd_mesh);
  }

But please let me know if I'm misunderstanding this point.

I wonder whether exporting normals should be skipped if the subdivision scheme is not set to "none". The USD docs seem to suggest this, if I'm reading them correctly: > Normals should not be authored on a subdivision mesh, since subdivision algorithms define their own normals. They should only be authored for polygonal meshes (subdivisionScheme = "none"). Please see https://openusd.org/dev/api/class_usd_geom_mesh.html#details. If that's the case, then maybe the code can be reordered to create normals only if needed: ``` /* Default to setting the subdivision scheme to None. */ pxr::TfToken subdiv_scheme = pxr::UsdGeomTokens->none; if (subsurfData) { if (subsurfData->subdivType == SUBSURF_TYPE_CATMULL_CLARK) { if (usd_export_context_.export_params.export_subdiv == USD_SUBDIV_BESTMATCH) { /* If a subdivision modifier exists, and it uses Catmull-Clark, then apply Catmull-Clark SubD scheme. */ subdiv_scheme = pxr::UsdGeomTokens->catmullClark; } } else { /* "Simple" is currently the only other subdivision type provided by Blender, */ /* and we do not yet provide a corresponding representation for USD export. */ WM_reportf(RPT_WARNING, "USD export: Simple subdivision not supported, exporting subdivided mesh at render quality"); } } usd_mesh.CreateSubdivisionSchemeAttr().Set(subdiv_scheme); if (usd_export_context_.export_params.export_normals && subdiv_scheme == pxr::UsdGeomTokens->none) { write_normals(mesh, usd_mesh); } ``` But please let me know if I'm misunderstanding this point.
Member

Thanks for the reminder on this, makes sense.

Thanks for the reminder on this, makes sense.
makowalski marked this conversation as resolved
Brecht Van Lommel approved these changes 2023-10-05 20:22:51 +02:00
Brecht Van Lommel left a comment
Owner

I have not looked at the implementation yet, but for the UI I suggest:

Animation
Materials
Hair

UV Maps
Normals
Subdivision [ Ignore | Tessellate | Best Match ]

Root Prim Path
I have not looked at the implementation yet, but for the UI I suggest: ``` Animation Materials Hair UV Maps Normals Subdivision [ Ignore | Tessellate | Best Match ] Root Prim Path ```
@ -93,1 +93,4 @@
const EnumPropertyItem rna_enum_usd_export_subdiv_mode_items[] = {
{USD_SUBDIV_NONE,
"SUBDIV_NONE",

No need for SUBDIV_ prefix in the identifier.

No need for `SUBDIV_` prefix in the identifier.
Member

Thanks, how about this?

const EnumPropertyItem rna_enum_usd_export_subdiv_mode_items[] = {
    {USD_SUBDIV_IGNORE,
     "IGNORE",
     0,
     "Ignore",
     "SubD scheme = None, export base mesh without subdivision"},
    {USD_SUBDIV_TESSELLATE,
     "TESSELATE",
     0,
     "Tessellate",
     "SubD scheme = None, export subdivided mesh"},
    {USD_SUBDIV_BESTMATCH,
     "BESTMATCH",
     0,
     "Best Match",
     "Export base mesh with Catmull-Clark subdivision scheme when possible.  "
     "Reverts to exporting the subdivided mesh for the Simple subdivision type. "},
    {0, nullptr, 0, nullptr, nullptr},
};
Thanks, how about this? ``` const EnumPropertyItem rna_enum_usd_export_subdiv_mode_items[] = { {USD_SUBDIV_IGNORE, "IGNORE", 0, "Ignore", "SubD scheme = None, export base mesh without subdivision"}, {USD_SUBDIV_TESSELLATE, "TESSELATE", 0, "Tessellate", "SubD scheme = None, export subdivided mesh"}, {USD_SUBDIV_BESTMATCH, "BESTMATCH", 0, "Best Match", "Export base mesh with Catmull-Clark subdivision scheme when possible. " "Reverts to exporting the subdivided mesh for the Simple subdivision type. "}, {0, nullptr, 0, nullptr, nullptr}, }; ```

That looks fine. Just change BESTMATCH -> BEST_MATCH.

That looks fine. Just change BESTMATCH -> BEST_MATCH.
brecht marked this conversation as resolved
Brecht Van Lommel requested review from Brecht Van Lommel 2023-10-05 20:22:59 +02:00

(I did not mean to approve yet.)

(I did not mean to approve yet.)
Brecht Van Lommel requested changes 2023-10-05 20:28:35 +02:00
@ -0,0 +18,4 @@
/**
* This code is adapted from the SubdivModifierDisabler class
* implementation in the Alembic exporter.

Is it possible to move this code to io/common and share the implementation with Alembic?

Is it possible to move this code to `io/common` and share the implementation with Alembic?
Member

Will look into that

Will look into that
brecht marked this conversation as resolved
Brecht Van Lommel reviewed 2023-10-05 21:24:16 +02:00
@ -477,0 +487,4 @@
if (subsurfData->subdivType == SUBSURF_TYPE_CATMULL_CLARK) {
if (usd_export_context_.export_params.export_subdiv == USD_SUBDIV_BESTMATCH) {
/* If a subdivision modifier exists, and it uses Catmull-Clark, then apply Catmull-Clark SubD scheme. */
usd_mesh.CreateSubdivisionSchemeAttr().Set(pxr::UsdGeomTokens->catmullClark);

It should be possible to export interpolation settings in the Advanced panel too.

CreateInterpolateBoundaryAttr
CreateFaceVaryingLinearInterpolationAttr

Maybe the Creases boolean can be taken into account as well, to skip exporting the crease attributes.

It should be possible to export interpolation settings in the Advanced panel too. [CreateInterpolateBoundaryAttr](https://openusd.org/dev/api/class_usd_geom_mesh.html#aaec976d873f541bcb3748390b91f3746) [CreateFaceVaryingLinearInterpolationAttr](https://openusd.org/dev/api/class_usd_geom_mesh.html#a718e19018f593bb86896d21439ec124d) Maybe the Creases boolean can be taken into account as well, to skip exporting the crease attributes.
Member

Great points, Brecht, will investigate.

Great points, Brecht, will investigate.
brecht marked this conversation as resolved
Michael Kowalski reviewed 2023-10-05 23:40:41 +02:00
@ -373,3 +381,3 @@
};
void USDGenericMeshWriter::write_mesh(HierarchyContext &context, Mesh *mesh)
void USDGenericMeshWriter::write_mesh(HierarchyContext &context, Mesh *mesh, SubsurfModifierData* subsurfData)

I think SubsurfModifierData* subsurfData can be const.

I think `SubsurfModifierData* subsurfData` can be const.
Matt-McLin marked this conversation as resolved

I think we may want to fetch the subdiv modifier only if it's the last modifer AND is enabled for the current depsgraph evaluation mode (viewport or render).

Maybe the logic can be wrapped in a utility function similar to this (I haven't tested this extensively):

static const SubsurfModifierData *get_subsurf_modifier(Depsgraph *depsgraph, Object *obj)
{
  BLI_assert(obj);
  BLI_assert(depsgraph);

  /* Return the subdiv modifier if it is the last modifier and has
   * the required mode enabled. */

  ModifierData *md = (ModifierData *)(obj->modifiers.last);

  if (!md) {
    return nullptr;
  }

  /* Determine if the modifier is enabled for the current evaluation mode. */
  const eEvaluationMode eval_mode = DEG_get_mode(depsgraph);
  ModifierMode mod_mode = (eval_mode == DAG_EVAL_RENDER) ? eModifierMode_Render :
                                                           eModifierMode_Realtime;

  if ((md->mode & mod_mode) != mod_mode) {
    return nullptr;
  }

  if (md->type == eModifierType_Subsurf) {
    return reinterpret_cast<SubsurfModifierData *>(md);
  }

  return nullptr;
}

@brecht Do you think this makes sense?

I think we may want to fetch the subdiv modifier only if it's the last modifer AND is enabled for the current depsgraph evaluation mode (`viewport` or `render`). Maybe the logic can be wrapped in a utility function similar to this (I haven't tested this extensively): ``` static const SubsurfModifierData *get_subsurf_modifier(Depsgraph *depsgraph, Object *obj) { BLI_assert(obj); BLI_assert(depsgraph); /* Return the subdiv modifier if it is the last modifier and has * the required mode enabled. */ ModifierData *md = (ModifierData *)(obj->modifiers.last); if (!md) { return nullptr; } /* Determine if the modifier is enabled for the current evaluation mode. */ const eEvaluationMode eval_mode = DEG_get_mode(depsgraph); ModifierMode mod_mode = (eval_mode == DAG_EVAL_RENDER) ? eModifierMode_Render : eModifierMode_Realtime; if ((md->mode & mod_mode) != mod_mode) { return nullptr; } if (md->type == eModifierType_Subsurf) { return reinterpret_cast<SubsurfModifierData *>(md); } return nullptr; } ``` @brecht Do you think this makes sense?
Member

@makowalski About taking evaluation mode into account for viewport vs render, that is a good catch, thanks, and thanks for the suggested code snippet. I think I just need a small change on that. Instead of asking the depsgraph for the evaluation mode, I think I need to look at the export option, right?

usd_export_context_.export_params.evaluation_mode instead of const eEvaluationMode eval_mode = DEG_get_mode(depsgraph);

Or am I misunderstanding?

@makowalski About taking evaluation mode into account for viewport vs render, that is a good catch, thanks, and thanks for the suggested code snippet. I think I just need a small change on that. Instead of asking the depsgraph for the evaluation mode, I think I need to look at the export option, right? `usd_export_context_.export_params.evaluation_mode` instead of `const eEvaluationMode eval_mode = DEG_get_mode(depsgraph);` Or am I misunderstanding?

@makowalski About taking evaluation mode into account for viewport vs render, that is a good catch, thanks, and thanks for the suggested code snippet. I think I just need a small change on that. Instead of asking the depsgraph for the evaluation mode, I think I need to look at the export option, right?

usd_export_context_.export_params.evaluation_mode instead of const eEvaluationMode eval_mode = DEG_get_mode(depsgraph);

Or am I misunderstanding?

The evaluation mode is originally set on the depsgraph from the export params, so it amounts to the same thing. But, you are right, we don't need the depsgraph for anything else in this function, so it's cleaner to get the evaluation mode from the export params directly. (Originally, I thought the depsgraph would be needed to query the enabled state of the modifier, which is why I passed it in, but it turned out not to be the case and I forgot to change it. Sorry about that.)

> @makowalski About taking evaluation mode into account for viewport vs render, that is a good catch, thanks, and thanks for the suggested code snippet. I think I just need a small change on that. Instead of asking the depsgraph for the evaluation mode, I think I need to look at the export option, right? > > `usd_export_context_.export_params.evaluation_mode` instead of `const eEvaluationMode eval_mode = DEG_get_mode(depsgraph);` > > Or am I misunderstanding? > The evaluation mode is originally set on the depsgraph from the export params, so it amounts to the same thing. But, you are right, we don't need the depsgraph for anything else in this function, so it's cleaner to get the evaluation mode from the export params directly. (Originally, I thought the depsgraph would be needed to query the enabled state of the modifier, which is why I passed it in, but it turned out not to be the case and I forgot to change it. Sorry about that.)
Matt McLin changed title from USD: optionally author subdivision schema on export to WIP: USD: optionally author subdivision schema on export 2023-10-06 17:30:23 +02:00
Michael B Johnson force-pushed dev/author_subdivs_v2 from e7b0d3b04f to d350ce3b3d 2023-10-23 19:05:34 +02:00 Compare
Member

Using the code in this PR, I exported a test asset which exposes all six combinations of UV and geometry smoothing/interpolation options, so that the resulting USD output can be compared across renderers/DCCs. See attached.

Using the code in this PR, I exported a test asset which exposes all six combinations of UV and geometry smoothing/interpolation options, so that the resulting USD output can be compared across renderers/DCCs. See attached.
Brecht Van Lommel requested changes 2023-10-24 14:11:00 +02:00
Brecht Van Lommel left a comment
Owner

Thanks, much closer now.

Thanks, much closer now.
@ -94,0 +98,4 @@
"Ignore",
"SubD scheme = None, export base mesh without subdivision"},
{USD_SUBDIV_TESSELLATE,
"TESSELATE",

TESSELATE -> TESSELLATE

TESSELATE -> TESSELLATE
brecht marked this conversation as resolved
@ -336,1 +359,4 @@
"material assignments as geometry subsets");
RNA_def_enum(ot->srna,
"export_subdiv",

Don't abbreviate: export_subdivision

Don't abbreviate: `export_subdivision`
brecht marked this conversation as resolved
@ -337,0 +362,4 @@
"export_subdiv",
rna_enum_usd_export_subdiv_mode_items,
USD_SUBDIV_BEST_MATCH,
"SubD Scheme", /* shortening "Subdivison surface" to "SubD" so this fits in the UI */

SubD Scheme -> Subdivision

We don't use the term "SubD" in the Blender interface,.

SubD Scheme -> Subdivision We don't use the term "SubD" in the Blender interface,.
brecht marked this conversation as resolved
@ -59,0 +62,4 @@
BLI_assert(obj);
/* Return the subdiv modifier if it is the last modifier and has
* the required mode enabled. */

This seems inconsistent with SubdivModifierDisabler::get_subsurf_modifier, which skips displacement and particle system modifiers.

Can we unify this code into a single function?

Ideally code would also be unified with Alembic export and moved into io/common

This seems inconsistent with `SubdivModifierDisabler::get_subsurf_modifier`, which skips displacement and particle system modifiers. Can we unify this code into a single function? Ideally code would also be unified with Alembic export and moved into `io/common`
brecht marked this conversation as resolved
@ -481,0 +517,4 @@
} else {
/* "Simple" is currently the only other subdivision type provided by Blender, */
/* and we do not yet provide a corresponding representation for USD export. */
WM_reportf(RPT_WARNING, "USD export: Simple subdivision not supported, exporting subdivided mesh");

#113883 recently eliminated all the usage of WM_report in this code, this will need to be changed as well.

#113883 recently eliminated all the usage of `WM_report` in this code, this will need to be changed as well.
brecht marked this conversation as resolved
Michael B Johnson changed title from WIP: USD: optionally author subdivision schema on export to WIP: USD: optionally author subdivision schema on export 2023-10-26 20:16:06 +02:00
wave changed target branch from main to blender-v4.0-release 2023-10-26 20:16:11 +02:00
Michael B Johnson changed title from WIP: USD: optionally author subdivision schema on export to WIP: USD: optionally author subdivision schema on export 2023-10-26 20:17:08 +02:00
wave changed target branch from blender-v4.0-release to main 2023-10-26 20:17:10 +02:00
Michael B Johnson added 1 commit 2023-12-04 17:54:56 +01:00
Michael B Johnson changed title from WIP: USD: optionally author subdivision schema on export to USD: optionally author subdivision schema on export 2023-12-04 18:03:17 +01:00
Author
Member

We believe we have addressed the issues raised, and we no longer consider this a WIP.

We believe we have addressed the issues raised, and we no longer consider this a WIP.
Matt McLin requested review from Brecht Van Lommel 2023-12-04 18:05:11 +01:00
Brecht Van Lommel added 1 commit 2023-12-04 18:16:55 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
a6d9a7016d
Merge branch 'main' into HEAD

@blender-bot build

@blender-bot build
Brecht Van Lommel approved these changes 2023-12-04 18:17:30 +01:00
Brecht Van Lommel left a comment
Owner

Looks good to me now.

Looks good to me now.
Jesse Yurkovich requested changes 2023-12-05 03:01:18 +01:00
Jesse Yurkovich left a comment
Member

Generally works well, just a few comments below, and one outstanding question related to the frame_has_been_written_ condition.

Generally works well, just a few comments below, and one outstanding question related to the `frame_has_been_written_` condition.
@ -94,0 +96,4 @@
"IGNORE",
0,
"Ignore",
"SubD scheme = None, export base mesh without subdivision"},

"SubD" -> "Subdivision" here and the other enum below since these are user-facing strings.

"SubD" -> "Subdivision" here and the other enum below since these are user-facing strings.
Member

Thanks for catching, I thought I had already changed this

Thanks for catching, I thought I had already changed this
Matt-McLin marked this conversation as resolved
@ -94,0 +106,4 @@
"BEST_MATCH",
0,
"Best Match",
"Export base mesh with Catmull-Clark subdivision scheme when possible. "

To match the format of the text above. How about:

Subdivision scheme = Catmull-Clark, when possible. Reverts to exporting the subdivided mesh for the Simple subdivision type

Also note that, due to a quirk of how these strings get processed. The last sentence should not end with a period(.)

To match the format of the text above. How about: ``` Subdivision scheme = Catmull-Clark, when possible. Reverts to exporting the subdivided mesh for the Simple subdivision type ``` Also note that, due to a quirk of how these strings get processed. The last sentence should _not_ end with a period(`.`)
Matt-McLin marked this conversation as resolved
@ -0,0 +45,4 @@
* in the list or if it's the last modifier preceding any particle system modifiers.
* This function ignores Simple subsurf modifiers.
*/
static ModifierData *get_subsurf_modifier(Scene *scene, const Object *ob, ModifierMode mode);

Let's continue to use subdiv in the api names/comments as much as possible. the subsurf term is legacy. Use get_subdiv_modifier and subdiv in comments rather than subsurf.

Let's continue to use subdiv in the api names/comments as much as possible. the subsurf term is legacy. Use `get_subdiv_modifier` and `subdiv` in comments rather than `subsurf`.
Matt-McLin marked this conversation as resolved
@ -56,18 +56,51 @@ bool USDGenericMeshWriter::is_supported(const HierarchyContext *context) const
return true;
}
static const SubsurfModifierData *get_subsurf_modifier(eEvaluationMode eval_mode, Object *obj)

Subsurf to subdiv in api names

Subsurf to subdiv in api names
Matt-McLin marked this conversation as resolved
@ -64,1 +92,4 @@
/* Only fetch the subdiv modifier if it is the last modifier, */
/* and is enabled for the selected evaluation mode. */
const SubsurfModifierData *subsurfData = get_subsurf_modifier(

Move this block into the try below as subsurfData is only used within that scope

Move this block into the `try` below as subsurfData is only used within that scope
Matt-McLin marked this conversation as resolved
@ -476,7 +507,74 @@ void USDGenericMeshWriter::write_mesh(HierarchyContext &context, Mesh *mesh)
return;

I wonder... do we need to keep the subdiv+normals writing before this frame_has_been_written_ condition? There's a few usdview warnings if I attempt an animation while keyframing SubD modifier enable: Invalid Hydra prim '/root/Plane_002/Plane_002': # of facevaryings mismatch (64 != 24) for primvar normals

These go away if subdiv+normals are written before the condition here. All that said, keyframing the enable buttons on viewport/render don't really do what I thought they'd do. Maybe look at that after the initial patch lands. Or maybe my understanding of what this condition protects is wrong.

I wonder... do we need to keep the subdiv+normals writing before this `frame_has_been_written_` condition? There's a few `usdview` warnings if I attempt an animation while keyframing SubD modifier enable: `Invalid Hydra prim '/root/Plane_002/Plane_002': # of facevaryings mismatch (64 != 24) for primvar normals` These go away if subdiv+normals are written before the condition here. All that said, keyframing the enable buttons on viewport/render don't really do what I thought they'd do. Maybe look at that after the initial patch lands. Or maybe my understanding of what this condition protects is wrong.
Member

Hi @deadpin , our assumption here is that the Subdivision scheme will never be animated, as we are not aware of any use cases where this would be desirable. I suggest we take this as a later update if such a use case arises.

Hi @deadpin , our assumption here is that the Subdivision scheme will never be animated, as we are not aware of any use cases where this would be desirable. I suggest we take this as a later update if such a use case arises.
Matt-McLin marked this conversation as resolved
@ -477,3 +508,3 @@
}
usd_mesh.CreateSubdivisionSchemeAttr().Set(pxr::UsdGeomTokens->none);
/* Default to setting the subdivision scheme to None. */

We should take this large block and factor it out to a write_subdiv_and_normals private method to help keep things organized.

We should take this large block and factor it out to a `write_subdiv_and_normals` private method to help keep things organized.
Matt-McLin marked this conversation as resolved
Michael Kowalski approved these changes 2023-12-05 03:25:54 +01:00
Michael Kowalski left a comment
Member

The changes look good to me. Thank you!

I know Jesse has requested additional changes, but I have no more comments to add, so I am approving this pull request.

The changes look good to me. Thank you! I know Jesse has requested additional changes, but I have no more comments to add, so I am approving this pull request.
Michael B Johnson added 1 commit 2023-12-08 19:39:59 +01:00

Great! I think the only thing remaining is really the placement of the write_subdiv_and_normals call. In main the normals are always written before the if (frame_has_been_written_) condition. Now they are written afterwards, along with subdiv data. This will produce differences between files during animation. Even if subdiv is not animatable, the normals might be.

Great! I think the only thing remaining is really the placement of the `write_subdiv_and_normals` call. In `main` the normals are always written before the `if (frame_has_been_written_)` condition. Now they are written afterwards, along with subdiv data. This will produce differences between files during animation. Even if subdiv is not animatable, the normals might be.
Member

@deadpin Hi Jesse, I think this means we should probably revert the factoring out into write_subdiv_and_normals, and go with our original implementation for that, as the subdiv scheme and normals need to be treated differently.

It is important that the subdiv scheme is not written as animated. Per the USD spec it is uniform and cannot be animated, and also it would result in verbose USD file with schema being authored multiple times per timecode. So the subdivision scheme absolutely needs to be written after the if (frame_has_been_written_) check as we had it originally.

@deadpin Hi Jesse, I think this means we should probably revert the factoring out into `write_subdiv_and_normals`, and go with our original implementation for that, as the subdiv scheme and normals need to be treated differently. It is important that the subdiv scheme is not written as animated. Per the USD spec it is uniform and cannot be animated, and also it would result in verbose USD file with schema being authored multiple times per timecode. So the subdivision scheme absolutely needs to be written after the `if (frame_has_been_written_)` check as we had it originally.

@Matt-McLin I see, alright. I would expect something like the following then I suppose:

pxr::TfToken subdiv_scheme = get_subdiv_scheme(...);
if (usd_export_context_.export_params.export_normals &&
    subdiv_scheme == pxr::UsdGeomTokens->none) {
    write_normals(mesh, usd_mesh);
  }

...
<the frame_has_been_written_ condition>
...

write_subdiv(subdiv_scheme ...);
@Matt-McLin I see, alright. I would expect something like the following then I suppose: ``` pxr::TfToken subdiv_scheme = get_subdiv_scheme(...); if (usd_export_context_.export_params.export_normals && subdiv_scheme == pxr::UsdGeomTokens->none) { write_normals(mesh, usd_mesh); } ... <the frame_has_been_written_ condition> ... write_subdiv(subdiv_scheme ...); ```
Michael B Johnson added 1 commit 2023-12-15 19:57:17 +01:00
Matt McLin requested review from Jesse Yurkovich 2023-12-15 19:57:47 +01:00
Jesse Yurkovich approved these changes 2023-12-15 20:22:34 +01:00
Jesse Yurkovich left a comment
Member

LGTM and tests fine locally.

LGTM and tests fine locally.
Jesse Yurkovich merged commit 5edda6cbcc into main 2023-12-15 21:11:50 +01:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#113267
No description provided.