Skip to content
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

model handpose #15

Merged
merged 11 commits into from
Jul 12, 2023
Merged

model handpose #15

merged 11 commits into from
Jul 12, 2023

Conversation

ziyuan-linn
Copy link
Member

  • Implement the palm and hand landmark detection feature.
  • Upgrade the old model to the MediaPipe hand-pose-detection model.
  • Add a p5 example that demonstrates this feature.
  • Fix a bug where ml5.handpose() does not return the model correctly when the callback is not supplied
  • Bring over the test file from the old library (untested)

This new model now supports 3D hand landmarks and tracks multiple hands simultaneously! A handpose example is also added to the example directory. Please let me know if I should change anything here or work on anything else.

@ziyuan-linn ziyuan-linn requested review from shiffman and gohai July 3, 2023 19:44
yining1023
yining1023 previously approved these changes Jul 8, 2023
Copy link
Member

@yining1023 yining1023 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!! This is amazing, @ziyuan-linn! The model and the example work so well, great grest job!! 👏 👏 👏

Left some minor and unblocking comments inline. I noticed a small thing where the flipHorizontal might need to be wrapped in {}. Feel free to merge it after solving the flipHorizontal issue!

For the next steps, I'm thinking:

  1. Maybe we can talk about the output format with the rest of the team in the next meeting. I think it would be nice to expose some keypoints("wrist", "thumb", "indexFinger"...) by name to users, like we did in posenet
[
    {
        "handedness": "Left",
        "wrist": { <-----------------------add
            "x": 57.2648811340332,
            "y": 489.2754936218262,
            "z": 0.00608062744140625,
            "confidence": 0.89
        },
        "thumb": [ <-----------------------add
            {
                "x": 57.2648811340332,
                "y": 489.2754936218262,
                "z": 0.00608062744140625,
                "confidence": 0.89,
            },
            {...},
            {...},
            {...},
            {...},
        ],
        "indexFinger":[], <-----------------------add
        "middleFinger":[], <-----------------------add
        "ringFinger":[], <-----------------------add
        "pinky":[], <-----------------------add
        "keypoints": [
            {
                "x": 57.2648811340332,
                "y": 489.2754936218262,
                "name": "wrist"
            },
        ],
        "keypoints3D": [
            {
                "x": -0.03214273601770401,
                "y": 0.08357296139001846,
                "z": 0.00608062744140625,
                "name": "wrist"
            }
        ],
        "score": 0.9638671875,
    },
    {
       "handedness": "Right",
       ...
    }
]

Alternatively, we can also add each point in each finger as well. More info.

[
  'wrist',
  'thumb_cmc',
  'thumb_mcp',
  'thumb_ip',
  'thumb_tip',
  ...
]
  1. We can bring over the jest test script since this PR adds test now.
  2. Sth we can talk about in the meeting is that I noticed that handedness definition was a little confusing to me. I thought it can detect if it was left or right hand, but the results were always the opposite to me. Maybe it means the hand on the left side of the screen?

examples/Handpose/script.js Outdated Show resolved Hide resolved
video = createCapture(VIDEO);
video.size(width, height);

const options = { maxHands: 2 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The model's default option is maxHands: 2, I thought about not passing options here in the example, but I think it's very helpful to let people know how to pass custom options. Maybe we could leave a comment here saying maxHands: 2 is the default option?

The other default options are:

// When creating the model
{
  modelType: 'full',
  runtime: "mediapipe" // or 'tfjs', tfjs also works
}
// When calling estimate
{ 
  flipHorizontal: false,
  staticImageMode: false,
}

I think flipHorizontal could be helpful to mention too. We can also mention it on the website, and not in this example.

More info

src/Handpose/index.js Outdated Show resolved Hide resolved
const modelConfig = {
runtime: "mediapipe", // use MediaPipe runtime by default
solutionPath: "https://cdn.jsdelivr.net/npm/@mediapipe/hands", // fetch model from mediapipe server
...this.config,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that we also allow people to pass two kinds of options, the options for creating the model, and the options when calling estimateHands.
It's a tiny thing and totally optional, we could only read the options we need here which would be:

{
  maxHands: this.config?.maxHands ?? 2,
  runtime: this.config?.runtime ?? "mediapipe",
}

src/Handpose/index.test.js Show resolved Hide resolved
Co-authored-by: Yining Shi <[email protected]>
@ziyuan-linn
Copy link
Member Author

@yining1023 Thank you again for your review! I made changes to the example file as you suggested. I also added filtering to the option arguments for both initialization and inference so we only pass in the options needed.

I will merge this now! I think the next step would be adding comments about the options in the example and wrapping the prediction output in a more user-friendly format after talking about it with the team next meeting.

@ziyuan-linn ziyuan-linn merged commit b751612 into main Jul 12, 2023
@gohai gohai deleted the model-handpose branch July 25, 2023 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants