Make Skeleton3D::add_bone return the new bone index

[MajorMcDoom]

This is a small quality-of-life change which makes Skeleton3D::add_bone return the newly assigned bone index as part of the same call, instead of requiring an additional function call.

This PR should be backwards-compatible and not break any existing use of the function.

Additionally, the original documentation was also confusing and misleading because it said that get_bone_count would become the new index. However, it fails to mention that this value must be retrieved before adding the bone. It's pretty unintuitive to get the index of a bone before you add it. If you fetch the index afterwards, you'd actually have to use get_bone_count() - 1. As part of this PR, the documentation was updated to avoid this confusion.

New usage:

idx = skel.add_bone("Thumb")

Old usage (correct):

idx = skel.get_bone_count()
skel.add_bone("Thumb")

OR

skel.add_bone("Thumb")
idx = skel.get_bone_count() - 1

Old usage (incorrect):

skel.add_bone("Thumb")
idx = skel.get_bone_count()

[Mickeon]

I do recall other things in the engine that do this, as well. It's also good to know when the bone cannot be added.

[MajorMcDoom]

I do recall other things in the engine that do this, as well. It's also good to know when the bone cannot be added.

I thought that would be an advantage as well, but unfortunately, the function still throws an error, which doesn't really make this a useful check. It just allows graceful continuance if you've written bad code. You'd have to rewrite it anyway.

[MajorMcDoom]

Okay apparently I have no idea how compatibility works lol. Is there something explicit I have to do when I change API? I thought making a void function return something wouldn't break anything, but one of the tests failed? Sorry I'm kinda new at this.

[smix8]

I thought making a void function return something wouldn't break anything ...

It breaks the function sig for a lot of extension and language API hence why those small innocent looking function changes are often not done. In this case it breaks GDExtension but I think C# also does not like this. Not sure about the comp details.

[MajorMcDoom]

I thought making a void function return something wouldn't break anything ...

It breaks the function sig for a lot of extension and language API hence why those small innocent looking function changes are often not done.

I understand. I'm not concerned about when or how likely this is to be merged. What I'd like to know is when a change like this is made, what steps need to be taken to prevent those failed tests? What additional things would one have to edit? Obviously API changes do happen at some point.

[smix8]

what steps need to be taken to prevent those failed tests

For GDExtension there are the validation files that need compatibility methods found in misc/extension_api_validation/. You can look at prs that mod those folders and files to see how it is done. Not sure about C#.

[MajorMcDoom]

I'm at a loss. :(
I've done the compatibility method bindings, and added entries into the 4.2-stable.expected file.
But for some reason, I can't get the build to link, because the project doesn't seem to ever include the .compat.inc file I made so it says my compatibility methods are undefined. What am I doing wrong here? Is there some good documentation for this process?

[akien-mga]

You need to include the skeleton_3d.compat.inc file in skeleton_3d.cpp, see https://github.com/godotengine/godot/blob/master/scene/2d/tile_map.cpp for an example.

[akien-mga]

Thanks!