-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add getConnections
method and deprecate getSkeleton
for bodyPose
#227
Conversation
This is great! Before merging I just want to check with @MOQN @gohai and @jackbdu who are teaching with
I'd like to merge this relatively soon so that we can get it into an upcoming release. Thank you! |
Thank you - this looks great.
My only comment would be about „depreciated and will be removed in the
future“:
I don’t know if we /really/ want to take the old method away in the future,
especially since it such an uncomplicated mapping, but if we wanted to: how
about a warning to the console (printed once per program invocation) „
getSkeleton will be removed from ml5 releases after April 2026 - use
getConnections() instead“?
|
Thank you @gohai, I think that's a great suggestion! I think the idea of deprecated is that it would never be removed until we have a major release (2.0) at which time we might introduce breaking changes. I'm also ok with keeping both "forever" and having |
That makes a lot of sense. Since it is in materials, should we maybe call
it an „alias“ for the other function - and possibly even keep it in the
reference with a link?
|
This is great! Thank you. We will update the website once it's applied to the library! |
All looks great to me! Looking forward to the next release. |
src/BodyPose/index.js
Outdated
* An alias for `getConnections`. This method is deprecated and will be removed in the future. | ||
* @returns {number[][]} an array of pairs of indices containing the connected keypoints. | ||
* @public | ||
* @deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last little detail here. Following up on my discussion with @gohai, would it make sense to take out "will be removed in the future" and well as "@deprecated" and just leave both methods in, one as an alias? Or is that confusing / bad practice? Would this make the documentation confusing to have two methods that do the same thing? cc @MOQN @QuinnHe @alanvww
We can always change our mind later, we don't have to feel this is set in stone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can take it out for now!
This had me really ponder; please allow me to share my humble thoughts. 🙏 I initially thought that it would be best if the alias were deprecated and removed in the future to minimize confusion. I really like This is based on my experience with other motion capture systems; I mostly used the term 'skeleton' rather than 'connections' while using their SDKs and relevant libraries. I assume that some users and our students might search for references using the keyword 'skeleton.' So, in this case, it would be great if they could find |
Thank you @MOQN for your thoughts! This is good to go! Merging! I believe this is the last piece of the puzzle for the next release to incorporate these new methods and data structures! |
This PR adds the
getConnections
method tobodyPose
and deprecates the originalgetSkeleton
method forbodyPose
. The two methods are functionally identical.getConnections
method and makegetSkeleton
an aliasgetSkeleton
as deprecatedgetConnections
instead ofgetSkeleton