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

Cosmetic changes in GLTF node generation code #79775

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

aaronfranke
Copy link
Member

This PR has some internal renames in the GLTF node generation code and some reordering.

  • Reorder _generate_scene_node and _generate_skeleton_bone_node arguments to put the node index first. The index is more "core" information to the node generation so it should come first. This may seem slight/pedantic but it was prompted by another change coming after this PR in which it will make more sense to order the arguments this way.
  • Fix missing p_ prefix at the start of parameter names in _generate_scene_node.
  • Rename _assign_scene_names to _assign_node_names because it assigns node names.
  • Rename Ref<GLTFNode> n to gltf_node to improve readability.

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.

Some of the code comments were changed to have a p_ underscore, but on general I'm ok with this.

@aaronfranke
Copy link
Member Author

That was on purpose to match the variable names but I can undo it.

@aaronfranke aaronfranke force-pushed the gltf-node-gen-cosmetic branch from e95a370 to 9551043 Compare July 30, 2023 19:01
@fire
Copy link
Member

fire commented Jul 31, 2023

There's some odd errors? Want to bump the cicd?

Have you been using this in your project? I was going to import the model and spot check https://github.com/KhronosGroup/glTF-Sample-Models but didn't get around to it. Maybe you can assist.

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.

Cosmetic

@akien-mga akien-mga merged commit 3988bf6 into godotengine:master Aug 2, 2023
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the gltf-node-gen-cosmetic branch August 2, 2023 16:07
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.

3 participants