blender
/
blender
134
398
Fork 580
Code Issues 6.3k Pull Requests 761 Projects 19 Wiki Activity

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
Conversation 12 Commits 9 Files Changed 12 +362 -135
Michael B Johnson commented 2023-10-05 00:58:03 +02:00
Member
Copy Link

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
Matt McLin commented 2023-10-05 01:55:41 +02:00
Member
Copy Link

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.
subdiv_scene_blender_native.png
1.9 MiB
subdivs.blend
887 KiB
subdiv_scene_import_of_usd_export.png
2.2 MiB
subdiv_scene_blender_native.png subdiv_scene_import_of_usd_export.png
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
Matt McLin commented 2023-10-05 02:01:36 +02:00
Member
Copy Link

Also screenshot of the export UI:

Also screenshot of the export UI:
usd_export_subdiv_option.png
177 KiB
usd_export_subdiv_option.png
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
source/blender/io/usd/intern/usd_writer_mesh.cc Outdated
@ -477,0 +495,4 @@
WM_reportf(RPT_WARNING, "USD export: Simple subdivision not supported, exporting subdivided mesh at render quality");
}
}
Michael Kowalski commented 2023-10-05 20:03:16 +02:00
Member
Copy Link

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.
Matt McLin commented 2023-10-05 21:00:32 +02:00
Member
Copy Link

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
Copy Link

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 ```
source/blender/editors/io/io_usd.cc Outdated
@ -93,1 +93,4 @@
const EnumPropertyItem rna_enum_usd_export_subdiv_mode_items[] = {
{USD_SUBDIV_NONE,
"SUBDIV_NONE",
Brecht Van Lommel commented 2023-10-05 20:19:51 +02:00
Owner
Copy Link

No need for SUBDIV_ prefix in the identifier.

No need for `SUBDIV_` prefix in the identifier.
Matt McLin commented 2023-10-05 20:58:40 +02:00
Member
Copy Link

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}, }; ```
Brecht Van Lommel commented 2023-10-05 21:21:19 +02:00
Owner
Copy Link

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
Brecht Van Lommel commented 2023-10-05 20:23:18 +02:00
Owner
Copy Link

(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
source/blender/io/usd/intern/usd_modifier_disabler.h Outdated
@ -0,0 +18,4 @@
/**
* This code is adapted from the SubdivModifierDisabler class
* implementation in the Alembic exporter.
Brecht Van Lommel commented 2023-10-05 20:27:59 +02:00
Owner
Copy Link

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?
Matt McLin commented 2023-10-05 23:31:23 +02:00
Member
Copy Link

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
source/blender/io/usd/intern/usd_writer_mesh.cc Outdated
@ -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);
Brecht Van Lommel commented 2023-10-05 21:17:53 +02:00
Owner
Copy Link

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.
Matt McLin commented 2023-10-06 00:14:43 +02:00
Member
Copy Link

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
source/blender/io/usd/intern/usd_writer_mesh.cc Outdated
@ -373,3 +381,3 @@
};
void USDGenericMeshWriter::write_mesh(HierarchyContext &context, Mesh *mesh)
void USDGenericMeshWriter::write_mesh(HierarchyContext &context, Mesh *mesh, SubsurfModifierData* subsurfData)
Michael Kowalski commented 2023-10-05 23:40:41 +02:00
Member
Copy Link

I think SubsurfModifierData* subsurfData can be const.

I think `SubsurfModifierData* subsurfData` can be const.
Matt-McLin marked this conversation as resolved
Michael Kowalski commented 2023-10-06 00:02:40 +02:00
Member
Copy Link

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?
subdiv_modifier_modes.PNG
14 KiB
usd_export_mode.PNG
10 KiB
subdiv_modifier_modes.PNG usd_export_mode.PNG
Matt McLin commented 2023-10-06 01:14:03 +02:00
Member
Copy Link

@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?
Michael Kowalski commented 2023-10-06 02:04:09 +02:00
Member
Copy Link

@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
Matt McLin commented 2023-10-23 19:58:40 +02:00
Member
Copy Link

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.
subdiv_interp_modes.usdz
757 KiB
Brecht Van Lommel requested changes 2023-10-24 14:11:00 +02:00
Brecht Van Lommel left a comment
Owner
Copy Link

Thanks, much closer now.

Thanks, much closer now.
source/blender/editors/io/io_usd.cc Outdated
@ -94,0 +98,4 @@
"Ignore",
"SubD scheme = None, export base mesh without subdivision"},
{USD_SUBDIV_TESSELLATE,
"TESSELATE",
Brecht Van Lommel commented 2023-10-24 14:01:01 +02:00
Owner
Copy Link

TESSELATE -> TESSELLATE

TESSELATE -> TESSELLATE
brecht marked this conversation as resolved
source/blender/editors/io/io_usd.cc Outdated
@ -336,1 +359,4 @@
"material assignments as geometry subsets");
RNA_def_enum(ot->srna,
"export_subdiv",
Brecht Van Lommel commented 2023-10-24 14:01:42 +02:00
Owner
Copy Link

Don't abbreviate: export_subdivision

Don't abbreviate: `export_subdivision`
brecht marked this conversation as resolved
source/blender/editors/io/io_usd.cc Outdated
@ -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 */
Brecht Van Lommel commented 2023-10-24 14:02:23 +02:00
Owner
Copy Link

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
source/blender/io/usd/intern/usd_writer_mesh.cc
@ -59,0 +62,4 @@
BLI_assert(obj);
/* Return the subdiv modifier if it is the last modifier and has
* the required mode enabled. */
Brecht Van Lommel commented 2023-10-24 14:08:52 +02:00
Owner
Copy Link

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
source/blender/io/usd/intern/usd_writer_mesh.cc Outdated
@ -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");
Brecht Van Lommel commented 2023-10-24 14:10:35 +02:00
Owner
Copy Link

#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
Michael B Johnson commented 2023-12-04 18:03:57 +01:00
Author
Member
Copy Link

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
Brecht Van Lommel commented 2023-12-04 18:17:11 +01:00
Owner
Copy Link

@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
Copy Link

Looks good to me now.

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

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.
👍 1
source/blender/editors/io/io_usd.cc Outdated
@ -94,0 +96,4 @@
"IGNORE",
0,
"Ignore",
"SubD scheme = None, export base mesh without subdivision"},
Jesse Yurkovich commented 2023-12-05 00:23:53 +01:00
Member
Copy Link

"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.
Matt McLin commented 2023-12-06 20:16:50 +01:00
Member
Copy Link

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
source/blender/editors/io/io_usd.cc Outdated
@ -94,0 +106,4 @@
"BEST_MATCH",
0,
"Best Match",
"Export base mesh with Catmull-Clark subdivision scheme when possible. "
Jesse Yurkovich commented 2023-12-05 00:27:21 +01:00
Member
Copy Link

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
source/blender/io/common/IO_subdiv_disabler.hh Outdated
@ -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);
Jesse Yurkovich commented 2023-12-05 00:34:17 +01:00
Member
Copy Link

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
source/blender/io/usd/intern/usd_writer_mesh.cc Outdated
@ -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)
Jesse Yurkovich commented 2023-12-05 01:22:47 +01:00
Member
Copy Link

Subsurf to subdiv in api names

Subsurf to subdiv in api names
Matt-McLin marked this conversation as resolved
source/blender/io/usd/intern/usd_writer_mesh.cc Outdated
@ -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(
Jesse Yurkovich commented 2023-12-05 01:47:35 +01:00
Member
Copy Link

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
source/blender/io/usd/intern/usd_writer_mesh.cc Outdated
@ -476,7 +507,74 @@ void USDGenericMeshWriter::write_mesh(HierarchyContext &context, Mesh *mesh)
return;
Jesse Yurkovich commented 2023-12-05 03:00:10 +01:00
Member
Copy Link

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.
Matt McLin commented 2023-12-06 21:30:36 +01:00
Member
Copy Link

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
source/blender/io/usd/intern/usd_writer_mesh.cc Outdated
@ -477,3 +508,3 @@
}
usd_mesh.CreateSubdivisionSchemeAttr().Set(pxr::UsdGeomTokens->none);
/* Default to setting the subdivision scheme to None. */
Jesse Yurkovich commented 2023-12-05 02:50:11 +01:00
Member
Copy Link

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
Copy Link

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.
👍 1
Michael B Johnson added 1 commit 2023-12-08 19:39:59 +01:00
Jesse Yurkovich commented 2023-12-09 06:02:40 +01:00
Member
Copy Link

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.
Matt McLin commented 2023-12-13 20:33:07 +01:00
Member
Copy Link

@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.
Jesse Yurkovich commented 2023-12-13 20:46:51 +01:00
Member
Copy Link

@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 ...); ```
👍 1
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
Copy Link

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.
Reviewers
No reviewers
Brecht Van Lommel
Michael Kowalski
Jesse Yurkovich
Labels
Clear labels   
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
This issue affects/is about backward or forward 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
Issues relating to Wayland windowing support.

  
Interest
Workbench

  
Interest: X11

Issues relating to Xorg/X11 support.

  
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
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
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 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
Interest: X11
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
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
USD
Assignees
Clear assignees
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.
Delete Branch "wave/blender_wave_Apple:dev/author_subdivs_v2"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?