Perhaps change the heading Additional Options to something more descriptive, like Assets?

Here and elsewhere, I believe the Blender coding style for return arguments is to use the prefix r_ rather than out (i.e., r_file_path).

Returning here will cause a memory leak, because vdb_directory_name, vdb_directory_path and vdb_file_name won't be freed.

For this reason, would it be better to rework the logic above to avoid calling BLI_strdup* to dynamically allocate the strings? Perhaps you could allocate local char array variables on the stack instead. For example,

char vdb_directory_name[FILE_MAXDIR];
BLI_strncpy(vdb_directory_name, usd_file_name, FILE_MAXDIR);
BLI_path_extension_replace(vdb_directory_name, FILE_MAXDIR, "_vdb");

and using a similar approach for the other string variables as well. I didn't compile or test that snippet, but you get the idea.

write_grid_relationship() could be a static function, as it doesn't need to access any USDVolumeWriter members. However, since this function is so simple, I wonder if it would be clearer to just call

usd_volume.CreateFieldRelationship(pxr::TfToken(grid_id), grid_path);

directly in the code.

What's interesting is that usd_volume could be a const reference:

const pxr::UsdVolVolume &usd_volume

even though it seems unintuitive. Likewise, in the following three functions, usd_grid can be declared const pxr::UsdVolOpenVDBAsset &usd_grid.

To clarify my last comment, the issue is that MEM_freeN() won't get called on the dynamically allocated strings if BKE_volume_save() fails and there is an early return on line 133. Thus, another solution, of course, is to make sure to call MEM_freenN() on these strings before the early return. I was suggesting allocating the char arrays on the stack as this makes future code maintenance easier (e.g., if someone adds an early return in the future and forgets to free the strings). Also, stack allocation is less overhead than dynamic allocation, but this is a minor point. I don't feel strongly about which approach is used to address this.

This is a relatively minor point, but I don't think you need to construct a VtValue() to wrap the volume_extent argument. I.e., the above line could simply be:

usd_volume.GetExtentAttr().Set(volume_extent, timecode);

There are other places where this change could be made in the code below. I know that I haven't always been consistent in my own code on this point, but I think it's probably better to set the values directly with the required type where possible.

As noted above, no need to wrap grid_name with VtValue().

No need to wrap grid_asset_path with VtValue().

No need to construct VtValue().

"Assets" is a word that has many meanings, and the use here is different from what Blender considers to be "assets" (i.e. in the scope of the asset browser).

Now that this option is no longer specific to textures, I feel that the separation between this group (material options + relative paths) and the other groups (subset of objects; data types) becomes a bit clumsy. Furthermore, this option group is still disabled when the "Materials" data type is deselected (if this changed, it's not described in the patch description), making it more confusing.

This may not be the best place to do this check. This function (AbstractHierarchyWriter::check_is_animated()) is for abstract Object-level data only. What's the reason this is done here, rather than in USDVolumeWriter::check_is_animated()?

Why is it okay to ignore errors writing VDB files? Errors should be reported to the user. If this is not necessary, the reason should be documented.

This function is called "write VDB file", but 90% of its code is actually only constructing the path the VDB file is supposed to be written to. I think it's better to split it up into separate functions.

As an added bonus, that way the caller can see the difference between "the file path could not be constructed" and "the VDB could not be written".

What are the chances of the export file path being empty, at this point in the export process? I don't think it is okay to silently skip such a situation. Using BLI_assert_msg() is probably a better approach, if you want to do a check on usd_file_path at all.

Basically the same question as above: in which situation will the splitting of usd_file_path not be succesful? And is it really okay to silently ignore the fact that the volume cannot be exported in this case?

Having a negation in the condition and an else clause is a bit much. Swap the if and else blocks and drop the negation to simplify the code a bit.

This logic seems to figure out which path to write to when new_vdb_file_path is empty. Such logic seems out of place in a function that's basically named "write (to) this file path".

Same question as before: in which cases will this path be empty? Is that even legal at this point in the export stage?

This block seems to do something different from the rest of the function, so should be in its own function (single responsibility principle).

usd_grid is modified here, even though it's a const parameter. This is confusing, especially since the name of the function doesn't suggest that it's setting anything on the usd_grid; rather it suggests something is written (which in the context of an exporter means "to disk").

Same issue as above, this function modifies the usd_grid but declares it as const. Even though it might technically be allowed to do this, I don't think it's very clear, especially since the function has no documentation.

Since the volumes are actually saved as VDB files and not included in the exported USD file, this class behaves differently from other subclasses of USDAbstractWriter. Its behaviour should be described here.

It would help to have some documentation here that describes which file paths are actually used for writing the VDB files.

Having a non-const reference means that the parameter can be modified without this being clear at the call itself. Why not simply return std::pair<std::string, bool>?

Document the meaning of the return value.

This function may need some documentation, as it's unclear what is written here. The name suggests it writes a file path, but it might also write *to* a file path, in which case it's unclear what the difference is between write_vdb_file() and write_file_path().

I'm afraid I'm responsible for this parameter being declared const. Piotr originally had this parameter non-const and I asked him to change this to const (here and in other similar cases of function declaration elsewhere in the code). It seems odd to me (and arguably incorrect) to require a non-const reference here when it isn't needed. I would opt for an additional comment describing the behavior of the function.

• Word "asset" is used by USD API/documentation in such cases. I'm open to other suggestions though and will be happy to implement such.
• In terms of UI grouping of the export options, I agree, it looks clumsy indeed. It is important to remember that doing overhaul of the UI/UX to match the quality of other importers/exporters is outside of the scope of this patch. With that being said, if you guys have some simple solution for this particular problem that can be implemented - I'm all ears.
• Regarding disabling the checkbox - I added comment to the description of this patch. Disabling no longer apply to the relative paths option.

I disagree with placing an assertion here. I don't see a reason for Blender to close in this situation. A proper export of volume data to USD will fail in this scenario which is why I added warning message in updated diff.

Is there any way that this can return a different value than data->filename passed to pxr::UsdStage::CreateNew(data->filename) in usd_capi_export.cc, function export_startjob()?

I think the code itself is fine. It could just use a comment that either explains that this is the same value (and obtaining it this way prevents passing around yet another string), or an explanation how it could be different (and motivate why this approach is better).

This is a nice explanation of why the filepath can be empty, thanks.

Flip these conditions and return early. That'll reduce the cognitive complexity.

This mostly avoids duplication, otherwise, as you mentioned, explicitly storing data->filename value as a member variable in USDHierarchyIterator class would be required. I don't think there is a way for the routine above to return different path, but I'm not USD expert, so @Michael Kowalski (makowalski) can hopefully confirm this.

I used vdb_file_path.value_or("") instead of vdb_file_path.value() - simplified the code even more and end effect is pretty much the same.