GLTFExporter: Add comment for empty strings name

[takahirox]

I think we need to compare object.name with undefined to allow 0 and empty string as node name in GLTFExporter.

[mrdoob]

Hmm... When would object.name be 0? Shouldn't the check be object.name !== ''?

[takahirox]

Wait, I've misunderstood a bit by any chance... Don't merge yet.

[takahirox]

I think I've misunderstood #13209 so I posted a question there.
I'll update (or close) this PR depending on the conclusion there.

[takahirox]

Seems like we can forget about numeric .name.

And my original motivation of this PR is I suppose we should allow empty string .name to be exported. So I think we can replace

if ( object.name ) {

	gltfNode.name = object.name;

}

with

gltfNode.name = object.name;

because Object3D.name is required property.

I'll update this PR after #13209 is reverted.

Or do you think we shouldn't export empty string .name (because empty string .name generally indicates no one cares that name in Three.js)? @fernandojsg @donmccurdy

[donmccurdy]

gltfNode.name = object.name;

I wouldn't do this, the default in three.js is an empty string and the default in glTF is undefined, empty strings would just be noise to a non-threejs glTF loader. Let's revert #13209 and then I don't think this PR will be necessary.

[takahirox]

Then when users wanna explicitly export empty string .name, how can they do? We don't need to care because it'd be a very rare case? Add an new option for that?

[donmccurdy]

They can file an issue and make a case for it. ;) But 99.99% of cases are empty strings because it's three.js's default, and I don't think we should propagate that without a clear use case.

[mrdoob]

Shouldn't this work?

if ( object.name !== '' ) {

    gltfNode.name = object.name;

}

[webprofusion-chrisc]

Lol, now I understand why you want to revert #13209! Folks, from 23 years of experience I can tell you people are going to throw all sorts of nonsense at importers and exporters and they will hope that they work. When it doesn't work they most likely won't be in a position to fix it themselves. Go with the simplest most pragmatic approach you can find.

[mrdoob]

@webprofusion-chrisc We have a chance to be strict with the glTF format and make sure that people produce correct files.

[takahirox]

@donmccurdy

Yeah, I know I'm talking about the edge case so considering when an issue is opened with actual use case would sound reasonable.

I feel like adding comment so far, like

// @TODO Consider a rare case where users wanna explicitly export empty strings name
if ( object.name ) {

    gltfNode.name = object.name;

}

or

// Empty strings is the default name in Three.js. We don't export empty strings name so far.

What do you think?

@mrdoob

When we assume object.name is string, I think its behavior is same as the code's after reverted

if ( object.name ) {

    gltfNode.name = object.name;

}

What's your suggestion's purpose?

[donmccurdy]

I wouldn’t call it a TODO, I do not think we should let empty strings be exported as node names (mainly because it’s impossible to differentiate from default empty strings, and not worth having an option for a use case that no one has requested yet and probably won’t).

Beyond that I’m OK with any of these changes.

[takahirox]

I came to think we should first clarify what empty strings names mean in Three.js.

@mrdoob Which one should Three.js regard the object with empty strings name as

1. it has an empty strings name ''.
2. it hasn't a name.

I thought Three.js expects 1., name can be any strings including length 0 strings so I wanted GLTFExporter to export empty strings name.

If Three.js expects 2. I don't think GLTFExporter needs to export it. And I wanna add note "Empty strings name is regarded as no-name in Three.js" into doc for clarification.

[mrdoob]

Shouldn't be a matter of doing this?

In GLTFLoader...

if ( node.name !== undefined ) object.name = node.name;

In GLTFExporter...

if ( object.name !== '' ) node.name = object.name;

Do the glTF validators check that node.name is undefined or String?

[donmccurdy]

I'm happy with that suggestion, aligning with case (2) where an empty string is considered lack of a name.

GLTFLoader will fail if the name is not a string (name.replace is not a function) and the official validator will also raise an error:

Undefined is fine in both.

[takahirox]

In GLTFExporter...

if ( object.name !== '' ) node.name = object.name;

With case (2), this's good.

With case (1), this may not be good because empty strings name info can be lost. It isn't guaranteed that other viewers convert undefined name to empty strings name.

As you Ricardo suggested so, probably case (2) would be the Three.js .name spec?

My motivation is just to clarify Three.js spec and let exporter follow it.

[donmccurdy]

Definitely (2) is the right idea here. Let's go with #13359 (comment).

[takahirox]

@mrdoob Which one is the right spec in Three.js, (1) or (2)?

[takahirox]

(And if (2) is the right spec,) is the motivation of replacing if ( object.name ) with if ( object.name !== '' ) for the code consistency? I saw if ( object.name !== '' ) style in other modules, but don't see if ( object.name ) style.

[mrdoob]

Definitely (2) is the right idea here. Let's go with #13359 (comment).

👍

[takahirox]

OK, thank you for your patience, all!

We clarified the name spec, then I've updated PR.

• Revert the change
• Add comment why we don't export empty strings name
• Replace if ( object.name ) with if ( object.name !== '' ) for the code consistency

And, as I said the above, I'm gonna add note about empty strings name in Doc in another PR.

[mrdoob]

Thank you!