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

GLTF: Add a comment for skinned mesh tree placement #80807

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Aug 20, 2023

This PR adds a comment explaining why we generate an empty node here. The reason is not obvious from just reading the code, so we need to have a comment explaining why we do this.

@fire
Copy link
Member

fire commented Aug 20, 2023

Since this is sensitive code, I think adding the comment is enough.

Edited:

The code compression is not worth it, because the previous code is straightforward to parse.

In this section of the gltf code we repeated the same code three times and that was easiest way to verify the correctness.

You only modified one of the three.

@aaronfranke aaronfranke changed the title GLTF: Add a comment for skinned mesh tree placement and cleanup code GLTF: Add a comment for skinned mesh tree placement Aug 20, 2023
@AThousandShips AThousandShips added this to the 4.x milestone Aug 20, 2023
@fire
Copy link
Member

fire commented Aug 20, 2023

We discussed there's a tradeoff between small bug fixes and large overhauls. Since this code is load bearing, smaller changes aren't worth doing the validating for correctness.

@aaronfranke wanted to make a new proposal.

The current discussion is about enhancing the Godot game engine's handling of GLTF nodes. Presently, Godot can only generate one node from a single GLTF node JSON, which limits the creation of complex structures without code duplication.

The proposed solution is to modify the code to allow multiple Godot nodes from a single GLTF node JSON. This would enable creating intricate structures like a physics body with various components without repeating code.

There's also concern about backward compatibility. Some believe the old method should not be preserved, while others suggest an import option for backward compatibility. The consensus is to treat this as a significant version change that upgrades with a checkbox. While the need for change is agreed upon, the specifics are still under discussion.

Summarized by ai

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

We discussed there's a tradeoff between small bug fixes and large overhauls. Since this code is load bearing, smaller changes aren't worth doing the validating for correctness.

@aaronfranke has a proposal to modify the code to allow multiple Godot nodes from a single GLTF node JSON.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

@lyuma I think this is good now. Can you check?

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Sep 5, 2023
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

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

Looks great. This was indeed one of the trickier edge cases and you'd otherwise have to git blame to find the PR and the linked issue with the particular testcase that led this to be added.

@YuriSizov YuriSizov merged commit 356624c into godotengine:master Sep 6, 2023
16 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@aaronfranke aaronfranke deleted the gltf-skin-mesh-comment branch September 7, 2023 16:57
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.

6 participants