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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - gltf: add a name to nodes without names #4396

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - gltf: add a name to nodes without names #4396

wants to merge 2 commits into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Apr 3, 2022

Objective

  • Animation is using Name to be able to address nodes in an entity free way
  • When loading random animated gltf files, I noticed some had animations without names sometimes

Solution

  • Add default names to all nodes

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 3, 2022
@mockersf mockersf added A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Apr 3, 2022
jakobhellermann
jakobhellermann approved these changes Apr 3, 2022
View reviewed changes
Copy link
Contributor

@jakobhellermann jakobhellermann left a comment

Choose a reason for hiding this comment

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

Can confirm that this works and fixes the AnimatedTriangle.gltf glTF sample model

@alice-i-cecile alice-i-cecile added this to the Bevy 0.7 milestone Apr 3, 2022
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 3, 2022
cart
cart reviewed Apr 4, 2022
View reviewed changes
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I'm on board!

crates/bevy_gltf/src/loader.rs Outdated Show resolved Hide resolved
@mockersf mockersf mentioned this pull request Apr 4, 2022
Closed
@cart
Copy link
Member

cart commented Apr 4, 2022

bors r+

bors bot pushed a commit that referenced this pull request Apr 4, 2022
@mockersf
gltf: add a name to nodes without names (#4396)
703ae5d 
# Objective

- Animation is using `Name` to be able to address nodes in an entity free way
- When loading random animated gltf files, I noticed some had animations without names sometimes

## Solution

- Add default names to all nodes
@cart
Copy link
Member

cart commented Apr 4, 2022 •
edited
Loading

Not blocking, but for scenes with thousands of nodes, allocating strings for each node for each instance of the scene seems like the wrong direction. Maybe (ultimately) "scene entity names" should be tied to the asset and looked up when needed? Or alternatively, we could use copy-on-write strings for names (I didn't mention rust's Cow here because it won't do what we want).

@bors
Copy link
Contributor

bors bot commented Apr 4, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title gltf: add a name to nodes without names [Merged by Bors] - gltf: add a name to nodes without names Apr 4, 2022
@bors bors bot closed this Apr 4, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
@mockersf @aevyrie
gltf: add a name to nodes without names (bevyengine#4396)
a31ec7e 
# Objective

- Animation is using `Name` to be able to address nodes in an entity free way
- When loading random animated gltf files, I noticed some had animations without names sometimes

## Solution

- Add default names to all nodes
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@mockersf @ItsDoot
gltf: add a name to nodes without names (bevyengine#4396)
7c40aed 
# Objective

- Animation is using `Name` to be able to address nodes in an entity free way
- When loading random animated gltf files, I noticed some had animations without names sometimes

## Solution

- Add default names to all nodes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cart cart cart left review comments

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@jakobhellermann jakobhellermann jakobhellermann approved these changes

Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
Bevy 0.7
Development

Successfully merging this pull request may close these issues.
4 participants
@mockersf @cart @alice-i-cecile @jakobhellermann