USD: Add common Blender attribute to USD primvar utilities #121145

Jesse Yurkovich · 2024-04-26T20:48:13+02:00

Jesse Yurkovich commented

2024-04-26 20:48:13 +02:00

This factors out the current set of attribute-to-primvar functions
inside the USD mesh reader/writer so we can use them elsewhere. Namely
they will also be used for exporting Curves attributes like UVs etc.

Outside of this PR I've verified these common functions should be
directly useful for upcoming PointCloud support and for our Curves.

This factors out the current set of attribute-to-primvar functions inside the USD mesh reader/writer so we can use them elsewhere. Namely they will also be used for exporting Curves attributes like UVs etc. Outside of this PR I've verified these common functions should be directly useful for upcoming PointCloud support and for our Curves.

Jesse Yurkovich added 2 commits 2024-04-26 20:48:22 +02:00

2b1fa93ef9 Add common Blender to USD attribute utilities

77c3aaa637 Make use of common attribute utilities for exporting Curve attributes

Jesse Yurkovich requested review from Michael Kowalski 2024-04-26 20:53:32 +02:00

Jesse Yurkovich requested review from Hans Goudey 2024-04-26 21:07:32 +02:00

Jesse Yurkovich reviewed 2024-04-26 21:12:25 +02:00

source/blender/io/usd/intern/usd_attribute_utils.hh Outdated

						
				@ -0,0 +177,4 @@

				          attribute.typed<math::Quaternion>(), timecode, primvar, value_writer);

				      break;

				    default:

				      BLI_assert_unreachable();

Jesse Yurkovich commented

2024-04-26 21:12:25 +02:00

Note: Generally this function works in concert with convert_blender_type_to_usd so we shouldn't see this even though e.g. a FLOAT4X4 type exists today.

Note: Generally this function works in concert with `convert_blender_type_to_usd` so we shouldn't see this even though e.g. a FLOAT4X4 type exists today.

Hans Goudey reviewed 2024-04-26 23:18:55 +02:00

Hans Goudey left a comment

The recent support for USD Shape colors factored out attribute "reading" into a common "mesh" attribute file. Should those move into this common "attribute" file instead

Yeah, it seems valuable to have similar utilities for reading attributes, to avoid duplication between meshes, point clouds, and curves.

Am I following the right patterns in transferring the attributes most efficiently? See copy_blender_buffer_to_primvar

That's a complex topic I think, but I think it's good enough. I left a comment there anyway.

We could move the implementation of the helper methods to a usd_attribute_utils.cc file. But it would require extra ceremony to duplicate the template signatures in the header. Should we do it?

The benefit would be reducing compile time? Not sure it's worth it if the templates are only used twice. But no strong opinion, and more information may help make that decision.

There's unfortunate convert_value overloads for bool, int, and float that I haven't figured out how to template away. Minor but annoying.

I think they are avoidable, left a comment inline.

>The recent support for USD Shape colors factored out attribute "reading" into a common "mesh" attribute file. Should those move into this common "attribute" file instead Yeah, it seems valuable to have similar utilities for reading attributes, to avoid duplication between meshes, point clouds, and curves. >Am I following the right patterns in transferring the attributes most efficiently? See copy_blender_buffer_to_primvar That's a complex topic I think, but I think it's good enough. I left a comment there anyway. >We could move the implementation of the helper methods to a usd_attribute_utils.cc file. But it would require extra ceremony to duplicate the template signatures in the header. Should we do it? The benefit would be reducing compile time? Not sure it's worth it if the templates are only used twice. But no strong opinion, and more information may help make that decision. >There's unfortunate convert_value overloads for bool, int, and float that I haven't figured out how to template away. Minor but annoying. I think they are avoidable, left a comment inline.

source/blender/io/usd/intern/usd_attribute_utils.hh Outdated

						
				@ -0,0 +42,4 @@

				/* Conversion utilities to convert a Blender type to an USD type. */

				template<typename BlenderT, typename USDT> inline USDT convert_value(const BlenderT value);

				template<> inline bool convert_value(const bool value)

Hans Goudey commented

2024-04-26 23:09:45 +02:00

I think you could avoid these redundant convert_value functions by using if constexpr in copy_blender_buffer_to_primvar

I think you could avoid these redundant `convert_value` functions by using `if constexpr` in `copy_blender_buffer_to_primvar`

deadpin marked this conversation as resolved

source/blender/io/usd/intern/usd_attribute_utils.hh

						
				@ -0,0 +115,4 @@

				  if (const std::optional<BlenderT> value = buffer.get_if_single()) {

				    usd_data.assign(buffer.size(), detail::convert_value<BlenderT, USDT>(*value));

				  }

				  else if (buffer.is_span() && (is_same || is_compatible)) {

Hans Goudey commented

2024-04-26 23:12:06 +02:00

const VArraySpan<T> src(buffer); would make this simpler (no need for the "not span" check), and probably won't be a bottleneck unless we get very fancy with out attribute storage, at which point we'll want to change this anyway.

`const VArraySpan<T> src(buffer);` would make this simpler (no need for the "not span" check), and probably won't be a bottleneck unless we get very fancy with out attribute storage, at which point we'll want to change this anyway.

Jesse Yurkovich commented

2024-04-29 03:42:30 +02:00

I'm a bit unsure what you mean for this one. How would you structure these conditionals instead?

Hans Goudey commented

2024-05-02 20:42:01 +02:00

Like this I think:

  if (const std::optional<BlenderT> value = buffer.get_if_single()) {
    usd_data.assign(buffer.size(), detail::convert_value<BlenderT, USDT>(*value));
  }
  else {
    const VArraySpan<BlenderT> data(buffer);
    if constexpr (is_same || is_compatible) {
      usd_data.assign(data.cast<USDT>().begin(), data.cast<USDT>().end());
    }
    else {
      for (const int i : buffer.index_range()) {
        usd_data[i] = detail::convert_value<BlenderT, USDT>(data[i]);
      }
    }
  }

I guess the last for loop could be written with std::transform too. Not sure about that though.

Like this I think: ```cpp if (const std::optional<BlenderT> value = buffer.get_if_single()) { usd_data.assign(buffer.size(), detail::convert_value<BlenderT, USDT>(*value)); } else { const VArraySpan<BlenderT> data(buffer); if constexpr (is_same || is_compatible) { usd_data.assign(data.cast<USDT>().begin(), data.cast<USDT>().end()); } else { for (const int i : buffer.index_range()) { usd_data[i] = detail::convert_value<BlenderT, USDT>(data[i]); } } } ``` I guess the last for loop could be written with `std::transform` too. Not sure about that though.

deadpin marked this conversation as resolved

source/blender/io/usd/intern/usd_attribute_utils.hh

						
				@ -0,0 +118,4 @@

				  else if (buffer.is_span() && (is_same || is_compatible)) {

				    Span<USDT> data = buffer.get_internal_span().cast<USDT>();

				    usd_data.assign(data.begin(), data.end());

Hans Goudey commented

2024-04-26 23:10:19 +02:00

This seems fine, with so much copying going on we're probably more memory bound anyway, so multithreading might not help so much.

deadpin marked this conversation as resolved

source/blender/io/usd/intern/usd_writer_curves.cc Outdated

						
				@ -30,3 +36,3 @@

				namespace blender::io::usd {

				#pragma optimize("", off)

Hans Goudey commented

2024-04-26 23:17:43 +02:00

Temporary?

Jesse Yurkovich commented

2024-04-29 03:41:00 +02:00

Yes, was completely temporary for local debugging and removed now.

deadpin marked this conversation as resolved

source/blender/io/usd/intern/usd_writer_curves.cc Outdated

						
				@ -357,0 +449,4 @@

				          }

				        }

				        else {

				          write_generic_data(curves, attribute_id, meta_data, usd_curves);

Hans Goudey commented

2024-04-26 23:13:59 +02:00

Use this-> to access class methods

Use `this->` to access class methods

deadpin marked this conversation as resolved

Jesse Yurkovich added 2 commits 2024-04-29 03:40:15 +02:00

a352618807 Merge remote-tracking branch 'upstream/main' into usd-commonattributes

b4de627d3c Feedback and tests

Jesse Yurkovich commented

2024-04-29 03:56:38 +02:00

Thanks for the look!

We could move the implementation of the helper methods to a usd_attribute_utils.cc file. But it would require extra ceremony to duplicate the template signatures in the header. Should we do it?

The benefit would be reducing compile time? Not sure it's worth it if the templates are only used twice. But no strong opinion, and more information may help make that decision.

Yes, there would probably be 4(?) uses of the file. Mesh, Curves, Point Clouds, and the Shape reader (if the common reader stuff is merged in here too)

There's unfortunate convert_value overloads for bool, int, and float that I haven't figured out how to template away. Minor but annoying.

I think they are avoidable, left a comment inline.

I think I found another, simpler solution while trying this out.

Thanks for the look! > >We could move the implementation of the helper methods to a usd_attribute_utils.cc file. But it would require extra ceremony to duplicate the template signatures in the header. Should we do it? > > The benefit would be reducing compile time? Not sure it's worth it if the templates are only used twice. But no strong opinion, and more information may help make that decision. Yes, there would probably be 4(?) uses of the file. Mesh, Curves, Point Clouds, and the Shape reader (if the common reader stuff is merged in here too) > >There's unfortunate convert_value overloads for bool, int, and float that I haven't figured out how to template away. Minor but annoying. > > I think they are avoidable, left a comment inline. I think I found another, simpler solution while trying this out.

Jesse Yurkovich referenced this pull request

2024-04-30 08:10:59 +02:00

USD: UsdGeomPoints import support #120060

Michael Kowalski commented

2024-05-02 03:08:17 +02:00

@deadpin I will review this PR in the next couple of days. My apologies for the delay.

Jesse Yurkovich added 3 commits 2024-05-02 20:08:27 +02:00

d60f8a1745 First pass at factoring out attribute reading common code

6c6fee0841 Merge remote-tracking branch 'upstream/main' into usd-commonattributes

d8a41ab5f3 Tidy up and add in the type optimization

Jesse Yurkovich commented

2024-05-02 20:12:06 +02:00

The most recent update to the PR now brings in the code for our Readers and commonizes it.
I have not tried this for Curves/PointClouds yet to see how well the API works in another example, but it works for the Meshes and has test coverage as of yesterday for it.

Along the way it lost logging to Reports, which I feel was always a bit weird since it can be so verbose. It would also be odd now that this is low-level common code shared among the object types. I do think that some code paths could warrent a CLOG output though, which would be fine, but that means we should probably move the templates into a .cc file somehow so we can use CLOG there...

The API set looks like:

Reading	Writing
`copy_primvar_to_blender_attribute` `copy_primvar_to_blender_buffer` `convert_usd_type_to_blender`	`copy_blender_attribute_to_primvar` `copy_blender_buffer_to_primvar` `convert_blender_type_to_usd`

The attribute functions contain the big switch statement which sets up the appropriate typed calls to the "buffer" functions.

The buffer functions do the actual copying, as efficiently as possible. They'll effectively memcpy for types that are layout compatible between Blender and USD i.e. float vs. float, or float3 vs. GfVec3 etc.

The type functions switch between Blender and USD type names as appropriate.

These names are a little verbose and I'm not married to them. Anything with symmetry would work. Perhaps we could append attr to the outer namspace and use something like:

Reading - attr::copy_from_primvar / attr::copy_from_usd_to_blender
Writing - attr::copy_to_primvar / attr::copy_to_usd_from_blender

The most recent update to the PR now brings in the code for our Readers and commonizes it. I have not tried this for Curves/PointClouds yet to see how well the API works in another example, but it works for the Meshes and has test coverage as of yesterday for it. Along the way it lost logging to Reports, which I feel was always a bit weird since it can be so verbose. It would also be odd now that this is low-level common code shared among the object types. I do think that some code paths could warrent a CLOG output though, which would be fine, but that means we should probably move the templates into a .cc file somehow so we can use CLOG there... The API set looks like: | Reading | Writing | | ---- | ---- | | `copy_primvar_to_blender_attribute` `copy_primvar_to_blender_buffer` `convert_usd_type_to_blender` | `copy_blender_attribute_to_primvar` `copy_blender_buffer_to_primvar` `convert_blender_type_to_usd` | The `attribute` functions contain the big switch statement which sets up the appropriate typed calls to the "buffer" functions. The `buffer` functions do the actual copying, as efficiently as possible. They'll effectively memcpy for types that are layout compatible between Blender and USD i.e. float vs. float, or float3 vs. GfVec3 etc. The `type` functions switch between Blender and USD type names as appropriate. These names are a little verbose and I'm not married to them. Anything with symmetry would work. Perhaps we could append `attr` to the outer namspace and use something like: ``` Reading - attr::copy_from_primvar / attr::copy_from_usd_to_blender Writing - attr::copy_to_primvar / attr::copy_to_usd_from_blender ```

Hans Goudey reviewed 2024-05-02 20:49:01 +02:00

source/blender/io/usd/intern/usd_attribute_utils.hh Outdated

						
				@ -0,0 +246,4 @@

				template<typename USDT, typename BlenderT>

				void copy_primvar_to_blender_buffer(const pxr::UsdGeomPrimvar &primvar,

				                                    const pxr::UsdTimeCode timecode,

				                                    const blender::OffsetIndices<int> face_indices,

Hans Goudey commented

2024-05-02 20:43:23 +02:00

blender:: unnecessary here I think? Also I'd go with just faces instead of face_indices which makes me think of an array of actual indices into the mesh faces arrays

`blender::` unnecessary here I think? Also I'd go with just `faces` instead of `face_indices` which makes me think of an array of actual indices into the mesh faces arrays

deadpin marked this conversation as resolved

source/blender/io/usd/intern/usd_attribute_utils.hh Outdated

						
				@ -0,0 +291,4 @@

				    if (usd_data.size() == attribute.size()) {

				      if constexpr (is_same || is_compatible) {

				        const Span<USDT> src(usd_data.data(), usd_data.size());

				        attribute.copy_from(src.cast<BlenderT>());

Hans Goudey commented

2024-05-02 20:48:41 +02:00

For creating Blender attributes in this is_same || is_compatible case, there's one potential optimization that might be worth thinking about at this point, creating attributes with implicitly shared data.

With implicit sharing, the custom data layer data pointer could just point to the VtArray if the VtArray could be owned by something like MEMFreeImplicitSharing.

To allow that optimization in the future it might be necessary to potentially delay creating the attribute until we know whether the buffer can be reused. Or not, maybe that gives you an idea.

For creating Blender attributes in this `is_same || is_compatible` case, there's one potential optimization that might be worth thinking about at this point, creating attributes with implicitly shared data. With implicit sharing, the custom data layer `data` pointer could just point to the `VtArray` if the `VtArray` could be owned by something like `MEMFreeImplicitSharing`. To allow that optimization in the future it might be necessary to potentially delay creating the attribute until we know whether the buffer can be reused. Or not, maybe that gives you an idea.

Michael Kowalski approved these changes 2024-05-04 04:19:46 +02:00

Michael Kowalski left a comment

The code looks good and works well in my tests.

I don't feel strongly about whether to move functions to a usd_attribute_utils.cc file. I would be inclined to leave the current implementation as-is and move the code to a .cc file later if needed, but I wouldn't object if you want to move it now, either.

The code looks good and works well in my tests. I don't feel strongly about whether to move functions to a `usd_attribute_utils.cc` file. I would be inclined to leave the current implementation as-is and move the code to a .cc file later if needed, but I wouldn't object if you want to move it now, either.

Jesse Yurkovich added 7 commits 2024-05-13 18:05:57 +02:00

80ad0751be Merge remote-tracking branch 'upstream/main' into usd-commonattributes

49f668a654 Merge remote-tracking branch 'upstream/main' into usd-commonattributes

951e57f815 Merge remote-tracking branch 'upstream/main' into usd-commonattributes

0feae969a7 Remove curve changes to review separately

ba778eae6b Merge remote-tracking branch 'upstream/main' into usd-commonattributes

520471ac4e Feedback and cleanup

5e20ad3513 Merge remote-tracking branch 'upstream/main' into usd-commonattributes

Jesse Yurkovich changed title from ~~WIP: USD: Add common Blender attribute to USD primvar utilities~~ to USD: Add common Blender attribute to USD primvar utilities

2024-05-13 18:08:57 +02:00

Jesse Yurkovich commented

2024-05-13 18:13:46 +02:00

Alright, this PR should be ready to go now in case folks here want to take a final look. Main changes since last time are the removal of the Curve* items which will come in separately since it's net-new functionality and addressing feedback/cleanup.

I'm still open to naming/namespace related changes if the raw function names are problematic in any way.

Alright, this PR should be ready to go now in case folks here want to take a final look. Main changes since last time are the removal of the Curve* items which will come in separately since it's net-new functionality and addressing feedback/cleanup. I'm still open to naming/namespace related changes if the raw function names are problematic in any way.

Michael Kowalski commented

2024-05-14 17:08:50 +02:00

The latest changes look good to me. I also preformed some basic testing and the code seems to work as expected. Thank you for the updates!

Hans Goudey reviewed 2024-05-14 17:32:28 +02:00

Hans Goudey left a comment

Looking at this again I noticed all of the important entry points aren't templated. And the inline functions are quite large. So I think it would make sense to put the implementations in .cc file. Other than that it looks great.

Looking at this again I noticed all of the important entry points aren't templated. And the inline functions are quite large. So I think it would make sense to put the implementations in `.cc` file. Other than that it looks great.

Jesse Yurkovich added 1 commit 2024-05-14 19:15:48 +02:00