USD Import: USD Shapes Support #104707

Merged
Hans Goudey merged 6 commits from CharlesWardlaw/blender:D16344-usd-shapes-export into main 2023-02-13 19:49:35 +01:00

This patch adds the ability to import USD Shape primitives (Gprims). They are imported as Blender Meshes using the USD API to convert, so that they appear the same as they would in other applications. USD Shapes are important in many workflows, particularly in gaming, where they are used for stand-in geometry or for collision primitives.

Example file with the five types are attached, as is one for testing animated value changes.

This patch adds the ability to import USD Shape primitives (Gprims). They are imported as Blender Meshes using the USD API to convert, so that they appear the same as they would in other applications. USD Shapes are important in many workflows, particularly in gaming, where they are used for stand-in geometry or for collision primitives. Example file with the five types are attached, as is one for testing animated value changes.
Charles Wardlaw added 1 commit 2023-02-13 17:50:01 +01:00
Hans Goudey added this to the Pipeline, Assets & IO project 2023-02-13 18:10:03 +01:00
Hans Goudey requested changes 2023-02-13 18:19:16 +01:00
@ -1 +1 @@
Subproject commit 4331c8e76c2f42b9fd903716c333d6cdeaa5cebd
Subproject commit 8d04896a130c12eb440b7b9ac33bf9dc152506b5
Member

These commit hashes shouldn't be included in the PR.

These commit hashes shouldn't be included in the PR.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +1,256 @@
Member

Missing license header

Missing license header
Author
Member

Added.

Added.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +8,4 @@
#include "DNA_mesh_types.h"
#include "DNA_meshdata_types.h"
#include "DNA_object_types.h"
#include "DNA_space_types.h" /* for FILE_MAX */
Member

The comment says the header is added for FILE_MAX, but FILE_MAX isn't used here. Make sure the headers included here are actually necessary.

The comment says the header is added `for FILE_MAX`, but `FILE_MAX` isn't used here. Make sure the headers included here are actually necessary.
Author
Member

Removed.

Removed.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +37,4 @@
void USDShapeReader::create_object(Main *bmain, double motionSampleTime)
{
UNUSED_VARS(motionSampleTime);
Member

Change this to /*motionSampleTime*/ in the function arguments

Change this to `/*motionSampleTime*/` in the function arguments
Author
Member

Changed.

Changed.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +52,4 @@
if (read_mesh != mesh) {
/* FIXME: after 2.80; `mesh->flag` isn't copied by #BKE_mesh_nomain_to_mesh() */
/* read_mesh can be freed by BKE_mesh_nomain_to_mesh(), so get the flag before that happens. */
Member

This isn't true anymore, this logic with mesh->flag can be removed.

This isn't true anymore, this logic with `mesh->flag` can be removed.
Author
Member

Removed.

Removed.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +88,4 @@
}
bool USDShapeReader::read_mesh_values(double motionSampleTime,
pxr::VtVec3fArray &positions,
Member

Looks like this is missing clang format here

Looks like this is missing clang format here
Author
Member

Reformatted file.

Reformatted file.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +93,4 @@
pxr::VtIntArray &face_counts) const
{
const bool is_capsule = prim_.IsA<pxr::UsdGeomCapsule>();
const bool is_cylinder = prim_.IsA<pxr::UsdGeomCylinder>();
Member

No need for these separate boolean variables IMO, just put the check directly inside the if statements.

No need for these separate boolean variables IMO, just put the check directly inside the if statements.
Author
Member

Changed.

Changed.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +130,4 @@
return false;
}
struct Mesh *USDShapeReader::read_mesh(struct Mesh *existing_mesh,
Member

No need for the struct keyword in C++ code

No need for the struct keyword in C++ code
Author
Member

Removed.

Removed.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +142,4 @@
return existing_mesh;
}
UNUSED_VARS(read_flag);
Member

Same here with the unused arguments

Same here with the unused arguments
Author
Member

Changed.

Changed.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +171,4 @@
}
}
BKE_mesh_calc_edges(active_mesh, false, false);
Member

Why do you need to calculate the edges if you didn't change anything above?

Why do you need to calculate the edges if you didn't change anything above?
Author
Member

mesh_from_prim() does not calculate edges; I deferred their calculation to this spot.

mesh_from_prim() does not calculate edges; I deferred their calculation to this spot.
Member

Yes, but my point is, shouldn't it be in the if (active_mesh != existing_mesh) { where the corner vertices are actually set? Otherwise it looks like it's calculating edges unnecessarily.

Yes, but my point is, shouldn't it be in the `if (active_mesh != existing_mesh) {` where the corner vertices are actually set? Otherwise it looks like it's calculating edges unnecessarily.
Author
Member

No, because active_mesh was created by mesh_from_prim() which does not calculate the edges. They need to be calculated either way.

No, because `active_mesh` was created by `mesh_from_prim()` which does not calculate the edges. They need to be calculated either way.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +172,4 @@
}
BKE_mesh_calc_edges(active_mesh, false, false);
BKE_mesh_normals_tag_dirty(active_mesh);
Member

No need to tag normals dirty on a new mesh. You aren't changing the positions here anyway, so it's not the correct update tag if any was actually needed.

No need to tag normals dirty on a new mesh. You aren't changing the positions here anyway, so it's not the correct update tag if any was actually needed.
Author
Member

Seems to be fine now without-- removed.

Seems to be fine now without-- removed.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +178,4 @@
}
Mesh *USDShapeReader::mesh_from_prim(Mesh *existing_mesh, double motionSampleTime,
Member

Clang format!

Clang format!
Author
Member

Reformatted file.

Reformatted file.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +196,4 @@
existing_mesh, positions.size(), 0, 0, face_indices.size(), face_counts.size());
}
MutableSpan<float3> verts = active_mesh->vert_positions_for_write();
Member

For consistency, change the verts name to vert_positions

For consistency, change the `verts` name to `vert_positions`
Author
Member

Changed.

Changed.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +212,4 @@
const bool is_capsule = prim_.IsA<pxr::UsdGeomCapsule>();
const bool is_cylinder = prim_.IsA<pxr::UsdGeomCylinder>();
const bool is_cone = prim_.IsA<pxr::UsdGeomCone>();
const bool is_cube = prim_.IsA<pxr::UsdGeomCube>();
Member

Same here with the no need for separate variables

Same here with the no need for separate variables
Author
Member

Removed.

Removed.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +1,58 @@
Member

Missing license header

Missing license header
Author
Member

Added.

Added.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +33,4 @@
/* Read the pxr:UsdGeomMesh values and convert them to a Blender Mesh,
* also returning face_indices and counts for further loop processing. */
Mesh *mesh_from_prim(Mesh *existing_mesh, double motionSampleTime,
Member

Clang format! Please configure your editor to format when you save the file, it will save everyone's time.

Clang format! Please configure your editor to format when you save the file, it will save everyone's time.
Author
Member

Reformatted file.

Reformatted file.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +43,4 @@
void create_object(Main *bmain, double motionSampleTime) override;
void read_object_data(Main *bmain, double motionSampleTime) override;
struct Mesh *read_mesh(struct Mesh *existing_mesh,
Member

No need for these struct keywords in C++ code, even headers.

No need for these struct keywords in C++ code, even headers.
Author
Member

Removed.

Removed.
Author
Member

Removed.

Removed.
CharlesWardlaw marked this conversation as resolved
@ -0,0 +49,4 @@
const char **err_str) override;
bool is_time_varying();
virtual bool topology_changed(const Mesh * /* existing_mesh */, double /* motionSampleTime */)
Member

Consistency: /* existing_mesh */ -> /*existing_mesh*/, etc.

Consistency: `/* existing_mesh */` -> `/*existing_mesh*/`, etc.
Author
Member

Changed.

Changed.
CharlesWardlaw marked this conversation as resolved
Hans Goudey changed title from USD Import: USD Shapes Support. to USD Import: USD Shapes Support 2023-02-13 18:19:27 +01:00
Member

Is there any particular reason this is optional? Seems like it would be nice to avoid adding the option unless it's actually helpful (maybe it is!)

Is there any particular reason this is optional? Seems like it would be nice to avoid adding the option unless it's actually helpful (maybe it is!)
Charles Wardlaw added 1 commit 2023-02-13 18:52:05 +01:00
Author
Member

Is there any particular reason this is optional? Seems like it would be nice to avoid adding the option unless it's actually helpful (maybe it is!)

Some folks aren't going to want Shapes imported at all, or at all during certain stages in a pipeline. In a future patch I could see removing the option from the main file picker tool panel area, but I think it is necessary in the Operator for pipelines.

> Is there any particular reason this is optional? Seems like it would be nice to avoid adding the option unless it's actually helpful (maybe it is!) Some folks aren't going to want Shapes imported at all, or at all during certain stages in a pipeline. In a future patch I could see removing the option from the main file picker tool panel area, but I think it is necessary in the Operator for pipelines.
Hans Goudey reviewed 2023-02-13 19:02:21 +01:00
@ -0,0 +141,4 @@
MutableSpan<MPoly> polys = active_mesh->polys_for_write();
MutableSpan<MLoop> loops = active_mesh->loops_for_write();
int loop_index = 0;
Member

Declare loop_index at the smallest scope possible, right above the for (int i = 0; i < face_counts.size(); i++) { loop.

Declare `loop_index` at the smallest scope possible, right above the `for (int i = 0; i < face_counts.size(); i++) {` loop.
CharlesWardlaw marked this conversation as resolved
Charles Wardlaw added 1 commit 2023-02-13 19:09:32 +01:00
Charles Wardlaw added 1 commit 2023-02-13 19:27:54 +01:00
Charles Wardlaw added 1 commit 2023-02-13 19:32:54 +01:00
Hans Goudey approved these changes 2023-02-13 19:35:40 +01:00
Hans Goudey left a comment
Member

Thanks, looks good now. Just one suggestion for an early return and some missed changes from earlier inline.

Thanks, looks good now. Just one suggestion for an early return and some missed changes from earlier inline.
@ -0,0 +123,4 @@
return false;
}
Mesh *USDShapeReader::read_mesh(struct Mesh *existing_mesh,
Member

struct Mesh -> Mesh

`struct Mesh` -> `Mesh`
@ -0,0 +138,4 @@
/* Should have a good set of data by this point-- copy over. */
Mesh *active_mesh = mesh_from_prim(existing_mesh, motionSampleTime, face_indices, face_counts);
MutableSpan<MPoly> polys = active_mesh->polys_for_write();
Member

Suggestion here so the reader has fewer things to keep track of at the same time:

  Mesh *active_mesh = mesh_from_prim(existing_mesh, motionSampleTime, face_indices, face_counts);
  if (active_mesh == existing_mesh) {
    return existing_mesh;
  }

  MutableSpan<MPoly> polys = active_mesh->polys_for_write();
  MutableSpan<MLoop> loops = active_mesh->loops_for_write();

  const char should_smooth = prim_.IsA<pxr::UsdGeomCube>() ? 0 : ME_SMOOTH;

  int loop_index = 0;
  for (int i = 0; i < face_counts.size(); i++) {
    const int face_size = face_counts[i];

    MPoly &poly = polys[i];
    poly.loopstart = loop_index;
    poly.totloop = face_size;

    /* Don't smooth-shade cubes; we're not worrying about sharpness for Gprims. */
    poly.flag |= should_smooth;

    for (int f = 0; f < face_size; ++f, ++loop_index) {
      loops[loop_index].v = face_indices[loop_index];
    }
  }
  BKE_mesh_calc_edges(active_mesh, false, false);

  return active_mesh;
Suggestion here so the reader has fewer things to keep track of at the same time: ``` Mesh *active_mesh = mesh_from_prim(existing_mesh, motionSampleTime, face_indices, face_counts); if (active_mesh == existing_mesh) { return existing_mesh; } MutableSpan<MPoly> polys = active_mesh->polys_for_write(); MutableSpan<MLoop> loops = active_mesh->loops_for_write(); const char should_smooth = prim_.IsA<pxr::UsdGeomCube>() ? 0 : ME_SMOOTH; int loop_index = 0; for (int i = 0; i < face_counts.size(); i++) { const int face_size = face_counts[i]; MPoly &poly = polys[i]; poly.loopstart = loop_index; poly.totloop = face_size; /* Don't smooth-shade cubes; we're not worrying about sharpness for Gprims. */ poly.flag |= should_smooth; for (int f = 0; f < face_size; ++f, ++loop_index) { loops[loop_index].v = face_indices[loop_index]; } } BKE_mesh_calc_edges(active_mesh, false, false); return active_mesh; ```
CharlesWardlaw marked this conversation as resolved
@ -0,0 +47,4 @@
void create_object(Main *bmain, double /*motionSampleTime*/) override;
void read_object_data(Main *bmain, double motionSampleTime) override;
Mesh *read_mesh(struct Mesh *existing_mesh,
Member

struct Mesh -> Mesh

`struct Mesh` -> `Mesh`
CharlesWardlaw marked this conversation as resolved
Charles Wardlaw added 1 commit 2023-02-13 19:43:48 +01:00
Hans Goudey merged commit 72a85d976a into main 2023-02-13 19:49:35 +01:00
Bastien Montagne removed this from the Pipeline, Assets & IO project 2023-07-03 13:02:14 +02:00
Sign in to join this conversation.
No reviewers
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
No Milestone
No project
No Assignees
2 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#104707
No description provided.