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

Fail gracefully when a mesh fails to import. #48912

Merged
merged 1 commit into from
May 21, 2021

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented May 21, 2021

When a mesh instance is processed that triggers an error in _process_mesh_instances, it causes the function to abort early and leaves many meshes with unassigned skeletons or skins.

This patch uses ERR_CONTINUE_MSG instead of ERR_FAIL_COND within the for loop.

Here are some valid glTF meshes which trigger errors at present in Godot (PRs pending):
DirectParentedSkeletons.glb

NestedSkeletonReproCaseV2Animated.glb

While I recognize that this patch is handling a failure case that ideally would never hapen, the point is it allows for maintaining consistent behavior even when there are bugs or invalid gltf nodes. Leaving ERR_FAIL here will cause a sort of undefined behavior (for example, skinned meshes arbitrarily not parented to their skeletons).

Worked with @fire on debugging this

@lyuma lyuma requested a review from a team as a code owner May 21, 2021 07:18
@akien-mga akien-mga added cherrypick:3.3 cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:import labels May 21, 2021
@akien-mga akien-mga added this to the 4.0 milestone May 21, 2021
@akien-mga akien-mga merged commit 6c2420b into godotengine:master May 21, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label May 21, 2021
@akien-mga
Copy link
Member

Cherry-picked for 3.3.2.

@lyuma lyuma deleted the gltf_graceful_fail_mesh branch May 19, 2023 10:46
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.

2 participants