search

minetest / irrlicht

Minetest's fork of Irrlicht
Other
115 stars 87 forks source link

Ensure that absent bone names work #284

Closed appgurueu closed 4 months ago

appgurueu commented 5 months ago

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: https://github.com/minetest/minetest/pull/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](https://github.com/minetest/irrlicht/assets/34514239/3049e2a6-3f61-49a3-9db4-201e27777d79)

sfan5 commented 5 months ago

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

appgurueu commented 5 months ago

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.