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

Closed
Charles Wardlaw wants to merge 8 commits from CharlesWardlaw/blender:feature/auto_display_name into universal-scene-description

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

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.
Charles Wardlaw added 2 commits 2023-12-05 16:13:52 +01:00
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

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

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
@ -280,0 +319,4 @@
default:
return "";
}
}(object->type);

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.

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"; ```
Author
Member

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.

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
@ -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);

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`.
Author
Member

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

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
Member

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.
@ -277,4 +279,119 @@ void USDHierarchyIterator::add_usd_skel_export_mapping(const Object *obj, const
}
}
std::string USDHierarchyIterator::find_unique_name(const char *token)

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.
Author
Member

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.

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
@ -280,0 +293,4 @@
}
std::string USDHierarchyIterator::find_unique_object_name(const Object *object,
const bool is_data = false)

is_data is not used.

`is_data` is not used.
CharlesWardlaw marked this conversation as resolved
@ -280,0 +317,4 @@
case OB_GREASE_PENCIL:
return "GreasePencil";
default:
return "";

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

If we keep the switch statement, we need to handle the type OB_MBALL as well.
CharlesWardlaw marked this conversation as resolved
@ -280,0 +359,4 @@
return get_computed_name(object, true);
}
std::optional<std::string> USDHierarchyIterator::get_display_name(const void *object)

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.
Author
Member

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

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

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
Charles Wardlaw added 1 commit 2023-12-21 21:43:51 +01:00
Author
Member

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

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

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

Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
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
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
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
2 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#115808
No description provided.