USD: Automatically Set displayName metadata on Objects, Meshes, and Materials at Export if they contain non-ASCII characters #115808

Charles Wardlaw · 2023-12-05T16:13:43+01:00

Charles Wardlaw commented

2023-12-05 16:13:43 +01:00

For users writing object names in non-ASCII characters, this patch aims to more automatically handle the creation of displayName metadata at export time. If an Object, Mesh Data, or Material name contains non-ASCII characters, instead of converting them all to underscores (which can lead to prim names made entirely of underscores, and cause name collisions), a new unique name is generated and the original name is assigned to the prim's displayName.

Note: if a string Custom Data property called "displayName" is assigned to the object, this takes priority! This patch is meant to extend the prior patch on displayNames, not override it. The automatic names are only assigned if the name contains non-ASCII characters and there is not already a displayName set by the user on the Blender side.

I've attached two files for testing. It's important to note that in the file with Suzanne and multiple meshes, the cube object has a fully Japanese name on both the Object and its Data, but both also have displayName Custom Data overriding that value, so on export the Custom Data names take priority. In the second file with two cubes, I was testing to make sure that an Object could have its automatic display name set without setting the data (see the second cube). I've also attached the USDA output from these scenes for comparison.

For users writing object names in non-ASCII characters, this patch aims to more automatically handle the creation of displayName metadata at export time. If an Object, Mesh Data, or Material name contains non-ASCII characters, instead of converting them all to underscores (which can lead to prim names made entirely of underscores, and cause name collisions), a new unique name is generated and the original name is assigned to the prim's displayName. Note: if a string Custom Data property called "displayName" is assigned to the object, this takes priority! This patch is meant to extend the prior patch on displayNames, not override it. The automatic names are only assigned if the name contains non-ASCII characters _and_ there is not already a displayName set by the user on the Blender side. I've attached two files for testing. It's important to note that in the file with Suzanne and multiple meshes, the cube object has a fully Japanese name on both the Object and its Data, but both also have displayName Custom Data overriding that value, so on export the Custom Data names take priority. In the second file with two cubes, I was testing to make sure that an Object could have its automatic display name set without setting the data (see the second cube). I've also attached the USDA output from these scenes for comparison.

変なUSDのファイル.usda

153 KiB

変なUSDのファイル.blend

990 KiB

変なUSDのファイル第二章.usd

3.6 KiB

変なUSDのファイル第二章.blend

884 KiB

Charles Wardlaw added 2 commits 2023-12-05 16:13:52 +01:00

4fc1cb3c5f Mostly working; still have a few kinks to iron out.

4eb9d493eb Mesh data now works correctly.

Iliya Katushenock added this to the USD project 2023-12-05 16:18:09 +01:00

Iliya Katushenock changed title from ~~[USD] Automatically Set displayName metadata on Objects, Meshes, and Materials at Export if they contain non-ASCII characters~~ to USD: Automatically Set displayName metadata on Objects, Meshes, and Materials at Export if they contain non-ASCII characters

2023-12-05 16:18:18 +01:00

Michael Kowalski commented

2023-12-07 21:39:41 +01:00

I like the approach and exporting the examples works well! I will have some comments and suggestions about the code, to follow.

Michael Kowalski reviewed 2023-12-07 23:57:04 +01:00

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

						
				@ -280,0 +319,4 @@

				      default:

				        return "";

				    }

				  }(object->type);

Michael Kowalski commented

2023-12-07 23:57:04 +01:00

I believe you can look up the data type name with the function BKE_idtype_idcode_to_name(), so you could replace invoking the function with the switch statement with the direct call

#include "BKE_idtype.h"

const ID *id = static_cast<ID *>(object->data);
const char *type_as_string = BKE_idtype_idcode_to_name(GS(id->name));

Just a thought.

I believe you can look up the data type name with the function `BKE_idtype_idcode_to_name()`, so you could replace invoking the function with the switch statement with the direct call ``` #include "BKE_idtype.h" const ID *id = static_cast<ID *>(object->data); const char *type_as_string = BKE_idtype_idcode_to_name(GS(id->name)); ``` Just a thought.

Michael Kowalski commented

2023-12-08 18:57:42 +01:00

To add to that, if we were to use this approach, we would need to guard against null data, maybe assuming that this is an Empty, in that case:

const ID *id = static_cast<ID *>(object->data);
const char *type_as_string = id ? BKE_idtype_idcode_to_name(GS(id->name)) : "Empty";

To add to that, if we were to use this approach, we would need to guard against null data, maybe assuming that this is an Empty, in that case: ``` const ID *id = static_cast<ID *>(object->data); const char *type_as_string = id ? BKE_idtype_idcode_to_name(GS(id->name)) : "Empty"; ```

Charles Wardlaw commented

2023-12-21 21:44:54 +01:00

Thanks for that function! I used it instead in this version of the patch-- please let me know if this is what you were thinking.

CharlesWardlaw marked this conversation as resolved

Michael Kowalski reviewed 2023-12-08 00:10:27 +01:00

source/blender/io/common/IO_abstract_hierarchy_iterator.h Outdated

						
				@ -247,3 +256,3 @@

				   * object->data. Overriding is necessary when the exported format does NOT expect the object's

				   * data to be a child of the object. */

				  virtual std::string get_object_data_path(const HierarchyContext *context) const;

				  virtual std::string get_object_data_path(const HierarchyContext *context);

Michael Kowalski commented

2023-12-08 00:10:27 +01:00

We can discuss, but it might be preferable to keep the const qualifier on the functions here and below and to instead declare the members that are being changed mutable.

We can discuss, but it might be preferable to keep the `const` qualifier on the functions here and below and to instead declare the members that are being changed `mutable`.

Charles Wardlaw commented

2023-12-21 21:45:43 +01:00

I've reworked the whole patch to leave these functions const, with everything being done from within the iterator.

CharlesWardlaw marked this conversation as resolved

Michael Kowalski requested changes 2023-12-09 00:29:16 +01:00

Michael Kowalski left a comment

A few more comments, which we can discuss on Monday.

I will have a couple more remarks and questions before the review is done.

A few more comments, which we can discuss on Monday. I will have a couple more remarks and questions before the review is done.

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

						
				@ -277,4 +279,119 @@ void USDHierarchyIterator::add_usd_skel_export_mapping(const Object *obj, const

				  }

				}

				std::string USDHierarchyIterator::find_unique_name(const char *token)

Michael Kowalski commented

2023-12-09 00:27:46 +01:00

I think it's possible that this won't account for name clashes with existing objects in the scene that don't have to be renamed. E.g., if he token is "Mesh" and there is already a "Mesh" object in the scene but not in computed_names_, then this function will return "Mesh" as the unique name and you might get a name clash in the USD prim path.

Let's think about possible ways around this.

I think it's possible that this won't account for name clashes with existing objects in the scene that don't have to be renamed. E.g., if he token is "Mesh" and there is already a "Mesh" object in the scene but not in `computed_names_`, then this function will return "Mesh" as the unique name and you might get a name clash in the USD prim path. Let's think about possible ways around this.

Charles Wardlaw commented

2023-12-21 21:47:51 +01:00

I've accounted for all kinds of collisions this time by also checking for different names between UTF8 and TfMakeValidIdentifier. Please let me know if this is a good solve.

CharlesWardlaw marked this conversation as resolved

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

						
				@ -280,0 +293,4 @@

				}

				std::string USDHierarchyIterator::find_unique_object_name(const Object *object,

				                                                          const bool is_data = false)

Michael Kowalski commented

2023-12-08 16:45:36 +01:00

is_data is not used.

`is_data` is not used.

CharlesWardlaw marked this conversation as resolved

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

						
				@ -280,0 +317,4 @@

				      case OB_GREASE_PENCIL:

				        return "GreasePencil";

				      default:

				        return "";

Michael Kowalski commented

2023-12-08 19:04:53 +01:00

If we keep the switch statement, we need to handle the type OB_MBALL as well.

CharlesWardlaw marked this conversation as resolved

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

						
				@ -280,0 +359,4 @@

				  return get_computed_name(object, true);

				}

				std::optional<std::string> USDHierarchyIterator::get_display_name(const void *object)

Michael Kowalski commented

2023-12-09 00:19:49 +01:00

I think you can make the argument a const Object *object, instead of a const void *. That way, you can just return

return {object->id.name + 2};

But please let me know if I'm missing something.

I think you can make the argument a `const Object *object`, instead of a `const void *`. That way, you can just return ``` return {object->id.name + 2}; ``` But please let me know if I'm missing something.

Charles Wardlaw commented

2023-12-21 21:49:15 +01:00

I kind of reworked how all of that worked; please let me know if how I've changed things addresses your comment here.

CharlesWardlaw marked this conversation as resolved

Michael Kowalski commented

2023-12-11 23:19:03 +01:00

You are solving a tricky problem and I had to give it some thought.

I like the core idea of your solution a lot. Good stuff!

However, I think we might need to adjust the implementation somewhat to address the following concerns:

As much as possible, the abstract base classes HierarchyContext and AbstractHierarchyIterator should remain unchanged, since this code is shared with the ABC exporter so it should remain generic.
We need to handle possible name collision with other objects in the scene.
Also, materials should be renamed globally, not per object. I'm not sure, but I think the current code might result in duplicate materials if materials to be renamed are shared between multiple objects. But I could be wrong.

I would propose the following:

We don't change the abstract base classed HierarchyContext and AbstractHierarchyIterator at all and make most of the changes in USDAbstractWriter.
Add USDHierarchyIterator member Map<const ID*, const std::string> names_map_ to associate a name with an ID (for both objects and data).
Add the overridden function std::string USDHierarchyIterator::get_id_name(const ID *id) const to query names_map_ to return the object/data name for the given id.
The renaming should be done in a sperate iteration over the objects before writers are created, so that all object names are available to resolve name collisions. This can be done by overriding AbstractHierarchyIterator::iterate_and_write() in USDAbstractWriter, similar to the following:

void USDHierarchyIterator::iterate_and_write()
{
  DEGObjectIterSettings deg_iter_settings{};
  deg_iter_settings.depsgraph = depsgraph_;
  deg_iter_settings.flags = DEG_ITER_OBJECT_FLAG_LINKED_DIRECTLY |
                            DEG_ITER_OBJECT_FLAG_LINKED_VIA_SET;
  DEG_OBJECT_ITER_BEGIN (&deg_iter_settings, object) {
    // Populate names_map_ with object/data names and create a list
    // of objects that need renaming.  Also build a global map of material
    // names here.
  }
  DEG_OBJECT_ITER_END;

  // Iterate over objects and materials that need renaming here, based on the lists created
  // above, updating names_map_ with the new names.

  AbstractHierarchyIterator::iterate_and_write();
}

Would love your thoughts on this. I'll follow up with you to discuss directly.

You are solving a tricky problem and I had to give it some thought. I like the core idea of your solution a lot. Good stuff! However, I think we might need to adjust the implementation somewhat to address the following concerns: * As much as possible, the abstract base classes `HierarchyContext` and `AbstractHierarchyIterator` should remain unchanged, since this code is shared with the ABC exporter so it should remain generic. * We need to handle possible name collision with other objects in the scene. * Also, materials should be renamed globally, not per object. I'm not sure, but I think the current code might result in duplicate materials if materials to be renamed are shared between multiple objects. But I could be wrong. I would propose the following: * We don't change the abstract base classed `HierarchyContext` and `AbstractHierarchyIterator` at all and make most of the changes in `USDAbstractWriter`. * Add `USDHierarchyIterator` member `Map<const ID*, const std::string> names_map_` to associate a name with an ID (for both objects and data). * Add the overridden function `std::string USDHierarchyIterator::get_id_name(const ID *id) const` to query `names_map_` to return the object/data name for the given id. * The renaming should be done in a sperate iteration over the objects before writers are created, so that all object names are available to resolve name collisions. This can be done by overriding `AbstractHierarchyIterator::iterate_and_write()` in `USDAbstractWriter`, similar to the following: ``` void USDHierarchyIterator::iterate_and_write() { DEGObjectIterSettings deg_iter_settings{}; deg_iter_settings.depsgraph = depsgraph_; deg_iter_settings.flags = DEG_ITER_OBJECT_FLAG_LINKED_DIRECTLY | DEG_ITER_OBJECT_FLAG_LINKED_VIA_SET; DEG_OBJECT_ITER_BEGIN (&deg_iter_settings, object) { // Populate names_map_ with object/data names and create a list // of objects that need renaming. Also build a global map of material // names here. } DEG_OBJECT_ITER_END; // Iterate over objects and materials that need renaming here, based on the lists created // above, updating names_map_ with the new names. AbstractHierarchyIterator::iterate_and_write(); } ``` Would love your thoughts on this. I'll follow up with you to discuss directly.

Charles Wardlaw added 4 commits 2023-12-21 21:31:42 +01:00

b1b3ba1184 Rewrite 1/x: Moving as much as possible into the USD iterator and scanning all objects, data, and materials at iterator creation time.

c2c9c6128a Rewrite 2/x: Finished moving over logic while keeping the base class functions const.

530fce2962 Merge branch 'feature/auto_display_name2' into merge

8601fd0772 Rewrite 3/x: Merge fixes. Added support for Light and Camera data to have displayNames.

Charles Wardlaw added 1 commit 2023-12-21 21:43:51 +01:00

f479e3de82 Merge remote-tracking branch 'origin/universal-scene-description' into feature/auto_display_name

Charles Wardlaw commented

2023-12-21 21:50:58 +01:00

Hi @makowalski -- Thank you very much for your suggestions! I've integrated everything and moved as much as I could into the USD Iterator to keep the original base class const. I hope this layout is better.

I've also added support for camera and light data.

Hi @makowalski -- Thank you very much for your suggestions! I've integrated everything and moved as much as I could into the USD Iterator to keep the original base class const. I hope this layout is better. I've also added support for camera and light data.

Charles Wardlaw added 1 commit 2023-12-21 22:30:37 +01:00

c85e6d14a2 Running make format

Michael Kowalski commented

2024-03-13 15:33:11 +01:00

I previously merged this pull request manually, so am closing this. Thanks for the great work!

Michael Kowalski closed this pull request

2024-03-13 15:33:14 +01:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

USD: Automatically Set displayName metadata on Objects, Meshes, and Materials at Export if they contain non-ASCII characters #115808

Pull request closed