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

Fix export mesh lib break due to imported gltf has a redundant Node3D #82792

Conversation

jsjtxietian
Copy link
Contributor

Fixes #81850

The code is a little messy this way, but imho it's better to change the mesh lib export code than change the gltf import code. I would appreciate any feedback or alternative suggestions.

@Chaosus Chaosus added this to the 4.2 milestone Oct 4, 2023
@akien-mga akien-mga requested a review from a team October 4, 2023 17:08
@fire
Copy link
Member

fire commented Oct 5, 2023

@aaronfranke does this interfere with any of your changes?

@aaronfranke
Copy link
Member

Long-term, I would like to adjust the mesh library import code so that it no longer needs a MeshInstance3D node. Instead, it should be able to just grab the mesh resources imported from the glTF file. That way it does not depend on the glTF file having any particular node structure, and can even work when the glTF file has no nodes.

Short-term, I don't have any objections to this ad-hoc fix.

@jsjtxietian
Copy link
Contributor Author

jsjtxietian commented Oct 5, 2023

Instead, it should be able to just grab the mesh resources imported from the glTF file.

Agreed, do we need a proposal for this ?Or add a todo in the comment.

@aaronfranke
Copy link
Member

aaronfranke commented Oct 5, 2023

@jsjtxietian I would do this as part of godotengine/godot-proposals#7494, allowing MeshLibrary as one of the resource import options. However this rework is significant so it must wait for Godot 4.3 at the earliest. I think this PR is fine to merge for Godot 4.2 as an ad-hoc fix for now.

@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Nov 10, 2023
@YuriSizov
Copy link
Contributor

Short-term, I don't have any objections to this ad-hoc fix.

I assume we'll need compatibility code for this?

@lyuma
Copy link
Contributor

lyuma commented Nov 11, 2023

Is there a case where this change can cause compatibility breakage? My understanding is this PR allows one extra case where an object wouldn't have been considered legal for a mesh library.

But I do agree with the assessment that ideally all meshes in the glTF document will be used regardless of node structure.

@akien-mga
Copy link
Member

Superseded by #87923.

@akien-mga akien-mga closed this Feb 6, 2024
@AThousandShips AThousandShips removed this from the 4.3 milestone Feb 6, 2024
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.

Mesh Libraries Breaking Due To A Redundant Node3D Being Generated Upon Asset Import.
8 participants