Node names are unset after creation when NodeCreated action is triggered

[LaneF]

Describe the bug GltfImport.InstantiateSceneInternal() separates the process of "CreateHierarchy" and "PopulateHierarchy". (GltfImport line ~2541) After CreateHierarchy is done, each run will fire the NodeCreated event for the object. Using this callback is guaranteed to be called at a time when the object that was created has an invalid (default) name. Even if this was considered acceptable, there's no alternative to get a callback for when the object is in a valid state.

PopulateHierarchy does not have any events to utilize for after each run, so in effect we cannot get a callback for Nodes that are in a valid state after they are created.

To Reproduce Steps to reproduce the behavior:

1. Subscribe to NodeCreated and log the object name out.
2. Observe that it is always "New Game Object".

Expected behavior I very much expect the name to be set in the CreateHierarchy function as the state of the object should be considered invalid if the name is not correct and no callback is provided to act over the object when it has reached a valid state.

Additional context No other context is required, as this is a straightforward issue. While I wouldnt consider it critical, it surely makes the implementation requirements more complex for us as we have to navigate through other pathways as a workaround.

[LaneF]

Further, things like m_nodeNames being private prevents us from accessing it in derived classes and limits extendability.

[atteneder]

@LaneF Thanks for bringing forward your requirements.

Agreed, this part of the API appears incomplete.

But first please tell me more about your use-case. In what ways are you using the GameObject names? What do you intend to do, once identifying/filtering by name works? I assume you assign names to nodes in your glTF generator/exporter so you can identify them later on? Depending on that I might be able to think of alternative, better solutions.

The way GameObjects are named by glTFast depends on a couple of factors.

• NameImportMethod Original:
  • Node's name
  • 1st fallback: Mesh's name
  • (There's no further fallback; just realized this might be considered inconsistent with the method below).
• NameImportMethod Original Unique (is enforced if glTF contains animation clips):
  • Node's name
  • 1st fallback: Mesh's name
  • 2nd fallback: Node-{node index}
  • If the resulting name already exists at the same level in the hierarchy, it's suffixed with _ and an incremented integer (e.g. SameName_0, SameName_1, ...)

m_nodeNames is not even set for the first method, so I'm hesitant to grant access to such internals.

I'll be investigating a solution, considering also your requirements once I know them in greater detail.

Thanks

[LaneF]

@atteneder Thanks for getting back!

I did notice the naming convention, generally we don't care what the name actually is - we just want to parse some data out of it one time during import. The uniqueness imposed does help us minimize duplicate guids when we hash them.

Use Case It's Arch/Viz, but massive structures (stadiums with nuts, bolts, pipes, everything) and includes everything's corresponding BIM data (also enormous). We hash the GameObject name to a Guid and have several dictionaries for identification mapping so the runtime interactions with individual objects are fast. No strings used.

We override GameObjectInstantiator.AddPrimitive which is great, and helps us inject important customization on the models. But, one of the things we need is the BIM ID from the source application (like Revit), which is stuffed into the name of the GameObject, so we peel it out during import because string operations are too slow to permit at runtime. Async we're actually parsing all of the BIM data at the same time, separately, and the object name (containing the bim id) is the only way to relate that separate data to the GameObjects later on in the process.

// public override void AddPrimitive....

bimId = mr.transform.name.TryGetBimId(out bool success);
// [...other id's then are processed from this...]
ObjectManager.Inject([various data/ids]);

but we don't have valid names at that point by default, so we have to do this before running the instantiator in order to get the name.

instantiator.NodeCreated += (nodeIndex, gameObject) =>
{
    // TODO REFACTOR we have to force the name here because we require it for the Guid hash.
    // See GltfImport.cs line 2541 (Create, then Populate - which will SetName)
    gameObject.name = gltf.GetSourceNode((int)nodeIndex).name;
    ObjectManager.Inject(gameObject);
};
await gltf.InstantiateMainSceneAsync(instantiator);

It seems most efficient to simply run our custom requirements inside the normal import functions and remove the need for extra passes after waiting for the import phase to finish, which is what I could also do but navigating the transform hierarchy to re-process objects that I've already got references to earlier is wasteful and we're pushing the envelope with these big models so there is very low tolerance for introducing performance impacts.

I hope that gives some useful context. Can't share specifics due to NDA, but collaboration may be mutually beneficial. I'm really curious if deeper level integration would improve loading times, but I'm not quite sure what that looks like.

[atteneder]

Thanks for the insight.

collaboration may be mutually beneficial.

Most definitely!

First, let's talk about the general purpose problem of node names getting set rather late and potential solutions.

The cleanest solution (without much performance overhead) would be to add the name as parameter to CreateNode and remove SetNodeName. Unfortunately that would be an API breaking change, so not an option before the next major release.

The next best thing is to call SetNodeName immediately after the node was created. That means the name is still not set when NodeCreated fires. However, the name would be correct when AddPrimitive* is called, so that's a slight improvement. We could also fire NodeCreated within SetNodeName to ensure names are correct as a temporary crutch (until we clean up the API in the next version). Not exactly pretty though.

Now about your project-specific problem. Actually many people try to do something similar. IMHO, from a data perspective, the cleaner solution would be to not rely on node names, but instead add meta-data (like GUIDs or hashes) to nodes either via custom extensions or in the extras field. With glTFast you can read back that data today already, but unfortunately this means using the Newtonsoft JSON, which is not as performant (I assume that's a top priority for you). We'll be investigating further how this can be achieved without compromise in the future, so stay tuned.

[LaneF]

The cleanest solution (without much performance overhead) would be to add the name as parameter to CreateNode and remove SetNodeName. Unfortunately that would be an API breaking change, so not an option before the next major release.

Agree

The next best thing is to call SetNodeName immediately after the node was created. That means the name is still not set when NodeCreated fires. However, the name would be correct when AddPrimitive* is called, so that's a slight improvement.

Agree, sufficient stop-gap.

We could also fire NodeCreated within SetNodeName to ensure names are correct as a temporary crutch (until we clean up the API in the next version). Not exactly pretty though.

Would not recommend, out of scope for the method and would be confusing.

add meta-data (like GUIDs or hashes) to nodes either via custom extensions or in the extras field. With glTFast you can read back that data today already, but unfortunately this means using the Newtonsoft JSON, which is not as performant (I assume that's a top priority for you). We'll be investigating further how this can be achieved without compromise in the future, so stay tuned.

That's a good idea I might investigate further. We had noticed there was some functionality to do this but I don't recall discussing it in any depth. But, either way you are correct that moving to JSON may impact perf. We are currently using protobuf which (technically we support json files too, but...) has been the best in terms of compression value and parsing performance on the meta data files in my tests.

[atteneder]

All right, a fix that invokes SetNodeName right after a node was created is scheduled for the next release.

Thanks for your help