phetsims / ratio-and-proportion

"Ratio and Proportion" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 4 forks source link

hand moves position in play area when my hand moves out of camera view #506

Open Nancy-Salpepi opened 2 years ago

Nancy-Salpepi commented 2 years ago

Test device MacBook Air m1 chip

Operating System macOS 12.5.1

Browser Safari 15.6.1

Problem description For https://github.com/phetsims/qa/issues/831, with camera input: hands:

When I move my left hand too high or too low so that it is out of camera view, the left hand on the screen will jump to the same position as the right hand on the screen. This doesn't happen if I move my right hand too high/low--the hand will just stay at the top/bottom of the play area.

Visuals yuck! I'm in a video!

https://user-images.githubusercontent.com/87318828/188715932-9fbd1370-dc6f-4881-8c11-0a5db10f870a.mp4

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Ratio and Proportion‬ URL: https://phet-dev.colorado.edu/html/ratio-and-proportion/1.2.0-dev.31/phet/ratio-and-proportion_all_phet.html?cameraInput=hands&showVideo Version: 1.2.0-dev.31 2022-08-24 16:50:40 UTC Features missing: touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/15.6.1 Safari/605.1.15 Language: en-US Window: 1322x704 Pixel Ratio: 2/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 (1.0) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 30 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}
zepumph commented 1 year ago

@emily-phet and I looked at this. Originally I thought it was because it then detected a single hand and controlled the "wrong one", but that doesn't seem to be the case, when the bug displays, it seems to always move back to the center, so I will need to double check on some calculation code in this case.

zepumph commented 1 year ago

When testing this morning with @emily-phet, I was very very easily able to reproduce the issue, but now it is much harder, though not impossible. I think that is potentially a bread crumb about the weird state we are getting into to set the value back to a "middle" point.

zepumph commented 1 year ago

Ok. My best guess from further investigation is that this has to do with how we are "smoothing" values for detection/interaction. When we lose the second hand going off screen, it looks like we actually have a single false positive detection where mediaPipe thinks that both hands are enabled. We have smoothing to determine if we are "interacting" together, but we don't have smoothing about false positives between 1 and 2 hands. Given the current state of the controller, and how it was originally built with only 2 hands in mind, having 1 hand interaction sprinkled in on top, I do not recommend fixing this bug, as I'm pretty sure it would ideally lead to an entire rewrite for the detection.

Tagging @emily-phet, and noting that in my opinion it is still quite usable has far as bugs go. I'm going to mark this deferred. Please comment if you would like more work done here.

Investigation Patch:

```difff Index: js/common/view/StationaryValueTracker.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/StationaryValueTracker.ts b/js/common/view/StationaryValueTracker.ts --- a/js/common/view/StationaryValueTracker.ts (revision b43547c50b6065b63b7a45aafd11cd6aebb66e93) +++ b/js/common/view/StationaryValueTracker.ts (date 1665607801493) @@ -63,6 +63,11 @@ const boxPlotValues = Stats.getBoxPlotValues( boxPlotTempArray ); return Math.abs( boxPlotValues.q3 - boxPlotValues.q1 ) < this.stationaryThreshold; } + + public clear(): void { + this.historyValues.length = 0; + this.isStationaryProperty.value = false; + } } Index: js/common/view/RAPMediaPipe.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/RAPMediaPipe.ts b/js/common/view/RAPMediaPipe.ts --- a/js/common/view/RAPMediaPipe.ts (revision b43547c50b6065b63b7a45aafd11cd6aebb66e93) +++ b/js/common/view/RAPMediaPipe.ts (date 1665610374959) @@ -56,7 +56,7 @@ const O_HAND_GESTURE_DETECTED_HISTORY_LENGTH = 15; // Number of previous states to keep to average out to determine if two (and only two) hands are detected by mediaPipe. -const TWO_HANDS_DETECTED_HISTORY_LENGTH = 10; +const HANDS_DETECTED_HISTORY_LENGTH = 10; // This is also used for one (and only one) hand detection smoothing. // The max value of each hand position vector component that we get from MediaPipe. (x,y,z) const HAND_POSITION_MAX_VALUE = 1; @@ -87,6 +87,7 @@ private consequentViewSounds: ViewSounds; private twoHandsDetectedHistory: boolean[] = []; + private oneHandDetectedHistory: boolean[] = []; private antecedentHandPositions: Vector3[] = []; private consequentHandPositions: Vector3[] = []; private oHandGestureDetectedHistory: boolean[] = []; @@ -152,7 +153,7 @@ // Be more tolerant about if we are interacting with MediaPipe. this.isBeingInteractedWithProperty.value = results ? - this.getSmoothedTwoHandsDetected( results.multiHandLandmarks ) : + this.getSmoothedHandsDetected( results.multiHandLandmarks ) : false; // Though isBeingInteractedWithProperty is tolerant, we actually need two hands to calculate sim changes. @@ -172,11 +173,23 @@ const position = handPositions[ 0 ]; + let toUpdate: StationaryValueTracker; + let toClear: StationaryValueTracker; + // if just one hand is detected, defer to the absolute position of the hand, arbitrarily splitting half way // through the screen. - const stationaryTracker = position.x >= HAND_POSITION_MAX_VALUE / 2 ? this.consequentStationaryTracker : - this.antecedentStationaryTracker; - stationaryTracker.update( position.y ); + if ( position.x >= HAND_POSITION_MAX_VALUE / 2 ) { + toUpdate = this.consequentStationaryTracker; + toClear = this.antecedentStationaryTracker; + } + else { + toUpdate = this.antecedentStationaryTracker; + toClear = this.consequentStationaryTracker; + } + + // TODO: an attempt to fix this issue, but I'm unsure that it is actually doing anything, https://github.com/phetsims/ratio-and-proportion/issues/506 + toClear.clear(); + toUpdate.update( position.y ); } const newTuple = this.tupleFromSmoothing( handPositions ); @@ -232,13 +245,37 @@ ).constrainFields( rapConstants.TOTAL_RATIO_TERM_VALUE_RANGE ); } - private getSmoothedTwoHandsDetected( multiHandLandmarks: HandLandmarks[] ): boolean { - const handsDetected = multiHandLandmarks.length === 2 || multiHandLandmarks.length === 1; - return handleSmoothValue( handsDetected, this.twoHandsDetectedHistory, TWO_HANDS_DETECTED_HISTORY_LENGTH, + private smoothHandsDetectedPredicate( history: boolean[] ): boolean { + return history.filter( _.identity ).length > HANDS_DETECTED_HISTORY_LENGTH / 2; + } + + // Smoothed over time version of if hands are detected. + private getSmoothedHandsDetected( multiHandLandmarks: HandLandmarks[] ): boolean { - // To reduce false positives and false negatives, 50% of the history must have two hands detected. - () => this.twoHandsDetectedHistory.filter( _.identity ).length > TWO_HANDS_DETECTED_HISTORY_LENGTH / 2 - ); + + // TODO: but we still need to update the twoHandDetection here to smooth it away. + // TODO: but if we are smoothed into 2 when we have but one, what is the new position for the second? + // TODO: Why can't smoothing happen after the step were we determine if we are actually going to use a hand or not. . .? + if ( multiHandLandmarks.length === 1 ) { + return handleSmoothValue( true, this.oneHandDetectedHistory, HANDS_DETECTED_HISTORY_LENGTH, + () => this.smoothHandsDetectedPredicate( this.oneHandDetectedHistory ) + ); + } + else if ( multiHandLandmarks.length === 2 ) { + return handleSmoothValue( true, this.twoHandsDetectedHistory, HANDS_DETECTED_HISTORY_LENGTH, + () => this.smoothHandsDetectedPredicate( this.twoHandsDetectedHistory ) + ); + + } + else { + // If not detected this round, smooth based on both one and two handed detection + return handleSmoothValue( false, this.oneHandDetectedHistory, HANDS_DETECTED_HISTORY_LENGTH, + () => this.smoothHandsDetectedPredicate( this.oneHandDetectedHistory ) + ) || + handleSmoothValue( false, this.twoHandsDetectedHistory, HANDS_DETECTED_HISTORY_LENGTH, + () => this.smoothHandsDetectedPredicate( this.twoHandsDetectedHistory ) + ); + } } /**