USD IO: Move to the new Mesh Attributes API for Colors #105347
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#105347
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "CharlesWardlaw/blender:feature/usd_colors"
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?
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 forward the Colors patch from https://archive.blender.org/developer/D16103to Porting forward the ColorsPorting forward the Colorsto USD IO: Moving to the new Mesh Attributes API for ColorsYou 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.
USD IO: Moving to the new Mesh Attributes API for Colorsto USD IO: Move to the new Mesh Attributes API for ColorsUSD IO: Move to the new Mesh Attributes API for Colorsto USD IO: Moving to the new Mesh Attributes API for ColorsChanged.
Why?
reposting from #104542
Does this handle
mesh->active_color_attribute
/mesh->default_color_attribute
(e.g. when there were no color attributes there prior) ?If this is about
charleswardlaw@noreply.localhost
(not sure it is), see https://blender.chat/channel/blender-coders?msg=SiPAk4e2aR2soFMArThe code looks pretty good, thanks!
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
andread_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.@ -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 towrite_color_data()
. Remove the empty line between the two to show this relation more visually.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.
If that's not what you mean then I'd appreciate a clarification.
@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.
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 ;-)
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:
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:
(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.USD IO: Moving to the new Mesh Attributes API for Colorsto USD IO: Move to the new Mesh Attributes API for ColorsExactly, testing if this is set should be enough (would prefer if this is done now rather than later)
👍
@blender-bot build