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 append_gltf_node to GLTFState #96468

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Sep 2, 2024

This PR adds append_gltf_node to GLTFState, and append_child_index to GLTFNode.

The append_gltf_node method is mostly the same as the previous internal _create_gltf_node, but cleaned up. Here are some of the differences:

  • append_gltf_node is responsible for determining the index it's inserting into, and returns it. The API of _create_gltf_node basically just trusted that the caller did not get it wrong, now it's always correct by design (to be clear the old code behaved correctly, but only because it was used correctly in the one place it was used).
  • append_gltf_node will handle setting the given node as a root node, if the parent was -1. This responsibility was previously handled by GLTFDocument::_convert_scene_node.
  • append_gltf_node has detailed documentation explaining when it can be called and what input is valid.
  • If append_gltf_node is used to append the current node during convert_scene_node extension code, then it won't also be appended by GLTFDocument. This allows extensions to "move" a node to another parent on export.
  • If the returned node's index is -1, it will be appended automatically. If -2 or less, it will be excluded.

I am proposing to expose this method because I need it in a GLTFDocumentExtension class for export logic. Currently the glTF exporter code expects that one Godot node should become one glTF node, and there's no (easy) way to make multiple glTF nodes for one Godot node. With this method exposed, now it's easy. I need this to allow my GLTFDocumentExtension class to export multiple glTF nodes from one Godot node, and I have tested this PR with that project.

As for adding append_child_index to GLTFNode, this doesn't work: gltf_node.children.append(5). You would need to make a local variable, then append to it, then set it back. That's 3 lines for what could just be 1, so that's why I'm adding this method to allow doing gltf_node.append_child_index(5).

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.

Looks good to me.

In a separate pr, I would appreciate to see how much can be done to FBXDocument, but I understand it's not directly here. We are trying to restore FBXDocumentExtensions.

@fire fire requested a review from a team September 13, 2024 19:54
@akien-mga akien-mga changed the title GLTF: Add append_gltf_node to GLTFState GLTF: Add append_gltf_node to GLTFState Sep 14, 2024
@akien-mga akien-mga merged commit a9364a9 into godotengine:master Sep 16, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the gltf-append-node branch September 16, 2024 12:56
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.

3 participants