Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GLTF2: Resolve #29265 #29273

Closed
wants to merge 1 commit into from
Closed

GLTF2: Resolve #29265 #29273

wants to merge 1 commit into from

Conversation

fire
Copy link
Member

@fire fire commented May 29, 2019

References: #29265

@fire
Copy link
Member Author

fire commented May 29, 2019

Please try more models.

@fire fire force-pushed the vrm_29265 branch 2 times, most recently from 5e87c8e to d1b8908 Compare May 29, 2019 06:49
@Chaosus Chaosus added this to the 3.2 milestone May 29, 2019
@akien-mga akien-mga requested a review from reduz May 29, 2019 11:15
@fire fire force-pushed the vrm_29265 branch 2 times, most recently from ebb99a3 to be992e4 Compare May 30, 2019 00:25
@fire fire changed the title GLTF2: Misc fixes. [WIP] GLTF2: Misc fixes. May 31, 2019
@vpenades
Copy link

Maybe I'm mistaken, but I've noticed in the code that the node name property is used to find parent nodes, etc.

Most exporters, specially those included in authoring packages, will fill names from the names in the scene...

But glTF does not require nodes, meshes or materials to have a specific and unique name.

Which means that names can be null, or repeated (I could export a model with all the nodes having the same name)

It is better to use object pointers or indices for referencing.

@fire
Copy link
Member Author

fire commented Jun 18, 2019

Godot doesn't allow non unique names in the same level. I think I can keep the requirement that it has a unique name in the entire file.

I cannot use object pointers or indices to reference. Generating a standard uuid is too difficult to use.

[edited] This is important when you import the same file into the existing scene.

@vpenades
Copy link

@fire So, does it mean a gltf model with nameless nodes cannot be imported?

@fire
Copy link
Member Author

fire commented Jul 12, 2019

@vpenades Can you open a different issue about this problem? This one is related to resolving the problems with #29265

@fire
Copy link
Member Author

fire commented Jul 12, 2019

@fire fire changed the title [WIP] GLTF2: Misc fixes. GLTF2: Resolve #29265 Jul 12, 2019
@fire fire changed the title GLTF2: Resolve #29265 [WIP] GLTF2: Resolve #29265 Jul 12, 2019
@fire fire changed the title [WIP] GLTF2: Resolve #29265 GLTF2: Resolve #29265 Jul 12, 2019
@fire fire requested a review from akien-mga July 24, 2019 06:02
* Skeleton element is ignored KhronosGroup/glTF#1270
* Set depth draw mode for transparent materials.
* A -1 parent means it's root, so do not dereference that node.
* A file with no scene is acceptable.
* Canonicalize gltf node names.
@akien-mga
Copy link
Member

@reduz's comment for now:

15:44 <reduz> Akien: not sure about this one, I need to spend some time again with the GLTF2 importer myself to fix some issues, so for now dont merge

@fire
Copy link
Member Author

fire commented Sep 24, 2019

This should be resolved by the current changes in master.

@fire fire closed this Sep 24, 2019
@fire fire deleted the vrm_29265 branch October 7, 2019 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants