USD IO: Move to the new Mesh Attributes API for Colors #105347

Merged
Sybren A. Stüvel merged 11 commits from CharlesWardlaw/blender:feature/usd_colors into main 2023-04-14 11:05:38 +02:00

Porting https://archive.blender.org/developer/D16103

Original Summary:

This revision moves the vertex color reading and writing in the USD import and export functions over to the new Mesh Attributes API. I have removed anything else (new features or unnecessary changes) that was present in the prior patches to focus only on this task.

On the import side, I've introduced a class method named read_custom_data. In this function is the call-out for reading mesh colors. As requested, this function is intended to be the starting point for future Attribute reads, with methods like the new read_color_data* methods being called when a USD primvar matches a specific heuristic. UVs will (in the future, not in this revision) also need to be processed here. In a later patch, any primvars that do not match a heuristic can be imported as generic Attributes. There is a matching function on the export side, write_custom_data.

Attached is a .blend file for testing. The plane has five Color Attributes. The colors should be visibly the same when exported and re-imported.

I have also enabled color attribute imports by default. I believe it would be counter intuitive for most users for this feature to be off-- it means that at some point, a person round-tripping with default settings will lose data.

Porting https://archive.blender.org/developer/D16103 Original Summary: This revision moves the vertex color reading and writing in the USD import and export functions over to the new Mesh Attributes API. I have removed anything else (new features or unnecessary changes) that was present in the prior patches to focus only on this task. On the import side, I've introduced a class method named read_custom_data. In this function is the call-out for reading mesh colors. As requested, this function is intended to be the starting point for future Attribute reads, with methods like the new read_color_data* methods being called when a USD primvar matches a specific heuristic. UVs will (in the future, not in this revision) also need to be processed here. In a later patch, any primvars that do not match a heuristic can be imported as generic Attributes. There is a matching function on the export side, write_custom_data. Attached is a .blend file for testing. The plane has five Color Attributes. The colors should be visibly the same when exported and re-imported. I have also enabled color attribute imports by default. I believe it would be counter intuitive for most users for this feature to be off-- it means that at some point, a person round-tripping with default settings will lose data.
Charles Wardlaw added 1 commit 2023-03-01 17:55:01 +01:00
Iliya Katushenock changed title from Porting forward the Colors patch from https://archive.blender.org/developer/D16103 to Porting forward the Colors 2023-03-01 17:57:38 +01:00
Iliya Katushenock changed title from Porting forward the Colors to USD IO: Moving to the new Mesh Attributes API for Colors 2023-03-01 17:58:52 +01:00

You can refer to the original in the body of the description, the title, as well as the base of the description, can still be the same as in the original. We all moved on gitea and moved all our things

Also, it looks like you need to check your mail in git.

You can refer to the original in the body of the description, the title, as well as the base of the description, can still be the same as in the original. We all moved on gitea and moved all our things Also, it looks like you need to check your mail in git.
Hans Goudey changed title from USD IO: Moving to the new Mesh Attributes API for Colors to USD IO: Move to the new Mesh Attributes API for Colors 2023-03-01 18:01:45 +01:00
Charles Wardlaw changed title from USD IO: Move to the new Mesh Attributes API for Colors to USD IO: Moving to the new Mesh Attributes API for Colors 2023-03-01 22:23:42 +01:00
Author
Member

You can refer to the original in the body of the description, the title, as well as the base of the description, can still be the same as in the original. We all moved on gitea and moved all our things

Changed.

Also, it looks like you need to check your mail in git.

Why?

> You can refer to the original in the body of the description, the title, as well as the base of the description, can still be the same as in the original. We all moved on gitea and moved all our things Changed. > Also, it looks like you need to check your mail in git. Why?
Member

reposting from #104542

Does this handle mesh->active_color_attribute / mesh->default_color_attribute (e.g. when there were no color attributes there prior) ?

reposting from https://projects.blender.org/blender/blender/pulls/104542 Does this handle `mesh->active_color_attribute` / `mesh->default_color_attribute` (e.g. when there were no color attributes there prior) ?
Member

Also, it looks like you need to check your mail in git.

Why?

If this is about charleswardlaw@noreply.localhost (not sure it is), see https://blender.chat/channel/blender-coders?msg=SiPAk4e2aR2soFMAr

> > Also, it looks like you need to check your mail in git. > > Why? If this is about `charleswardlaw@noreply.localhost` (not sure it is), see https://blender.chat/channel/blender-coders?msg=SiPAk4e2aR2soFMAr
Sybren A. Stüvel requested changes 2023-03-02 11:31:09 +01:00
Sybren A. Stüvel left a comment
Member

The code looks pretty good, thanks!

Attached is a .blend file for testing. The plane has five Color Attributes. The colors should be visibly the same when exported and re-imported.

This should be done in a unit test, then.

The code looks pretty good, thanks! > Attached is a .blend file for testing. The plane has five Color Attributes. The colors should be visibly the same when exported and re-imported. This should be done in a unit test, then.
@ -76,0 +77,4 @@
Mesh *mesh,
double motionSampleTime);
void read_color_data_all(Mesh *mesh, double motionSampleTime);

Now there is ..._data and ..._data_all. It's not immediately clear from the naming how these relate. read_color_data_primvar and read_color_data_all_primvars would be better, but if you feel that's getting too long, documentation of what they do would already help a lot.

Now there is `..._data` and `..._data_all`. It's not immediately clear from the naming how these relate. `read_color_data_primvar` and `read_color_data_all_primvars` would be better, but if you feel that's getting too long, documentation of what they do would already help a lot.
CharlesWardlaw marked this conversation as resolved
@ -36,1 +38,4 @@
void write_surface_velocity(const Mesh *mesh, pxr::UsdGeomMesh usd_mesh);
void write_custom_data(const Mesh *mesh, pxr::UsdGeomMesh usd_mesh);

At the moment write_custom_data() only writes color data, meaning that this function is strongly related to write_color_data(). Remove the empty line between the two to show this relation more visually.

At the moment `write_custom_data()` only writes color data, meaning that this function is strongly related to `write_color_data()`. Remove the empty line between the two to show this relation more visually.
CharlesWardlaw marked this conversation as resolved
Matt McLin self-assigned this 2023-03-17 19:38:46 +01:00
Matt McLin removed their assignment 2023-03-17 19:39:21 +01:00
Charles Wardlaw added 3 commits 2023-03-30 22:58:03 +02:00
Charles Wardlaw added 2 commits 2023-03-31 00:18:17 +02:00
Member

reposting from #104542

Does this handle mesh->active_color_attribute / mesh->default_color_attribute (e.g. when there were no color attributes there prior) ?

I think this is still not handled?

> reposting from https://projects.blender.org/blender/blender/pulls/104542 > > Does this handle `mesh->active_color_attribute` / `mesh->default_color_attribute` (e.g. when there were no color attributes there prior) ? I think this is still not handled?
Author
Member

reposting from #104542

Does this handle mesh->active_color_attribute / mesh->default_color_attribute (e.g. when there were no color attributes there prior) ?

I think this is still not handled?

https://projects.blender.org/CharlesWardlaw/blender/src/branch/feature/usd_colors/source/blender/io/usd/intern/usd_reader_mesh.cc#L462

It's handled after all color attributes are loaded. When you built my branch, did it not work for you? I'm testing by loading the Pixar Kitchen scene and then changing the regular viewport draw mode to Attribute to see the colors.

image

If that's not what you mean then I'd appreciate a clarification.

> > reposting from https://projects.blender.org/blender/blender/pulls/104542 > > > > Does this handle `mesh->active_color_attribute` / `mesh->default_color_attribute` (e.g. when there were no color attributes there prior) ? > > I think this is still not handled? https://projects.blender.org/CharlesWardlaw/blender/src/branch/feature/usd_colors/source/blender/io/usd/intern/usd_reader_mesh.cc#L462 It's handled after all color attributes are loaded. When you built my branch, did it not work for you? I'm testing by loading the Pixar Kitchen scene and then changing the regular viewport draw mode to Attribute to see the colors. ![image](/attachments/f473d13c-dda3-470d-9610-5c2216fb26a0) If that's not what you mean then I'd appreciate a clarification.
864 KiB
Charles Wardlaw added 1 commit 2023-03-31 16:17:08 +02:00
Member

@CharlesWardlaw : seeing handling active, but how about BKE_id_attributes_default_color_set?

Can build the patch on Monday (on my way out...)

@CharlesWardlaw : seeing handling active, but how about `BKE_id_attributes_default_color_set`? Can build the patch on Monday (on my way out...)
Charles Wardlaw added 1 commit 2023-04-01 21:27:23 +02:00
Author
Member

@CharlesWardlaw : seeing handling active, but how about BKE_id_attributes_default_color_set?

Can build the patch on Monday (on my way out...)

Is that just the little render camera next to the attribute name? If this is a requirement of the patch being accepted then I'd need to know how to test the addition.

That said, this patch has been sitting for a very long time. My preference would be to add anything else as a followup pull request.

> @CharlesWardlaw : seeing handling active, but how about `BKE_id_attributes_default_color_set`? > > > Can build the patch on Monday (on my way out...) Is that just the little render camera next to the attribute name? If this is a requirement of the patch being accepted then I'd need to know how to test the addition. That said, this patch has been sitting for a very long time. My preference would be to add anything else as a followup pull request.
Sybren A. Stüvel reviewed 2023-04-03 12:02:47 +02:00
Sybren A. Stüvel left a comment
Member

It's handled after all color attributes are loaded. When you built my branch, did it not work for you? I'm testing by loading the Pixar Kitchen scene and then changing the regular viewport draw mode to Attribute to see the colors.

Please put this kind of info in the PR description next time, it helps a lot when reviewing. Personally I'm not familiar with the new mesh attributes system, and knowing how you tested will speed up my review work significantly ;-)

The colors should be visibly the same when exported and re-imported.

Can you either make screenshots or describe what they should look like? Because if there's a bug that swaps red and green (just a simple hypothetical case) it could still come out the same after exporting & importing.

When I export the blend file to a usda file, I have these primvars for the colors:

color3f[] primvars:Face_Byte = [(0.5209956, 0.5209956, 0.5209956), (0.5209956, 0.5209956, 0.5209956), (0.5209956, 0.5209956, 0.5209956), (0.5209956, 0.5209956, 0.5209956)] (
    interpolation = "faceVarying"
)
color3f[] primvars:Face_Color = [(0, 0, 0.9999995), (0, 0, 0.99999946), (0, 0, 0.9999995), (0, 0, 0.99999815)] (
    interpolation = "faceVarying"
)
color3f[] primvars:Four_Color_Blend = [(1.0000001, 0, 0.0000015428262), (0, 0, 0.9999995), (0, 0.9999995, 2.8362777e-7), (0.99999875, 0.9999989, 0.9999998)] (
    interpolation = "vertex"
)
color3f[] primvars:Vertex_Byte = [(1, 0, 0), (0.47353154, 0, 0), (1, 0, 0), (0.98225087, 0, 0)] (
    interpolation = "vertex"
)
color3f[] primvars:Vertex_Color = [(0, 0.99999195, 2.8311965e-7), (0, 0.999995, 2.8312056e-7), (0, 0.99997807, 2.831157e-7), (0, 0.9999995, 2.8312178e-7)] (
    interpolation = "vertex"
)

The faceVarying primvars are, as expected, exported per face corner. Altering the USDA file so that each color is different and reimporting doesnt't result in any color gradients, though.

Also I feel that the test set of one face is good (because it made it simpler to figure this out) but also might not be enough to distinguish differences in handling of per-vertex and per-facecorner cases.

> It's handled after all color attributes are loaded. When you built my branch, did it not work for you? I'm testing by loading the Pixar Kitchen scene and then changing the regular viewport draw mode to Attribute to see the colors. Please put this kind of info in the PR description next time, it helps a lot when reviewing. Personally I'm not familiar with the new mesh attributes system, and knowing how you tested will speed up my review work significantly ;-) > The colors should be visibly the same when exported and re-imported. Can you either make screenshots or describe what they should look like? Because if there's a bug that swaps red and green (just a simple hypothetical case) it could still come out the same after exporting & importing. When I export the blend file to a usda file, I have these primvars for the colors: ```c color3f[] primvars:Face_Byte = [(0.5209956, 0.5209956, 0.5209956), (0.5209956, 0.5209956, 0.5209956), (0.5209956, 0.5209956, 0.5209956), (0.5209956, 0.5209956, 0.5209956)] ( interpolation = "faceVarying" ) color3f[] primvars:Face_Color = [(0, 0, 0.9999995), (0, 0, 0.99999946), (0, 0, 0.9999995), (0, 0, 0.99999815)] ( interpolation = "faceVarying" ) color3f[] primvars:Four_Color_Blend = [(1.0000001, 0, 0.0000015428262), (0, 0, 0.9999995), (0, 0.9999995, 2.8362777e-7), (0.99999875, 0.9999989, 0.9999998)] ( interpolation = "vertex" ) color3f[] primvars:Vertex_Byte = [(1, 0, 0), (0.47353154, 0, 0), (1, 0, 0), (0.98225087, 0, 0)] ( interpolation = "vertex" ) color3f[] primvars:Vertex_Color = [(0, 0.99999195, 2.8311965e-7), (0, 0.999995, 2.8312056e-7), (0, 0.99997807, 2.831157e-7), (0, 0.9999995, 2.8312178e-7)] ( interpolation = "vertex" ) ``` The `faceVarying` primvars are, as expected, exported per face corner. Altering the USDA file so that each color is different and reimporting doesnt't result in any color gradients, though. Also I feel that the test set of one face is good (because it made it simpler to figure this out) but also might not be enough to distinguish differences in handling of per-vertex and per-facecorner cases.
@ -457,1 +492,3 @@
std::cerr << "WARNING: display colors count mismatch\n" << std::endl;
pxr::TfToken interp = color_primvar.GetInterpolation();
if ((interp == pxr::UsdGeomTokens->faceVarying && usd_colors.size() != mesh->totloop) ||

For a future patch (because I feel it's more important to land this patch & improve upon it than to keep delaying further), I think it might be better to structure this something like:

int expected_size(const pxr::TfToken interp, const Mesh *mesh) {
  switch (interp) {
  case pxr::UsdGeomTokens->faceVarying:
  case pxr::UsdGeomTokens->varying:
    return mesh->totloop;
  case pxr::UsdGeomTokens->vertex:
    return mesh->totvert;
  case pxr::UsdGeomTokens->constant:
    return 1;
  case pxr::UsdGeomTokens->uniform:
    return mesh->totpoly;
  default:
    return 0;
  }
}

...

const int expect = expected_size(interp, mesh)
if (expect && usd_colors.size() != expected_size(interp, mesh)) { ... }

(update: also see my comment below about a way to take this even further)

For a future patch (because I feel it's more important to land this patch & improve upon it than to keep delaying further), I think it might be better to structure this something like: ```cpp int expected_size(const pxr::TfToken interp, const Mesh *mesh) { switch (interp) { case pxr::UsdGeomTokens->faceVarying: case pxr::UsdGeomTokens->varying: return mesh->totloop; case pxr::UsdGeomTokens->vertex: return mesh->totvert; case pxr::UsdGeomTokens->constant: return 1; case pxr::UsdGeomTokens->uniform: return mesh->totpoly; default: return 0; } } ... const int expect = expected_size(interp, mesh) if (expect && usd_colors.size() != expected_size(interp, mesh)) { ... } ``` (update: also see my comment below about a way to take this even further)
@ -466,3 +525,3 @@
}
MLoopCol *colors = static_cast<MLoopCol *>(cd_ptr);
if (ELEM(interp, pxr::UsdGeomTokens->constant, pxr::UsdGeomTokens->uniform)) {

For the code below, a refactor would be in order. The handling of the interpolation type is now interleaved through various conditionals, and that makes things hard to follow / extend.

Instead of my suggestion above, it could be nice to have a strategy pattern, where each interpolation type extends an abstract base class. The selection of the concrete class would be done with a switch like above, and then that class can handle the 'how many entries are expected?' queries as well as handling the actual filling of the mesh data.

For the code below, a refactor would be in order. The handling of the interpolation type is now interleaved through various conditionals, and that makes things hard to follow / extend. Instead of my suggestion above, it could be nice to have a strategy pattern, where each interpolation type extends an abstract base class. The selection of the concrete class would be done with a `switch` like above, and then that class can handle the 'how many entries are expected?' queries as well as handling the actual filling of the mesh data.
Hans Goudey changed title from USD IO: Moving to the new Mesh Attributes API for Colors to USD IO: Move to the new Mesh Attributes API for Colors 2023-04-03 14:00:59 +02:00
Member

Is that just the little render camera next to the attribute name? If this is a requirement of the patch being accepted then I'd need to know how to test the addition.

Exactly, testing if this is set should be enough (would prefer if this is done now rather than later)

> Is that just the little render camera next to the attribute name? If this is a requirement of the patch being accepted then I'd need to know how to test the addition. Exactly, testing if this is set should be enough (would prefer if this is done now rather than later)
Charles Wardlaw added 2 commits 2023-04-13 20:20:07 +02:00
Charles Wardlaw added 1 commit 2023-04-13 20:22:24 +02:00
Merge branch 'main' into feature/usd_colors
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
f25a5ea48d
Sybren A. Stüvel approved these changes 2023-04-14 10:27:57 +02:00
Sybren A. Stüvel left a comment
Member

👍

:+1:

@blender-bot build

@blender-bot build
Sybren A. Stüvel merged commit 33bfbb2a0c into main 2023-04-14 11:05:38 +02:00
Sybren A. Stüvel deleted branch feature/usd_colors 2023-04-14 11:05:39 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
4 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#105347
No description provided.