-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 labels: add enum to avoid misspelling and keep up-to-date list documented #13586
glTF labels: add enum to avoid misspelling and keep up-to-date list documented #13586
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much nicer pattern: I really like this.
Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Rob Parrett <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The from_asset(path)
thing reads a little weird, it would be nice if the asset label was a postfix thing like the string labels are but I'm not sure how that would even be implemented. Maybe something like GltfPath::new(<path>).label(<GltAssetLabel>)
.
Anyway, not a huge deal, having the enum in place is definitely a good thing and makes it way more self documenting.
LGTM
I have a moderate preference for this syntax, but if you don't get to it by this weekend I'm merging this on Monday anyways. |
Great! I think this is impactful enough to have a small section in the release note. |
I didn't manage to have an impl of this that I like, with the fact that |
Well it was worth a try, but it's not a huge deal if there's no clean solution. I'm happy with the PR as is and if someone finds something cleaner in the future we can always change it. |
Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1327 if you'd like to help out. |
Objective
Solution