```suggestion * Compatibility note: Clients prior to 5.9.0 only support absolute position and rotation. ```

then do `s/bone_pos/it/` too :)

should be `const BoneOverride &`

nitpick: get rid of `bone_pos` if you want to keep the naming consistent

```suggestion auto push_prop = [L](const char *name, const auto &prop, v3f vec) { ```

```suggestion static void push_bone_override(lua_State *L, const BoneOverride &props) ```

is there a reason for this pattern? `props.position.vector` will be default initialized already

if we're introducing a new api `nil` should probably not silently count for `""`, confusing imo

log_deprecated missing here and below

rebase mistake (how?)

```suggestion { ```

dunno ```suggestion * `interpolation`: Old and new values are interpolated over this timeframe (in seconds) ```

nitpick: this note seems to encourage using this function, however it is deprecated that it supports old clients can also be deduced from below information

``` 2022-10-30 13:28:04: ERROR[Main]: ServerError: AsyncErr: Lua: Runtime error from mod 'lab' in callback on_chat_message(): attempt to index a string value 2022-10-30 13:28:04: ERROR[Main]: stack traceback: 2022-10-30 13:28:04: ERROR[Main]: [C]: in function 'get_bone_override' 2022-10-30 13:28:04: ERROR[Main]: XX/mods/lab/init_126_bone_overrides_12388.lua:29: in function 'func' ``` ```lua local player = assert(minetest.get_player_by_name(name)) print(dump(player:get_bone_override("Head"))) ```

``` 2022-10-30 13:27:06: ERROR[Main]: ServerError: AsyncErr: Lua: Runtime error from mod 'lab' in callback on_chat_message(): attempt to index a userdata value 2022-10-30 13:27:06: ERROR[Main]: stack traceback: 2022-10-30 13:27:06: ERROR[Main]: [C]: in function 'get_bone_overrides' 2022-10-30 13:27:06: ERROR[Main]: XXX/mods/lab/init_126_bone_overrides_12388.lua:29: in function 'func' ``` ```lua local player = assert(minetest.get_player_by_name(name)) print(dump(player:get_bone_overrides())) ```

Inconsistent with `"vec"` from the input. Perhaps using macros in this file would help to avoid such mismatches....

Might it make sense to pass by reference? There's quite some data in this struct.

How about `BoneOverride &props = it.second;` instead of manual overwrites?

also no need for pointers here

no clang-format

static

```suggestion ```

it doesn't look necessary here to create a new object instead of reusing whats already there

proper OOP _would_ be to have these implementations inside `BoneOverride` (not saying you have to)

`interpolate_timer` would be more apt

no reason to use pointers here

```suggestion * `x`, `y` and `z` are in same coordinate system as the model, and in degrees for rotation ```

I think something like `interpolate_dur` or just `interpolate` would be better -- Also mixing tables too much is annoying. Any reason not to use `{ pos = {...}, interpolate = 0, absolute = false }`? Edit: This is double bad because the engine will return a vector with a metatable set and everything but also contains foreign fields. **Definitely needs changing**.

```suggestion * `get_bone_overrides()`: returns all bone overrides as table `{[bonename] = override, ...}` ```

```suggestion * `get_bone_position(bone)`: returns the previously set position and rotation of the bone ```

```suggestion auto it = m_bone_override.find(bone); return it != m_bone_override.end() ? it->second : nullptr; ``` .. because indexing creates a new entry which then shows up in `player:get_bone_overrides()`.

When an error is thrown in one of the checks below (`check_v3f`), this pointer is not freed. Same issue for `l_set_bone_position` Suggestion: Use `std::unique_ptr` + `.release()`

What's the idea behind this conversion? For documentation purposes?

```suggestion if (prev_props != props) ``` For completeness' sake, if anyone ever wants to feed back a pointer obtained from `getBoneOverride`.

`props` should be freed here.

1. BoneOverride's ownership is passed to this function. It must be deleted. 2. `override`. Minor issue. The function is (to my knowledge) not called.

```suggestion * Compatibility note: Clients prior to 5.7.0 only support absolute position and rotation. All values are treated as absolute. ``` But more importantly: what if an old client receives relative values? They will be treated as absolute; totally messing up the visuals. However I cannot find an easy solution for this issue - there is no client-specific handling in `generateUpdateBonePositionCommand`.

Missing indent

I'd move this down, above the compatibility note. This looks like a nested field documentation.

```suggestion const BoneOverrideMap &UnitSAO::getBoneOverrides() const ``` Or could be moved to the header because it's a one-liner.

Unfortunate variable naming again (keyword). I don't comment on all occurrences. It looks like this one was forgotten?

Tip: Put each section into its own scope for readability like this: ```C++ // Rotation { core::quaternion rotation; progress = props->dtime_passed / props->rotation.interpolation_duration; if (progress > 1.0f || props->rotation.interpolation_duration == 0.0f) progress = 1.0f; rotation.slerp(props->rotation.previous, props->rotation.next, progress); if (!props->rotation.absolute) { core::quaternion bone_rot(bone->getRotation() * core::DEGTORAD); rotation = rotation * bone_rot; // first rotate around bone rot, then rot } v3f rot_euler; rotation.toEuler(rot_euler); bone->setRotation(rot_euler * core::RADTODEG); } // Scale { ```

> Putting the body of an if statement on the same line as the condition is strictly prohibited. https://dev.minetest.net/Code_style_guidelines#if_statements Same for the other occurrences you can find, e.g. in `GenericCAO::processMessage`, `UnitSAO::setBoneOverride`.

It looks like you merged this incorrectly, the bone position comment is now gone

Is `auto &prop` possible? If so, that would eliminate the assignments below. If not, it might be necessary to use struct inheritance in `BoneOverride` to have a defined object type in this Lambda.

```suggestion const std::unordered_map<std::string, BoneOverride*> &getBoneOverrides() const; ```

```suggestion writeU8(os, (override->position.absolute & 1) << 0 | (override->rotation.absolute & 1) << 1 | (override->scale.absolute & 1) << 2); ``` In the same style as `PlayerControl`. This 1. ensures proper values and 2. Obvious bit positions.

Idea: Shorten this map type using a `typedef` somewhere in `activeobject.h`.

Suggestion: Move to a Lambda, like above. Also add new lines to separate each section (i.e. table).

Invalid index here and for absolute. Suggestion: Replace the common `"absolute"` and `"interpolation_duration"` code with a Lambda to reduce redundancy and inconsistency.

```suggestion if (lua_isnoneornil(L, 3)) { ``` More new lines to separate the sections would improve the code readability too.

These could be specified as defaults in the `BoneOverride` struct directly.

Unfortunate variable name here as well.

Unnecessary include. This slows down the compile process. ```C++ class BoneOverride; ``` (a few lines below, though)

This is dangerous. Map assignments and object freeing should happen in the same scope to avoid dangling pointers caused by logic errors. Generally, I would prefer to not use pointers in this case. A sequential memcpy on this struct is quick to execute during runtime.

Please include a note for the current protocol version. Even though this does not need a bump, a rough timestamp for this legacy code would be helpful. Also needs an entry in `networkprotocol.h`.

```suggestion const BoneOverride *spec = it.second; ``` 1. To avoid the `override` keyword for syntax highlighting reasons 2. Const to keep the code reasonable and more self-explanatory.

Initializer list is not necessary for classes with default constructors, i.e. `v3*` and `core::quaternion`.

+4 spaces for each indent, especially for the `absolute` part above to easier distinguish the sub-lists.

```suggestion * Shorthand for `set_bone_override(bone, {position = ..., rotation = ...})` using absolute values. ```

Is this a memleak - should I delete the previous override?