Skip to content
This repository has been archived by the owner on May 13, 2024. It is now read-only.

Ensure that absent bone names work #284

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

appgurueu
Copy link
Contributor

@appgurueu appgurueu commented Jan 30, 2024

Achieved by refactoring the code to use std::optional<std::string> instead of core::stringc.

Soft prerequisite for gltf support: gltf does not require bones to be named. We want to support models with "unnamed" bones.

Corresponding Minetest changes: minetest/minetest#14330

How to test

  1. Read carefully (the changes should be pretty straightforwardly fine "per construction")
  2. Regression tests: Do .b3d models like Sam still work? .x models (like our "cool guy" in the demo)? Attachments to bones?

cool guy :] haha

Achieved by refactoring the code to use
`std::optional<std::string>` instead of `core::stringc`.
@appgurueu appgurueu added the enhancement New feature or request label Jan 30, 2024
@appgurueu appgurueu marked this pull request as draft January 30, 2024 23:15
@sfan5
Copy link
Member

sfan5 commented Feb 1, 2024

Wondering why you didn't just make it a std::string? or is "" somehow reserved?

@appgurueu
Copy link
Contributor Author

Wondering why you didn't just make it a std::string? or is "" somehow reserved?

Two reasons:

  1. I'm not sure whether the empty string would be an admissible bone / scene node name. I don't see a definitive reason why it shouldn't work, at least. If it is, I don't want to mess with that.
  2. std::optional forces me to handle the "absence" case everywhere. It makes it explicit when a name is absent, forcing me to handle that correctly, whereas "empty name means absence" is just a convention that's easy to (accidentally) violate.

include/ISceneNode.h Show resolved Hide resolved
include/ISkinnedMesh.h Show resolved Hide resolved
@sfan5 sfan5 merged commit f150409 into minetest:master Feb 6, 2024
19 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants