Open Flynsarmy opened 1 month ago
Wasn't get_external_skeleton/set_external_skeleton an old API? If so, how would this change be done without breaking compatibility.
furthermore, it is common in node APIs for references to other nodes to be NodePath, not raw object references.
in other words, I believe the get_skeleton() API is the anomaly.
I think renaming getters and properties to _path is going to lead to lots of api churn
get_skeleton()
was IMO a good addition for a few of reasons.
get_external_skeleton()
is difficult to use in that if you're calling my_bone_attachment.get_external_skeleton()
from another node (like your scene root), the path is relative to the bone attachment, not the script you're calling from and it can often be annoying to fix up the discrepancy. Back to this proposal though, whether get_skeleton()
was a good addition or not, we now have two methods that sound like they'd return similar things but they don't which is a potential problem. My proposal was to deprecate the old and replace with a method name that makes more sense. The old method still works and therefore doesn't break compatibility however developers are directed to stop using it and move to the new method.
It would be interesting to see just how many places we're using badly named NodePath methods - there may not be enough of them out there for this to become a big deal though that's up to the maintainers discretion. Do we rip the bandaid off and fix them all or do we leave them until they become a problem? Do we add methods to return the objects where it makes sense or not? Issues for another proposal.
Describe the project you are working on
Any using BoneAttachment3D.
Describe the problem or limitation you are having in your project
With the introduction of https://github.com/godotengine/godot/pull/95643 we now have
get_skeleton()
and aget_external_skeleton()
methods. The former returns aSkeleton3D
node, the latter confusingly returns aNodePath
.Describe the feature / enhancement and how it helps to overcome the problem or limitation
Rename
get_external_skeleton()
toget_external_skeleton_path()
.Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams
Deprecate
get_external_skeleton()
and introduce a newget_external_skeleton_path()
method.If this enhancement will not be used often, can it be worked around with a few lines of script?
It can be worked around by calling
get_external_skeleton()
expecting aSkeleton3D
, being confused when you get an error, reading the docs then correcting your code.Is there a reason why this should be core and not an add-on in the asset library?
What we have at the moment is already a core feature - however in its current state its confusing at best and deceptive at worst.
Ninja edit: Setters too.