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 GLTF exporting invalid meshes and attempting to export gizmo meshes #87934

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

aaronfranke
Copy link
Member

This PR has 2 related but technically distinct bug fixes:

  • Fix GLTF exporting invalid meshes. In GLTF it is not allowed to have an empty mesh. The export code will now check for this and fail, instead of exporting something that did not comply to the glTF specification.

  • Fix GLTF exporting trying to export non-owned nodes in the editor. These are created by tool scripts, such as custom gizmo code. These nodes are temporary, usually for visualization purposes, they are not saved to the Godot scene and should not be saved in the GLTF file. In the event that somebody does want a tool script to save to the scene persistently and be exported to GLTF, you just need to set the owner to the scene root.

    • Note: This only affects the editor. User code at runtime is likely not using the owner system and therefore for runtime exports we should include all nodes like we have already been doing.

    • Note: This adds a new macro, WARN_VERBOSE, to only show a warning when in verbose mode. Lyuma was concerned about spamming warnings for valid workflows, which makes sense.

@aaronfranke aaronfranke added bug topic:import topic:3d cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Feb 4, 2024
@aaronfranke aaronfranke added this to the 4.3 milestone Feb 4, 2024
@aaronfranke aaronfranke requested review from a team as code owners February 4, 2024 09:03
/**
* Warns about `m_msg` only when verbose mode is enabled.
*/
#define WARN_VERBOSE(m_msg) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this, but typically need an opinion from the core team about these additions to macros.

@fire
Copy link
Member

fire commented Feb 6, 2024

@fire
Copy link
Member

fire commented Feb 6, 2024

I lose the face blend shapes.

https://github.com/V-Sekai/v-sekai-other-world/blob/main/tools/TOOL_godot_ik/ik_test.tscn

editor_screenshot_2024-02-06T073927

image

I posted the project. Will try to see where it breaks and if it's related to this pr.

Edit: It does seem to pass blender import without error so the pr does its stated goals.

@fire
Copy link
Member

fire commented Feb 6, 2024

BUG: the animation that is in the scene isn't associated with the export.

@lyuma
Copy link
Contributor

lyuma commented Feb 7, 2024

@fire The loss of blend shapes is happening during import due to a faulty property setter in vrm_secondary.gd (I see you have addons/vrm in your project).

The property setter causes a gizmo to be added during the import process, as an internal child (INTERNAL_MODE_FRONT) which triggers a Godot bug in Node::replace_by since it keeps the internal index but does not forward data.internal_mode to add_child.
Node::replace_by is buggy and is what ultimately causes the corruption during import in this case, but I fixed godot-vrm to not attempt to add gizmos in a property setter when not is_inside_tree(). that should mitigate the import issue at least.

Anyway, the bug you hit with the face which I describe above is during import and unrelated to this export PR. It just happens to have a similar cause. So this shouldn't block merging.

@akien-mga akien-mga merged commit 763d5cb into godotengine:master Feb 8, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the fix-gltf-bad-mesh-export branch February 8, 2024 16:32
@akien-mga
Copy link
Member

akien-mga commented Mar 11, 2024

Cherry-picked for 4.2.2.
Cherry-picked for 4.1.4.

@akien-mga akien-mga removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants