-
Notifications
You must be signed in to change notification settings - Fork 420
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
Implement holistic landmarker example #355
base: main
Are you sure you want to change the base?
Implement holistic landmarker example #355
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
UI comment: the peek window height should be adjusted. We don't need to show the inference time below the expanding arrow until the developer has expanded the bottom sheet. |
UI: If the blendshapes option is off, let's hide the blendshapes '--' empty adapter |
helperState.value?.minHandLandmarkThreshold ?: 0f | ||
helperState.value = | ||
helperState.value?.copy( | ||
minHandLandmarkThreshold = ((currentLandmarkConfidence.toBigDecimal() + confidence.toBigDecimal()).toFloat()).coerceIn( |
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.
Let's set all options to range from a minimum of 0.3f to a max of 0.9f.
Edit: just saw the email about the segmentation crash, so sending it to eng to verify. |
Does this API support more than 1 person at a time? If so, we should give the dev an option to view 1-3 different poses. |
@schmidt-sebastian I am seeing a lot of "Replacing 305 out of 305 node(s) with delegate (unknown) node, yielding 1 partitions for the whole graph." in our logs, though numbers vary. Is this expected? I also noticed pose detection completely fails if a face isn't detected (even when I lower the face detection threshold to 0) |
) | ||
} | ||
|
||
fun setMinPoseLandmarkConfidence(confidence: Float) { |
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.
Let's make sure we include comments for these functions to denote what they do. Similar to what we've done in other examples (https://github.com/googlesamples/mediapipe/blob/main/examples/pose_landmarker/android/app/src/main/java/com/google/mediapipe/examples/poselandmarker/PoseLandmarkerHelper.kt) so devs can read through comments along with the actual code.
private var categories: MutableList<Category?> = MutableList(52) { null } | ||
|
||
fun updateResults(faceLandmarkerResult: HolisticLandmarkerResult? = null) { | ||
categories = MutableList(52) { null } |
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.
Let's only display the top three rather than all 52.
facePaint.strokeWidth = LANDMARK_STROKE_WIDTH | ||
facePaint.style = Paint.Style.STROKE | ||
|
||
handPaint.color = |
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.
If we're making both hands the same color, the R.color value doesn't need to be 'right hand'
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 think it would be better if we separate left and right hand into two different colors, i have pushed commit to implement this.
Does this match with our expectations?
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.
That sounds good to me!
val currentLandmarkConfidence = | ||
helperState.value?.minFacePresenceThreshold ?: 0f | ||
helperState.value = helperState.value?.copy( | ||
minFacePresenceThreshold = ((currentLandmarkConfidence.toBigDecimal() + confidence.toBigDecimal()).toFloat()).coerceIn( |
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.
'set min facelandmarker confidence' shouldn't accept a delta - a set should set the value to whatever is passed to it. Let's only pass in the new value to these methods and set it rather than using the addition logic.
} | ||
} | ||
|
||
// Update UI after face have been detected. Extracts original |
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.
Comment is a copy/paste from face landmarker. Should update to reflect what actually triggers the action.
…er of results to 3
@PaulTR I'm afraid not, our detecting API only return one person at a time. |
Description
This pull request adds a new holistic landmarker API
Checklist
Type of Change
Please check the relevant option below:
Screenshots