USD IO: Generic Attributes Support #109518

Merged
Michael Kowalski merged 34 commits from CharlesWardlaw/blender:feature/generic_attributes into main 2023-08-11 23:47:24 +02:00

Part 2 of the patch I wrote moving USD over to the new Attributes API for Colors: #105347

This patch adds support for more types of generic Mesh Attributes. Attribute Types and Domains are converted to their USD counterparts where possible. For example, float Attributes used for modifying shader masks or int Attributes for grouping are now able to be round-tripped. Due to the differences in the two systems some conversions are necessary, but attempts were made to keep data loss to a minimum.

If you export to USDA, you'll find the Attributes get prefixed with a "primvar:" namespace; this is expected behavior and identifies the exported Attributes as different from other USD Schema.

Not supported:

  • Edge domain. There doesn't seem to be a proper conversion for this in USD. One exception is for creasing and sharpness, but if they are desired I can add them in a future patch.

For testing, I've been using the Einar grooming file: https://studio.blender.org/films/charge/?asset=6072

Part 2 of the patch I wrote moving USD over to the new Attributes API for Colors: https://projects.blender.org/blender/blender/pulls/105347 This patch adds support for more types of generic Mesh Attributes. Attribute Types and Domains are converted to their USD counterparts where possible. For example, float Attributes used for modifying shader masks or int Attributes for grouping are now able to be round-tripped. Due to the differences in the two systems some conversions are necessary, but attempts were made to keep data loss to a minimum. If you export to USDA, you'll find the Attributes get prefixed with a "primvar:" namespace; this is expected behavior and identifies the exported Attributes as different from other USD Schema. Not supported: - Edge domain. There doesn't seem to be a proper conversion for this in USD. One exception is for creasing and sharpness, but if they are desired I can add them in a future patch. For testing, I've been using the Einar grooming file: https://studio.blender.org/films/charge/?asset=6072
Charles Wardlaw added 10 commits 2023-06-29 16:24:33 +02:00
Hans Goudey requested changes 2023-06-30 15:27:06 +02:00
Hans Goudey left a comment
Member

Inline I mostly added comments about the structuring of the code. The most significant ones are about reducing duplication and the need for helper functions.

I do find it a bit strange that there's an option for "Attributes" in the importer, but not the exporter. Do you think it makes sense to have symmetry in the options there?

Just to be clear, I don't have much experience using USD, so I'm only looking at the code in this review.

attempts were made to keep data loss to a minimum.

Sounds good! You mention the edge domain as something that's unsupported in USD. It would be helpful if you could list the other data that's unsupported in USD or Blender.

Inline I mostly added comments about the structuring of the code. The most significant ones are about reducing duplication and the need for helper functions. I do find it a bit strange that there's an option for "Attributes" in the importer, but not the exporter. Do you think it makes sense to have symmetry in the options there? Just to be clear, I don't have much experience using USD, so I'm only looking at the code in this review. >attempts were made to keep data loss to a minimum. Sounds good! You mention the edge domain as something that's unsupported in USD. It would be helpful if you could list the other data that's unsupported in USD or Blender.
@ -186,6 +181,55 @@ USDMeshReader::USDMeshReader(const pxr::UsdPrim &prim,
{
}
static const int convert_usd_type_to_blender(const std::string usd_type)
Member

Should return std::optional<eCustomDataType>

Should return `std::optional<eCustomDataType>`
CharlesWardlaw marked this conversation as resolved
@ -188,1 +183,4 @@
static const int convert_usd_type_to_blender(const std::string usd_type)
{
static std::unordered_map<std::string, int> type_map{
Member

Typically blender::Map is preferred over std::unordered_map in Blender code. In the ValueTypeName header it also mentions that the strings shouldn't be used for comparison. Suggest this instead:

  static const Map<pxr::SdfValueTypeName, eCustomDataType> type_map = []() {
    Map<pxr::SdfValueTypeName, eCustomDataType> map;
    map.add_new(pxr::SdfValueTypeNames->FloatArray, CD_PROP_FLOAT);
    map.add_new(pxr::SdfValueTypeNames->Double, CD_PROP_FLOAT);
    map.add_new(pxr::SdfValueTypeNames->IntArray, CD_PROP_INT32);
    map.add_new(pxr::SdfValueTypeNames->Float2Array, CD_PROP_FLOAT2);
    map.add_new(pxr::SdfValueTypeNames->TexCoord2dArray, CD_PROP_FLOAT2);
    map.add_new(pxr::SdfValueTypeNames->TexCoord2fArray, CD_PROP_FLOAT2);
    map.add_new(pxr::SdfValueTypeNames->TexCoord2hArray, CD_PROP_FLOAT2);
    map.add_new(pxr::SdfValueTypeNames->TexCoord3dArray, CD_PROP_FLOAT2);
    map.add_new(pxr::SdfValueTypeNames->TexCoord3fArray, CD_PROP_FLOAT2);
    map.add_new(pxr::SdfValueTypeNames->TexCoord3hArray, CD_PROP_FLOAT2);
    map.add_new(pxr::SdfValueTypeNames->Float3Array, CD_PROP_FLOAT3);
    map.add_new(pxr::SdfValueTypeNames->Color3fArray, CD_PROP_COLOR);
    map.add_new(pxr::SdfValueTypeNames->Color3hArray, CD_PROP_COLOR);
    map.add_new(pxr::SdfValueTypeNames->Color3dArray, CD_PROP_COLOR);
    map.add_new(pxr::SdfValueTypeNames->StringArray, CD_PROP_STRING);
    map.add_new(pxr::SdfValueTypeNames->BoolArray, CD_PROP_BOOL);
    map.add_new(pxr::SdfValueTypeNames->QuatfArray, CD_PROP_QUATERNION);
    return map;
  }();

  const eCustomDataType *ptr = type_map.lookup_ptr(usd_type);

Map is made aware of the hash with this above in the Blender namespace:

namespace blender {
template<> struct DefaultHash<pxr::SdfValueTypeName> {
  uint64_t operator()(const pxr::SdfValueTypeName &value) const
  {
    return value.GetHash();
  }
};
}  // namespace blender
Typically `blender::Map` is preferred over `std::unordered_map` in Blender code. In the `ValueTypeName` header it also mentions that the strings shouldn't be used for comparison. Suggest this instead: ``` static const Map<pxr::SdfValueTypeName, eCustomDataType> type_map = []() { Map<pxr::SdfValueTypeName, eCustomDataType> map; map.add_new(pxr::SdfValueTypeNames->FloatArray, CD_PROP_FLOAT); map.add_new(pxr::SdfValueTypeNames->Double, CD_PROP_FLOAT); map.add_new(pxr::SdfValueTypeNames->IntArray, CD_PROP_INT32); map.add_new(pxr::SdfValueTypeNames->Float2Array, CD_PROP_FLOAT2); map.add_new(pxr::SdfValueTypeNames->TexCoord2dArray, CD_PROP_FLOAT2); map.add_new(pxr::SdfValueTypeNames->TexCoord2fArray, CD_PROP_FLOAT2); map.add_new(pxr::SdfValueTypeNames->TexCoord2hArray, CD_PROP_FLOAT2); map.add_new(pxr::SdfValueTypeNames->TexCoord3dArray, CD_PROP_FLOAT2); map.add_new(pxr::SdfValueTypeNames->TexCoord3fArray, CD_PROP_FLOAT2); map.add_new(pxr::SdfValueTypeNames->TexCoord3hArray, CD_PROP_FLOAT2); map.add_new(pxr::SdfValueTypeNames->Float3Array, CD_PROP_FLOAT3); map.add_new(pxr::SdfValueTypeNames->Color3fArray, CD_PROP_COLOR); map.add_new(pxr::SdfValueTypeNames->Color3hArray, CD_PROP_COLOR); map.add_new(pxr::SdfValueTypeNames->Color3dArray, CD_PROP_COLOR); map.add_new(pxr::SdfValueTypeNames->StringArray, CD_PROP_STRING); map.add_new(pxr::SdfValueTypeNames->BoolArray, CD_PROP_BOOL); map.add_new(pxr::SdfValueTypeNames->QuatfArray, CD_PROP_QUATERNION); return map; }(); const eCustomDataType *ptr = type_map.lookup_ptr(usd_type); ``` `Map` is made aware of the hash with this above in the Blender namespace: ``` namespace blender { template<> struct DefaultHash<pxr::SdfValueTypeName> { uint64_t operator()(const pxr::SdfValueTypeName &value) const { return value.GetHash(); } }; } // namespace blender ```
Author
Member

Changed; added new header usd_hash_types.h so the hashes required by blender::Map can be shared across translation units.

Changed; added new header usd_hash_types.h so the hashes required by blender::Map can be shared across translation units.
CharlesWardlaw marked this conversation as resolved
@ -189,0 +212,4 @@
return value->second;
}
static const int convert_usd_varying_to_blender(const std::string usd_domain)
Member

Should return std::optional<eAttrDomain> rather than using 0 as an error value

Should return `std::optional<eAttrDomain>` rather than using 0 as an error value
CharlesWardlaw marked this conversation as resolved
@ -189,0 +213,4 @@
}
static const int convert_usd_varying_to_blender(const std::string usd_domain)
{
Member

Pass strings with StringRef, this function currently duplicates the string argument

Pass strings with `StringRef`, this function currently duplicates the string argument
CharlesWardlaw marked this conversation as resolved
@ -469,3 +344,2 @@
void USDMeshReader::read_color_data_primvar(Mesh *mesh,
const pxr::UsdGeomPrimvar &color_primvar,
const pxr::UsdGeomPrimvar &primvar,
Member

Better to leave these name changes out of this PR. Cleanup and functional changes should be separate.

Better to leave these name changes out of this PR. Cleanup and functional changes should be separate.
Author
Member

Changed back.

Changed back.
CharlesWardlaw marked this conversation as resolved
@ -609,0 +486,4 @@
const StringRef primvar_name(primvar.StripPrimvarsName(primvar.GetName()).GetString());
pxr::VtArray<pxr::GfVec2d> usd_uvs;
if (!primvar.ComputeFlattened(&usd_uvs, motionSampleTime)) {
Member

It looks like ComputeFlattened can do type conversions from the underlying format, is that right? If so, might as well make this a 2D float vector instead of doubles, to reduce the temporary memory usage?

It looks like `ComputeFlattened` can do type conversions from the underlying format, is that right? If so, might as well make this a 2D float vector instead of doubles, to reduce the temporary memory usage?
CharlesWardlaw marked this conversation as resolved
@ -609,0 +495,4 @@
bke::MutableAttributeAccessor attributes = mesh->attributes_for_write();
bke::SpanAttributeWriter<float2> uv_data;
uv_data = attributes.lookup_or_add_for_write_only_span<float2>(primvar_name, ATTR_DOMAIN_CORNER);
Member

Declare the variable and assign its value in the same statement

Declare the variable and assign its value in the same statement
CharlesWardlaw marked this conversation as resolved
@ -609,0 +506,4 @@
for (int i = 0; i < usd_uvs.size(); i++) {
uv_data.span[i] = float2(usd_uvs[i][0], usd_uvs[i][1]);
;
Member

extra semicolon

extra semicolon
CharlesWardlaw marked this conversation as resolved
@ -609,0 +525,4 @@
template<typename T, typename U>
void copy_prim_array_to_blender_buffer(const pxr::UsdGeomPrimvar &primvar,
bke::SpanAttributeWriter<U> &buffer,
Member

Call SpanAttributeWriter variables "attribute" rather than "buffer", that's just more consistent with elsewhere in Blender and more aligned with the purpose of the type. If the span was passed separately, might make more sense to call that a "buffer".

Call `SpanAttributeWriter` variables "attribute" rather than "buffer", that's just more consistent with elsewhere in Blender and more aligned with the purpose of the type. If the span was passed separately, might make more sense to call _that_ a "buffer".
Author
Member

Renamed, and renamed functions to _attribute.

Renamed, and renamed functions to _attribute.
CharlesWardlaw marked this conversation as resolved
@ -609,0 +619,4 @@
switch (type) {
case CD_PROP_FLOAT: {
pxr::VtArray<float> primvar_array;
Member

A bunch of unused variables here

A bunch of unused variables here
CharlesWardlaw marked this conversation as resolved
@ -609,0 +625,4 @@
copy_prim_array_to_blender_buffer<float, float>(primvar, buffer, motionSampleTime);
break;
}
case CD_PROP_INT32: {
Member

I think it makes sense to structure this code a bit differently to avoid duplication and remove the need for helper functions that obfuscate the process:

  1. Retrieve the generic-type Blender attribute writer outside of the switch statement.
  2. Inside the switch statement, call one function that returns a VtArray of the pxr type
  3. Also inside the switch statement, call a function that takes VtArray and MutableSpan<T> arguments to convert from USD to Blender arrays. You can retrieve a typed span from a generic one with the .typed<T>() method.
  4. Implement that conversion function with a separate template function that converts a single value of the USD type to the Blender type

I think this way there will be no more code than necessary, and the process will be easier to read.

I think it makes sense to structure this code a bit differently to avoid duplication and remove the need for helper functions that obfuscate the process: 1. Retrieve the generic-type Blender attribute writer outside of the switch statement. 2. Inside the switch statement, call one function that returns a `VtArray` of the `pxr` type 3. Also inside the switch statement, call a function that takes `VtArray` and `MutableSpan<T>` arguments to convert from USD to Blender arrays. You can retrieve a typed span from a generic one with the `.typed<T>()` method. 4. Implement that conversion function with a separate template function that converts a single value of the USD type to the Blender type I think this way there will be no more code than necessary, and the process will be easier to read.
@ -791,0 +867,4 @@
}
WM_reportf(RPT_WARNING,
"+++ Reading primvar %s, mesh %s (%s > %s)",
Member

Looks like this report should be removed?

Looks like this report should be removed?
CharlesWardlaw marked this conversation as resolved
@ -20,14 +18,10 @@
#include "BKE_material.h"
#include "BKE_mesh.hh"
#include "BKE_mesh_wrapper.h"
#include "BKE_modifier.h"
Member

Fully support removing unused includes, but it's not really related to this PR, should probably be done separately. Feel free to just commit that if you have commit rights.

Fully support removing unused includes, but it's not really related to this PR, should probably be done separately. Feel free to just commit that if you have commit rights.
@ -84,0 +88,4 @@
return true;
}
/* Splitting out the if clauses here so that we're only switching
Member

Not sure this comment makes sense here, sounds more like a description of the change in this PR. And the pattern is clear from the code below.

Not sure this comment makes sense here, sounds more like a description of the change in this PR. And the pattern is clear from the code below.
CharlesWardlaw marked this conversation as resolved
@ -95,0 +157,4 @@
}
template<typename T>
const VArray<T> get_attribute_buffer(const Mesh *mesh,
Member

VArray is not really a "buffer," which usually refers to a contiguous block of memory. How about get_attribute_array?

`VArray` is not really a "buffer," which usually refers to a contiguous block of memory. How about `get_attribute_array`?
CharlesWardlaw marked this conversation as resolved
@ -95,0 +252,4 @@
if (prim_varying == empty_token || prim_attr_type == pxr::SdfValueTypeNames->Opaque) {
WM_reportf(RPT_WARNING,
"Mesh %s, Attribute %s cannot be converted to USD.",
Member
/home/hans/blender-git/blender/source/blender/io/usd/intern/usd_writer_mesh.cc: In member function ‘void blender::io::usd::USDGenericMeshWriter::write_generic_data(const Mesh*, pxrInternal_v0_23__pxrReserved__::UsdGeomMesh, const blender::bke::AttributeIDRef&, const blender::bke::AttributeMetaData&)’:
/home/hans/blender-git/blender/source/blender/io/usd/intern/usd_writer_mesh.cc:255:16: warning: format ‘%s’ expects argument of type ‘char*’, but argument 4 has type ‘blender::StringRef’ [-Wformat=]
  255 |                "Mesh %s, Attribute %s cannot be converted to USD.",
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  256 |                &mesh->id.name[2],
  257 |                attribute_id.name());
      |                ~~~~~~~~~~~~~~~~~~~
      |                                 |
      |                                 blender::StringRef
``` /home/hans/blender-git/blender/source/blender/io/usd/intern/usd_writer_mesh.cc: In member function ‘void blender::io::usd::USDGenericMeshWriter::write_generic_data(const Mesh*, pxrInternal_v0_23__pxrReserved__::UsdGeomMesh, const blender::bke::AttributeIDRef&, const blender::bke::AttributeMetaData&)’: /home/hans/blender-git/blender/source/blender/io/usd/intern/usd_writer_mesh.cc:255:16: warning: format ‘%s’ expects argument of type ‘char*’, but argument 4 has type ‘blender::StringRef’ [-Wformat=] 255 | "Mesh %s, Attribute %s cannot be converted to USD.", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 256 | &mesh->id.name[2], 257 | attribute_id.name()); | ~~~~~~~~~~~~~~~~~~~ | | | blender::StringRef ```
Author
Member

Changed to &mesh->id.name[2]

Changed to &mesh->id.name[2]
CharlesWardlaw marked this conversation as resolved
@ -95,0 +261,4 @@
pxr::UsdGeomPrimvar attribute_pv = pvApi.CreatePrimvar(
primvar_name, prim_attr_type, prim_varying);
switch (meta_data.data_type) {
Member

I'd suggest refactoring this in the same way as the importer, to reduce code duplication and to reduce the amount of templated code.

I'd suggest refactoring this in the same way as the importer, to reduce code duplication and to reduce the amount of templated code.
@ -95,0 +294,4 @@
break;
}
/*
* // This seems to be unsupported so far?
Member

The quaternion attribute type is just new, but it should be supported here if possible. Best to use math::Quaternion for the Blender type if possible.

The quaternion attribute type is just new, but it should be supported here if possible. Best to use `math::Quaternion` for the Blender type if possible.
@ -95,0 +314,4 @@
const bke::AttributeMetaData &meta_data,
const char *active_set_name)
{
static blender::StringRef default_active_name = "st";
Member
/home/hans/blender-git/blender/source/blender/io/usd/intern/usd_writer_mesh.cc:317:29: warning: variable ‘default_active_name’ set but not used [-Wunused-but-set-variable]
  317 |   static blender::StringRef default_active_name = "st";
      |                             ^~~~~~~~~~~~~~~~~~~
``` /home/hans/blender-git/blender/source/blender/io/usd/intern/usd_writer_mesh.cc:317:29: warning: variable ‘default_active_name’ set but not used [-Wunused-but-set-variable] 317 | static blender::StringRef default_active_name = "st"; | ^~~~~~~~~~~~~~~~~~~ ```
CharlesWardlaw marked this conversation as resolved
Charles Wardlaw added 1 commit 2023-07-04 21:57:58 +02:00
Charles Wardlaw added 1 commit 2023-07-05 17:49:53 +02:00
Charles Wardlaw added 1 commit 2023-07-05 22:09:43 +02:00
8d389069c6 More bug fixes:
- Switched "face" to "uniform" on usd writes-- face was incorrect.
- Removed material_index from writes (it's handled separately)
- Added checks for USD files with malformed primvar attributes (broken interpolation); broken interpolation no longer crashes during import.
Charles Wardlaw added 2 commits 2023-07-06 17:18:15 +02:00

Regarding conversion of face colors to face_corner colors (which was only mentioned in the chat afaict), talked a bit about it with @dfelinto.

Question is, do we want to automatically convert USD face color attributes to Blender face_corner color attributes, or keep them as face attributes in Blender.

  • Face color attributes are currently handled in Blender as 'basic' attributes, they are nor recognized as coloring data by higher-level tools and in the UI.
  • They are usable e.g. in geometry and shading nodes, but require some manual set-up to be used in any way.
  • They are not directly editable by the user (not paintable).

The ideal solution would probably be to add support for face color attributes in editors and tools in Blender (paint mode, etc.). @JosephEagar knows probably best how mu=ch work that would be? If that seems doable for 4.0, and there are people volunteering to work on it, then I would consider this "new" task as blocking for this patch, since it would then allow to import USD face colors as-is without any issue anymore.

Otherwise, I would by default convert to face_corner colors for the time being.

Having a (non-UI exposed) generic option to always import attributes into their original domain would probably also be a good-to-have improvement in general to the USD importer, for more advanced cases (like pipeline integration) where preserving original data as much as possible is more important?

Regarding conversion of face colors to face_corner colors (which was only mentioned in the chat afaict), talked a bit about it with @dfelinto. Question is, do we want to automatically convert USD face color attributes to Blender face_corner color attributes, or keep them as face attributes in Blender. - Face color attributes are currently handled in Blender as 'basic' attributes, they are nor recognized as coloring data by higher-level tools and in the UI. - They are usable e.g. in geometry and shading nodes, but require some manual set-up to be used in any way. - They are not directly editable by the user (not paintable). The ideal solution would probably be to add support for face color attributes in editors and tools in Blender (paint mode, etc.). @JosephEagar knows probably best how mu=ch work that would be? If that seems doable for 4.0, and there are people volunteering to work on it, then I would consider this "new" task as blocking for this patch, since it would then allow to import USD face colors as-is without any issue anymore. Otherwise, I would by default convert to face_corner colors for the time being. Having a (non-UI exposed) generic option to always import attributes into their original domain would probably also be a good-to-have improvement in general to the USD importer, for more advanced cases (like pipeline integration) where preserving original data as much as possible is more important?
Hans Goudey requested changes 2023-07-06 18:51:42 +02:00
Hans Goudey left a comment
Member

Here's what I mean with my comment about code duplication in the attribute conversion code. I implemented it but didn't test it here: 87cc81044a

Let me know if you have questions about it. I think the improvements are fairly clear though.

Here's what I mean with my comment about code duplication in the attribute conversion code. I implemented it but didn't test it here: https://projects.blender.org/HooglyBoogly/blender/commit/87cc81044a6455b6df8cf3f5626958c565fffd9e Let me know if you have questions about it. I think the improvements are fairly clear though.
@ -402,6 +410,7 @@ static int wm_usd_import_exec(bContext *C, wmOperator *op)
const bool read_mesh_uvs = RNA_boolean_get(op->ptr, "read_mesh_uvs");
const bool read_mesh_colors = RNA_boolean_get(op->ptr, "read_mesh_colors");
const bool read_mesh_attributes = RNA_boolean_get(op->ptr, "read_mesh_attributes");
Member
/home/hans/blender-git/blender/source/blender/editors/io/io_usd.cc:413:14: warning: unused variable ‘read_mesh_attributes’ [-Wunused-variable]
  413 |   const bool read_mesh_attributes = RNA_boolean_get(op->ptr, "read_mesh_attributes");
      |              ^~~~~~~~~~~~~~~~~~~~
``` /home/hans/blender-git/blender/source/blender/editors/io/io_usd.cc:413:14: warning: unused variable ‘read_mesh_attributes’ [-Wunused-variable] 413 | const bool read_mesh_attributes = RNA_boolean_get(op->ptr, "read_mesh_attributes"); | ^~~~~~~~~~~~~~~~~~~~ ```
CharlesWardlaw marked this conversation as resolved
@ -0,0 +1,23 @@
#ifndef BLENDER_USD_HASH_TYPES_H
Member

Missing license and copyright

Missing license and copyright
CharlesWardlaw marked this conversation as resolved
@ -0,0 +1,23 @@
#ifndef BLENDER_USD_HASH_TYPES_H
#define BLENDER_USD_HASH_TYPES_H
Member

Blender code uses #pragma once rather than include guards

Blender code uses `#pragma once` rather than include guards
CharlesWardlaw marked this conversation as resolved
@ -0,0 +3,4 @@
#include <pxr/base/tf/token.h>
#include <pxr/usd/sdf/valueTypeName.h>
Member

Compiler error here without this too #include "BLI_hash.hh"

Compiler error here without this too `#include "BLI_hash.hh"`
CharlesWardlaw marked this conversation as resolved
@ -90,0 +253,4 @@
meta_data.data_type);
if (!prim_varying.has_value() || !prim_attr_type.has_value()) {
WM_reportf(
Member
/home/hans/blender-git/blender/source/blender/io/usd/intern/usd_writer_mesh.cc:223:22: warning: format ‘%s’ expects a matching ‘char*’ argument [-Wformat=]
  223 |         RPT_WARNING, "Mesh %s, Attribute %s cannot be converted to USD.", &mesh->id.name[2]);
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
``` /home/hans/blender-git/blender/source/blender/io/usd/intern/usd_writer_mesh.cc:223:22: warning: format ‘%s’ expects a matching ‘char*’ argument [-Wformat=] 223 | RPT_WARNING, "Mesh %s, Attribute %s cannot be converted to USD.", &mesh->id.name[2]); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
CharlesWardlaw marked this conversation as resolved
Bastien Montagne added the
Interest
USD
Interest
Import Export
labels 2023-07-07 11:01:40 +02:00
Bastien Montagne added this to the USD project 2023-07-07 11:01:49 +02:00
Member

The ideal solution would probably be to add support for face color attributes in editors and tools in Blender (paint mode, etc.).

I will work on that this week, it shouldn't take long but I'm working on some other things too.

I would consider this "new" task as blocking for this patch

I'd request this not be considered blocking though. Not sure when this PR will finish up exactly, but as long as there isn't too much time in between, I think it's fine if the two changes don't land at the same time or in that order. It's a relatively minor problem compared with the scale of this new feature.

>The ideal solution would probably be to add support for face color attributes in editors and tools in Blender (paint mode, etc.). I will work on that this week, it shouldn't take long but I'm working on some other things too. >I would consider this "new" task as blocking for this patch I'd request this not be considered blocking though. Not sure when this PR will finish up exactly, but as long as there isn't too much time in between, I think it's fine if the two changes don't land at the same time or in that order. It's a relatively minor problem compared with the scale of this new feature.

It's not a 'relatively minor problem', you will get bug reports if users start not being able to easily/directly use their color attributes when importing USD data. Never break user experience/expected behavior if you can avoid it, even 'for a few weeks'.

Either face-level color attribute proper support is implemented before this patch lands, or this patch needs to bring back conversion to face corners for color attribute (and switch back to face ones later once proper support for it is in Blender).

Personally, I'd rather see the first option given a try.

It's not a 'relatively minor problem', you will get bug reports if users start not being able to easily/directly use their color attributes when importing USD data. Never break user experience/expected behavior if you can avoid it, even 'for a few weeks'. Either face-level color attribute proper support is implemented before this patch lands, or this patch needs to bring back conversion to face corners for color attribute (and switch back to face ones later once proper support for it is in Blender). Personally, I'd rather see the first option given a try.
Charles Wardlaw added 2 commits 2023-07-26 17:25:07 +02:00
e932ed6264 Addressing Hans' comments, part 3:
- Integrated a modified version of his template specialization setup
- Moved copy_prim_array_to_blender_attribute into the class for variable access.
- Fixed issues with winding left-hand vs right-hand winding order for face corner attributes (UVs)
- Added MOD_MESHSEQ_READ_ATTRIBUTES flag for the mesh cache modifier.
- Added sharp_edge and crease_edge to the list of skippable attributes to silence warnings (they are unsupported).
- Addressed a crash introduced by switching to optionals.
- Added copyright for the new header
- ran `make format`
Hans Goudey requested changes 2023-07-26 17:43:04 +02:00
Hans Goudey left a comment
Member

A few more small comments. Looking pretty good now.

Assessing my own time in the next couple weeks realistically, I doubt I will have time to finish the face color attribute painting. Still don't see that as blocking personally, but to move forward sooner rather than later, it may be best to add the conversion from face to corner into this PR if you're up for it. Other than that, the code seems close to me.

A few more small comments. Looking pretty good now. Assessing my own time in the next couple weeks realistically, I doubt I will have time to finish the face color attribute painting. Still don't see that as blocking personally, but to move forward sooner rather than later, it may be best to add the conversion from face to corner into this PR if you're up for it. Other than that, the code seems close to me.
@ -312,0 +316,4 @@
"export_mesh_colors",
true,
"Color Attributes",
"Include Mesh Color Attributes in the export");
Member

Mesh Color Attributes -> mesh color attributes

Same below. Described here: https://wiki.blender.org/wiki/Human_Interface_Guidelines/Writing_Style

`Mesh Color Attributes` -> `mesh color attributes` Same below. Described here: https://wiki.blender.org/wiki/Human_Interface_Guidelines/Writing_Style
Member

Missed the "same below" part in the description for "read_mesh_attributes"

Missed the "same below" part in the description for `"read_mesh_attributes"`
CharlesWardlaw marked this conversation as resolved
@ -609,0 +514,4 @@
}
bke::MutableAttributeAccessor attributes = mesh->attributes_for_write();
bke::SpanAttributeWriter<float2> uv_data;
Member

Declare variable and assign its value in the same statement

Declare variable and assign its value in the same statement
CharlesWardlaw marked this conversation as resolved
@ -791,0 +925,4 @@
pxr::UsdGeomTokens->faceVarying,
pxr::UsdGeomTokens->varying) &&
ELEM(type,
pxr::SdfValueTypeNames->TexCoord2dArray,
Member

Could you check convert_usd_type_to_blender and the domain conversion function instead of listing the types here again?

Could you check `convert_usd_type_to_blender` and the domain conversion function instead of listing the types here again?
CharlesWardlaw marked this conversation as resolved
@ -39,2 +34,4 @@
namespace blender::io::usd {
const pxr::TfToken empty_token;
const pxr::UsdTimeCode defaultTime = pxr::UsdTimeCode::Default();
Member

These two globals look unused (which is a good thing ;)

These two globals look unused (which is a good thing ;)
Author
Member

Empty Token was unused but defaultTime is not-- I removed empty_token.

Empty Token was unused but defaultTime is not-- I removed empty_token.
CharlesWardlaw marked this conversation as resolved
@ -83,1 +87,4 @@
[&](const bke::AttributeIDRef &attribute_id, const bke::AttributeMetaData &meta_data) {
/* Skipping "internal" Blender properties. Skipping
* material_index as it's dealt with elsewhere. Skipping
* edge sharp and crease because USD doesn't have a
Member

Wouldn't it make more sense to skip all edge attributes here? That's why sharp_edge and crease_edge are unsupported here, right? (though I think crease_edge is copied elsewhere in the exporter already)

Wouldn't it make more sense to skip all edge attributes here? That's why `sharp_edge` and `crease_edge` are unsupported here, right? (though I think `crease_edge` is copied elsewhere in the exporter already)
Author
Member

I'm not sure tbh. If that's what you'd prefer I can do.

I'm not sure tbh. If that's what you'd prefer I can do.
Member

It's just not clear why skipping sharp_edge and crease_edge is necessary here, when they won't be processed a later anyway because convert_blender_domain_to_usd will return std::nullopt. Especially those attributes in particular, since there may be other edge attributes

It's just not clear why skipping `sharp_edge` and `crease_edge` is necessary here, when they won't be processed a later anyway because `convert_blender_domain_to_usd` will return `std::nullopt`. Especially those attributes in particular, since there may be other edge attributes
Author
Member

Modified it to be more explicit that we're skipping the edge domain.

Modified it to be more explicit that we're skipping the edge domain.
CharlesWardlaw marked this conversation as resolved
@ -84,0 +89,4 @@
* material_index as it's dealt with elsewhere. Skipping
* edge sharp and crease because USD doesn't have a
* conversion for them. */
if (attribute_id.name()[0] == '.' ||
Member

Anonymous attributes from geometry nodes can be skipped here too, can be checked with attribute_id.is_anonymous()

Anonymous attributes from geometry nodes can be skipped here too, can be checked with `attribute_id.is_anonymous()`
CharlesWardlaw marked this conversation as resolved
@ -95,0 +212,4 @@
usd_value_writer_.SetAttribute(prim_attr, pxr::VtValue(data), timecode);
}
#pragma optimize("", off)
Member

Guess this is temporary for debugging?

Guess this is temporary for debugging?
CharlesWardlaw marked this conversation as resolved
@ -95,0 +234,4 @@
return;
}
if (!prim_varying || !prim_varying.has_value() || !prim_attr_type || !prim_attr_type.has_value())
Member

!prim_varying || !prim_varying.has_value() is the same as !prim_varying as far as I know. Better to shorten this then I think.

`!prim_varying || !prim_varying.has_value()` is the same as `!prim_varying` as far as I know. Better to shorten this then I think.
Author
Member

Now that it's an optional, there's a possibility that prim_varying is null, and attempting to access .has_value will cause a crash. I have to leave it like this to handle all cases.

Now that it's an optional, there's a possibility that prim_varying is null, and attempting to access .has_value will cause a crash. I have to leave it like this to handle all cases.
Member

Sorry, but I'm not sure this is right. Maybe the crash was coming from elsewhere? In my standard library implementation, the implicit bool conversion and has_value() are the same, and I'm pretty sure I remember using !optional_value in this way before. cppreference seems to say the same thing

      constexpr explicit operator bool() const noexcept
      { return this->_M_is_engaged(); }

      constexpr bool has_value() const noexcept
      { return this->_M_is_engaged(); }
Sorry, but I'm not sure this is right. Maybe the crash was coming from elsewhere? In my standard library implementation, the implicit bool conversion and `has_value()` are the same, and I'm pretty sure I remember using `!optional_value` in this way before. [cppreference](https://en.cppreference.com/w/cpp/utility/optional/operator_bool) seems to say the same thing ``` constexpr explicit operator bool() const noexcept { return this->_M_is_engaged(); } constexpr bool has_value() const noexcept { return this->_M_is_engaged(); } ```
Author
Member

Ahh looks like you're right-- I was confused about has_value being from the optional vs being from the pxr class. Changed.

Ahh looks like you're right-- I was confused about has_value being from the optional vs being from the pxr class. Changed.
CharlesWardlaw marked this conversation as resolved
@ -95,0 +247,4 @@
primvar_name, *prim_attr_type, *prim_varying);
switch (meta_data.data_type) {
case CD_PROP_FLOAT: {
Member

The curly brackets are unnecessary in this switch and make this function a bit long IMO

The curly brackets are unnecessary in this switch and make this function a bit long IMO
Author
Member

Removed-- they were a holdover from when I was grabbing a return value and wanted to use the same name in each scope.

Removed-- they were a holdover from when I was grabbing a return value and wanted to use the same name in each scope.
CharlesWardlaw marked this conversation as resolved
@ -95,0 +280,4 @@
break;
}
default:
BLI_assert_msg(0, "Unsupported domain for mesh data.");
Member

Unsupported domain -> Unsupported type

`Unsupported domain` -> `Unsupported type`
CharlesWardlaw marked this conversation as resolved
Charles Wardlaw added 2 commits 2023-07-26 18:52:55 +02:00
Charles Wardlaw added 1 commit 2023-07-26 20:31:55 +02:00
Charles Wardlaw added 1 commit 2023-07-26 20:33:19 +02:00
Hans Goudey reviewed 2023-07-26 20:34:27 +02:00
@ -95,0 +144,4 @@
case CD_PROP_QUATERNION:
return pxr::SdfValueTypeNames->QuatfArray;
default:
WM_reportf(RPT_WARNING, "Unsupported domain for mesh data.");
Member

domain -> type

`domain` -> `type`
CharlesWardlaw marked this conversation as resolved
Hans Goudey reviewed 2023-07-26 20:42:19 +02:00
@ -791,0 +906,4 @@
/* Read Color primvars. */
if (ELEM(type,
pxr::SdfValueTypeNames->Color3hArray,
Member

Looks like this could use convert_usd_type_to_blender

Looks like this could use `convert_usd_type_to_blender`
CharlesWardlaw marked this conversation as resolved
@ -791,0 +924,4 @@
/* Read UV primvars. */
else if (ELEM(varying_type,
pxr::UsdGeomTokens->vertex,
Member

Might as well use convert_blender_domain_to_usd to remove the duplication here too?

Might as well use `convert_blender_domain_to_usd` to remove the duplication here too?
Author
Member

In this case no-- the conversions don't necessarily match up and it could cause false positives.

In this case no-- the conversions don't necessarily match up and it could cause false positives.
CharlesWardlaw marked this conversation as resolved
@ -95,0 +268,4 @@
copy_blender_buffer_to_prim<float2, pxr::GfVec2f>(
attribute.typed<float2>(), timecode, attribute_pv);
break;
Member

This is picky I guess, but there's no real need for the blank lines between the cases here, doesn't add much, and doesn't save any space this way compared to the brackets :P

This is picky I guess, but there's no real need for the blank lines between the cases here, doesn't add much, and doesn't save any space this way compared to the brackets :P
Author
Member

I find it much harder to read, but I made the edit :p

I find it much harder to read, but I made the edit :p
CharlesWardlaw marked this conversation as resolved
Charles Wardlaw added 3 commits 2023-07-26 21:29:48 +02:00
Hans Goudey approved these changes 2023-07-26 21:44:54 +02:00
Hans Goudey left a comment
Member

Left some small comments, but it all basically looks good to me. Note I didn't do any testing though. I'm not familiar enough with using USD that my testing would be more useful than whatever you're doing.

It would be nice to have some automated test cases for attributes. I'm fine with that being done as a separate step though.

ED_geometry_attribute_convert can be used as a simple way to convert face colors to face corner colors.

Left some small comments, but it all basically looks good to me. Note I didn't do any testing though. I'm not familiar enough with using USD that my testing would be more useful than whatever you're doing. It would be nice to have some automated test cases for attributes. I'm fine with that being done as a separate step though. `ED_geometry_attribute_convert` can be used as a simple way to convert face colors to face corner colors.
@ -609,0 +518,4 @@
}
bke::MutableAttributeAccessor attributes = mesh->attributes_for_write();
bke::SpanAttributeWriter<float2> uv_data = attributes.lookup_or_add_for_write_only_span<float2>(primvar_name, ATTR_DOMAIN_CORNER);
Member

Missing clang format

Missing clang format
CharlesWardlaw marked this conversation as resolved
@ -609,0 +535,4 @@
for (const int i : faces.index_range()) {
const IndexRange face = faces[i];
for (int j = 0; j < face.size(); j++) {
const int rev_index = face.start() + face.size() - 1 - j;
Member

IndexRange handles a bit of this itself.

for (const int i : face.index_range()) {
   const int rev_index = face.last(i);
`IndexRange` handles a bit of this itself. ``` for (const int i : face.index_range()) { const int rev_index = face.last(i); ```
CharlesWardlaw marked this conversation as resolved
@ -609,0 +550,4 @@
/* Handle vertex interpolation. */
const Span<int> corner_verts = mesh->corner_verts();
BLI_assert(mesh->totvert == usd_uvs.size());
for (int i = 0; i < uv_data.span.size(); ++i) {
Member

If you'd like, this pattern is handled in the array_utils header--
array_utils::gather(Span(usd_uvs.data(), usd_uvs.size()), corner_verts, uv_data.span);
It'll also be multithreaded that way

If you'd like, this pattern is handled in the `array_utils` header-- `array_utils::gather(Span(usd_uvs.data(), usd_uvs.size()), corner_verts, uv_data.span);` It'll also be multithreaded that way
Author
Member

I can make this change in a future patch, looking at threading more cohesively. For now, the variant that compiled:

array_utils::gather(Span(usd_uvs.data(), usd_uvs.size()), IndexMask(usd_uvs.size()), uv_data.span, 4096);

seems to need some other define to satisfy the template.

I can make this change in a future patch, looking at threading more cohesively. For now, the variant that compiled: ```array_utils::gather(Span(usd_uvs.data(), usd_uvs.size()), IndexMask(usd_uvs.size()), uv_data.span, 4096);``` seems to need some other define to satisfy the template.
@ -609,0 +597,4 @@
if (ELEM(interp, pxr::UsdGeomTokens->constant, pxr::UsdGeomTokens->uniform)) {
/* For situations where there's only a single item, flood fill the object. */
attribute.fill(convert_value<USDT, BlenderT>(primvar_array[0]));
Member

Missing a return after this maybe?

Missing a return after this maybe?
Author
Member

They all fall off the end with the if / else clauses-- no return needed, I think.

They all fall off the end with the if / else clauses-- no return needed, I think.
CharlesWardlaw marked this conversation as resolved
@ -609,0 +607,4 @@
for (const int i : faces.index_range()) {
const IndexRange face = faces[i];
for (int j = 0; j < face.size(); j++) {
const int rev_index = face.start() + face.size() - 1 - j;
Member

Same comment here about IndexRange handling the reversing

Same comment here about `IndexRange` handling the reversing
CharlesWardlaw marked this conversation as resolved
@ -609,0 +663,4 @@
copy_prim_array_to_blender_attribute<int32_t>(
mesh, primvar, motionSampleTime, attribute.span.typed<int>());
break;
Member

For consistency, no need for the blank lines here too. The switch cases are all doing the same thing, there isn't much reason to separate them.

For consistency, no need for the blank lines here too. The switch cases are all doing the same thing, there isn't much reason to separate them.
CharlesWardlaw marked this conversation as resolved
@ -134,3 +344,2 @@
default:
BLI_assert_msg(0, "Invalid domain for mesh color data.");
return;
BLI_assert_msg(0, "Invalid type for mesh color data.");
Member

type -> domain

`type` -> `domain`
CharlesWardlaw marked this conversation as resolved
Charles Wardlaw added 1 commit 2023-08-10 16:47:49 +02:00
Charles Wardlaw added 5 commits 2023-08-10 23:10:03 +02:00
Charles Wardlaw added 1 commit 2023-08-10 23:22:25 +02:00
Hans Goudey approved these changes 2023-08-11 08:20:56 +02:00
Hans Goudey left a comment
Member

Just noticed three tiny things, but they definitely don't need another review iteration and the code looks good to go from my POV.

Just noticed three tiny things, but they definitely don't need another review iteration and the code looks good to go from my POV.
@ -0,0 +1,26 @@
/* SPDX-FileCopyrightText: 2023 Blender Foundation
Member

Looks like this header should be added to source/blender/io/usd/CMakeLists.txt, right before intern/usd_hierarchy_iterator.h I guess.

Looks like this header should be added to `source/blender/io/usd/CMakeLists.txt`, right before `intern/usd_hierarchy_iterator.h` I guess.
@ -188,0 +216,4 @@
const eAttrDomain *value = domain_map.lookup_ptr(usd_domain);
if (value == nullptr) {
WM_reportf(RPT_WARNING, "Unsupported domain for mesh data type %s.", usd_domain.GetText());
Member

Looks like most error messages in Blender don't end with a period, might as well be consistent here and above.

Looks like most error messages in Blender don't end with a period, might as well be consistent here and above.
@ -608,0 +511,4 @@
(varying_type == pxr::UsdGeomTokens->varying && usd_uvs.size() != mesh->totloop))
{
WM_reportf(RPT_WARNING,
"USD Import: uv attribute value '%s' count inconsistent with interpolation type",
Member

Capitalize UV

Capitalize `UV`
Michael Kowalski reviewed 2023-08-11 16:04:57 +02:00
@ -188,0 +177,4 @@
map.add_new(pxr::SdfValueTypeNames->TexCoord3dArray, CD_PROP_FLOAT2);
map.add_new(pxr::SdfValueTypeNames->TexCoord3fArray, CD_PROP_FLOAT2);
map.add_new(pxr::SdfValueTypeNames->TexCoord3hArray, CD_PROP_FLOAT2);
map.add_new(pxr::SdfValueTypeNames->Float3Array, CD_PROP_FLOAT3);

@CharlesWardlaw Can we also handle USD Vector3 types here?

    map.add_new(pxr::SdfValueTypeNames->Vector3fArray, CD_PROP_FLOAT3);
    map.add_new(pxr::SdfValueTypeNames->Vector3hArray, CD_PROP_FLOAT3);
    map.add_new(pxr::SdfValueTypeNames->Vector3dArray, CD_PROP_FLOAT3);
@CharlesWardlaw Can we also handle USD Vector3 types here? ``` map.add_new(pxr::SdfValueTypeNames->Vector3fArray, CD_PROP_FLOAT3); map.add_new(pxr::SdfValueTypeNames->Vector3hArray, CD_PROP_FLOAT3); map.add_new(pxr::SdfValueTypeNames->Vector3dArray, CD_PROP_FLOAT3); ```
Charles Wardlaw added 3 commits 2023-08-11 23:00:09 +02:00

@blender-bot build

@blender-bot build
Michael Kowalski merged commit cf5666345d into main 2023-08-11 23:47:24 +02:00
Bastien Montagne removed this from the USD project 2023-08-25 23:53:40 +02:00
Contributor

It seems when importing a USD curve (for example a NURBS curve), no attributes are read. It's a shame since most hair is exported as curves

It seems when importing a USD curve (for example a NURBS curve), no attributes are read. It's a shame since most hair is exported as curves
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser Project (Legacy)
Interest
Asset System
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
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#109518
No description provided.