Sim becomes unresponsive when blocks are in a grabbed state(alt input) and moved with the mouse #356
Assignees
Labels
type:bug
Something isn't working
Comments
|
Here is what the console says: 
|
|
Tagging phetsims/qa#1136. |
|
I can easily reproduce this. The problem is that DragListener.press has this assertion: Right before this, PressListener.onPress has this code: this.isPressedProperty.value = true; // <== correctly sets to true
// Notify after everything else is set up
this._pressListener( event, this );
callback && callback();GrabDragInteraction is interrupting the listener and setting isPressedProperty.value to false. |
|
The problem is that pressing on the block with the mouse calls MassView.endKeyboardInteraction, which calls @zepumph can you please review/test? The fix is just getting rid of Subject: [PATCH] For focus order, update PDOM element when the shapeProperty changes, see https://github.com/phetsims/density-buoyancy-common/issues/355
---
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts (revision 0263304feda8d78884908ad18f9fccef3ae940e0)
+++ b/axon/js/ReadOnlyProperty.ts (date 1724419654724)
@@ -582,6 +582,14 @@
this.unguardedSet( this.phetioValueType.fromStateObject( stateObject.value ) );
}
+ public lazyDebugger(): void {
+ this.lazyLink( ( newValue, oldValue ) => {
+
+ // eslint-disable-next-line no-debugger
+ debugger;
+ } );
+ }
+
/**
* An observable Property that triggers notifications when the value changes.
* This caching implementation should be kept in sync with the other parametric IOType caching implementations.
Index: scenery/js/listeners/PressListener.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/listeners/PressListener.ts b/scenery/js/listeners/PressListener.ts
--- a/scenery/js/listeners/PressListener.ts (revision c99e98288912e27dff23b914ff67756c193f379a)
+++ b/scenery/js/listeners/PressListener.ts (date 1724419647402)
@@ -304,6 +304,7 @@
this.overPointers = createObservableArray();
this.isPressedProperty = new BooleanProperty( false, { reentrant: true } );
+ this.isPressedProperty.lazyDebugger();
this.isOverProperty = new BooleanProperty( false );
this.looksOverProperty = new BooleanProperty( false );
this.isHoveringProperty = new BooleanProperty( false );
Index: density-buoyancy-common/js/common/view/MassView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/common/view/MassView.ts b/density-buoyancy-common/js/common/view/MassView.ts
--- a/density-buoyancy-common/js/common/view/MassView.ts (revision a5c3250f5c691b7ca2c4e06afcce1074b5a9e829)
+++ b/density-buoyancy-common/js/common/view/MassView.ts (date 1724419889906)
@@ -119,7 +119,7 @@
} );
const endKeyboardInteraction = () => {
- this.grabDragInteraction!.interrupt();
+ // this.grabDragInteraction!.interrupt();
// BackgroundTargetEventListener calls mass.interruptedEmitter.emit(); on mouse/touch down to clean up interaction
// This interrupts keyboard interaction, so we must be graceful in case there was no keyboard interaction. |
zepumph
added a commit
to phetsims/scenery-phet
that referenced
this issue
Aug 23, 2024
…common#356 Signed-off-by: Michael Kauzmann <[email protected]>
samreid
added a commit
that referenced
this issue
Aug 23, 2024
|
@zepumph and I reviewed and agreed that interrupting the mouse drag when we intended to interrupt the keyboard drag was causing the error. Fixed in the commit. |
|
@Matthew-Moore240 please review on phettest. Please close the issue if it seems ok. |
|
@samreid The bug still appears for me on phettest unfortunately. |
|
@Matthew-Moore240 Should we meet on zoom? I'm testing https://bayes.colorado.edu/dev/phettest/buoyancy/buoyancy_en.html?ea&brand=phet with my cache disabled, in mac chrome and no longer able to reproduce the problem. Does any error show in the dev tools console? |
|
@Matthew-Moore240 and I discovered that it seems fixed on Chrome, but on Firefox, we both get this new stack trace: I'll investigate. |
|
This later issue is now showing on CT |
|
On firefox, this interaction:
ends with a removePointerConstraint with this stack: removePointerConstraint@http://localhost/chipper/dist/js/density-buoyancy-common/js/common/model/PhysicsEngine.js:370:17
endDrag@http://localhost/chipper/dist/js/density-buoyancy-common/js/common/model/Mass.js:373:19
endKeyboardInteraction@http://localhost/chipper/dist/js/density-buoyancy-common/js/common/view/MassView.js:101:14
onRelease@http://localhost/chipper/dist/js/density-buoyancy-common/js/common/view/MassView.js:124:11
GrabDragInteraction/this.onRelease@http://localhost/chipper/dist/js/scenery-phet/js/accessibility/GrabDragInteraction.js:287:56
releaseDraggable@http://localhost/chipper/dist/js/scenery-phet/js/accessibility/GrabDragInteraction.js:506:10
blur@http://localhost/chipper/dist/js/scenery-phet/js/accessibility/GrabDragInteraction.js:420:24
focusout@http://localhost/chipper/dist/js/scenery/js/listeners/KeyboardListener.js:175:10
dispatchToListeners@http://localhost/chipper/dist/js/scenery/js/input/Input.js:1700:23
dispatchToTargets@http://localhost/chipper/dist/js/scenery/js/input/Input.js:1728:12
dispatchEvent@http://localhost/chipper/dist/js/scenery/js/input/Input.js:1662:10
dispatchPDOMEvent@http://localhost/chipper/dist/js/scenery/js/input/Input.js:1006:12
Input/this.focusoutAction<@http://localhost/chipper/dist/js/scenery/js/input/Input.js:639:12
execute@http://localhost/chipper/dist/js/tandem/js/PhetioAction.js:137:17
focusOut@http://localhost/chipper/dist/js/scenery/js/input/Input.js:1396:25
run@http://localhost/chipper/dist/js/scenery/js/input/BatchedDOMEvent.js:71:16
fireBatchedEvents@http://localhost/chipper/dist/js/scenery/js/input/Input.js:812:22
batchEvent@http://localhost/chipper/dist/js/scenery/js/input/Input.js:776:14
batchWindowEvent@http://localhost/chipper/dist/js/scenery/js/input/BrowserEvents.js:269:15
onfocusout@http://localhost/chipper/dist/js/scenery/js/input/BrowserEvents.js:652:19This means that the blur causes GrabDragInteraction.onRelease, which again calls debug patch: Subject: [PATCH] Ensure BottleView and BoatView are only created once, see https://github.com/phetsims/density-buoyancy-common/issues/354
---
Index: js/common/model/PhysicsEngine.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/PhysicsEngine.ts b/js/common/model/PhysicsEngine.ts
--- a/js/common/model/PhysicsEngine.ts (revision 3749dbf869ac0ba065c184defc4b60668e9b6423)
+++ b/js/common/model/PhysicsEngine.ts (date 1724700165657)
@@ -363,6 +363,9 @@
* (if the body is getting dragged).
*/
public addPointerConstraint( body: PhysicsEngineBody, position: Vector2 ): void {
+
+ console.log( 'add pointer constraint, body.id', body.id );
+
// Create an empty body used for the constraint (we don't want it intersecting). It will just be used for applying
// the effects of this constraint.
const nullBody = new p2.Body();
@@ -388,6 +391,7 @@
* Updates a pointer constraint so that the body will essentially be dragged to the new position.
*/
public updatePointerConstraint( body: PhysicsEngineBody, position: Vector2 ): void {
+ console.log( 'update pointer constraint for body.id', body.id );
const pointerConstraint = this.pointerConstraintMap[ body.id ];
assert && assert( pointerConstraint );
@@ -401,6 +405,9 @@
* Removes a pointer constraint.
*/
public removePointerConstraint( body: PhysicsEngineBody ): void {
+ console.log( 'remove pointer constraint for body.id', body.id );
+ console.log(new Error().stack);
+ console.log( '/remove pointer constraint for body.id', body.id );
const nullBody = this.nullBodyMap[ body.id ];
const pointerConstraint = this.pointerConstraintMap[ body.id ];
|
|
Taking a look now |
|
I'm pretty stumped here. I cannot figure out how to have the end listener be graceful. It is further complicated by the DragListener in GrabDragInteraction that turns the block draggable when you press on the focusablePath with the mouse. Here is a patch, but probably it is best to pair on this one. Subject: [PATCH] Update keyboard order when mode changes, see https://github.com/phetsims/density-buoyancy-common/issues/355
---
Index: js/common/model/Mass.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/Mass.ts b/js/common/model/Mass.ts
--- a/js/common/model/Mass.ts (revision cac7c01941846b4cadb2b889e132646329499052)
+++ b/js/common/model/Mass.ts (date 1724711827460)
@@ -40,6 +40,7 @@
import MaterialProperty, { MaterialPropertyOptions } from './MaterialProperty.js';
import DensityBuoyancyCommonQueryParameters from '../DensityBuoyancyCommonQueryParameters.js';
import Disposable from '../../../../axon/js/Disposable.js';
+import BooleanIO from '../../../../tandem/js/types/BooleanIO.js';
// For the Buoyancy Shapes screen, but needed here because setRatios is included in each core type
// See https://github.com/phetsims/buoyancy/issues/29
@@ -95,7 +96,8 @@
// Without the matrix applied (effectively in "local" model coordinates)
public readonly shapeProperty: Property<Shape>;
- public readonly userControlledProperty: Property<boolean>;
+ public readonly controlledInteractionsCountProperty = new NumberProperty( 0 );
+ public readonly userControlledProperty: TReadOnlyProperty<boolean>;
public readonly inputEnabledProperty: Property<boolean>;
// This property is like a model value, indicating whether it should be shown
@@ -216,11 +218,10 @@
} );
const tandem = options.tandem;
- this.userControlledProperty = new BooleanProperty( false, {
+ this.userControlledProperty = new DerivedProperty( [ this.controlledInteractionsCountProperty ], count => count > 0, {
tandem: tandem.createTandem( 'userControlledProperty' ),
+ phetioValueType: BooleanIO,
phetioDocumentation: 'For internal use only',
- phetioReadOnly: true,
- phetioState: false,
phetioFeatured: true
} );
@@ -492,9 +493,11 @@
* Starts a physics model engine drag at the given 2d (x,y) model position.
*/
public startDrag( position: Vector2 ): void {
- assert && assert( !this.userControlledProperty.value, 'cannot start a drag when already userControlled' );
- this.userControlledProperty.value = true;
- this.engine.addPointerConstraint( this.body, position );
+ this.controlledInteractionsCountProperty.value = 0;
+ const isStarting = this.controlledInteractionsCountProperty.value === 0;
+ this.controlledInteractionsCountProperty.value += 1;
+ console.log( 'startDrag', this.controlledInteractionsCountProperty.value );
+ isStarting && this.engine.addPointerConstraint( this.body, position );
}
/**
@@ -508,9 +511,11 @@
* Ends a physics model engine drag.
*/
public endDrag(): void {
- if ( this.userControlledProperty.value ) {
+ console.log( 'endDrag', this.controlledInteractionsCountProperty.value );
+ const previous = this.controlledInteractionsCountProperty.value;
+ this.controlledInteractionsCountProperty.value = Math.max( this.controlledInteractionsCountProperty.value - 1, 0 );
+ if ( this.controlledInteractionsCountProperty.value === 0 && previous !== 0 ) {
this.engine.removePointerConstraint( this.body );
- this.userControlledProperty.value = false;
}
}
@@ -579,7 +584,7 @@
this.materialProperty.reset();
this.volumeProperty.reset();
this.containedMassProperty.reset();
- this.userControlledProperty.reset();
+ this.controlledInteractionsCountProperty.reset();
this.gravityForceInterpolatedProperty.reset();
this.buoyancyForceInterpolatedProperty.reset();
Index: js/common/view/MassView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/MassView.ts b/js/common/view/MassView.ts
--- a/js/common/view/MassView.ts (revision cac7c01941846b4cadb2b889e132646329499052)
+++ b/js/common/view/MassView.ts (date 1724711827450)
@@ -119,6 +119,7 @@
} );
const endKeyboardInteraction = () => {
+ console.log( 'end keyboard interaction' );
// BackgroundTargetEventListener calls mass.interruptedEmitter.emit(); on mouse/touch down to clean up interaction
// This interrupts keyboard interaction, so we must be graceful in case there was no keyboard interaction.
@@ -144,8 +145,11 @@
} );
this.grabDragInteraction = new GrabDragInteraction( this.focusablePath, keyboardDragListener, {
- onGrab() {
+ onGrab( event ) {
+ if ( !event.isFromPDOM() ) {
+ return;
+ }
// We want the newer interaction to take precedent, so tabbing to the item should interrupt the previous mouse drag (if applicable).
mass.userControlledProperty.value && mass.interruptedEmitter.emit();
@@ -153,7 +157,8 @@
grabSoundPlayer.play();
mass.startDrag( mass.matrix.translation );
},
- onRelease() {
+ onRelease( event ) {
+ console.log( 'on release' );
endKeyboardInteraction();
},
tandem: Tandem.OPT_OUT,
Index: js/common/model/PhysicsEngine.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/PhysicsEngine.ts b/js/common/model/PhysicsEngine.ts
--- a/js/common/model/PhysicsEngine.ts (revision cac7c01941846b4cadb2b889e132646329499052)
+++ b/js/common/model/PhysicsEngine.ts (date 1724711446342)
@@ -363,6 +363,7 @@
* (if the body is getting dragged).
*/
public addPointerConstraint( body: PhysicsEngineBody, position: Vector2 ): void {
+ console.log( 'physics add', body?.id );
// Create an empty body used for the constraint (we don't want it intersecting). It will just be used for applying
// the effects of this constraint.
const nullBody = new p2.Body();
@@ -389,7 +390,7 @@
*/
public updatePointerConstraint( body: PhysicsEngineBody, position: Vector2 ): void {
const pointerConstraint = this.pointerConstraintMap[ body.id ];
- assert && assert( pointerConstraint );
+ assert && assert( pointerConstraint, `pointer constraint expected for physics body #${body.id}` );
// @ts-expect-error it should have pivotA...
p2.vec2.copy( pointerConstraint.pivotA, PhysicsEngine.vectorToP2( position ) );
@@ -401,7 +402,9 @@
* Removes a pointer constraint.
*/
public removePointerConstraint( body: PhysicsEngineBody ): void {
+ console.log( 'physics remove', body?.id );
const nullBody = this.nullBodyMap[ body.id ];
+ assert && assert( nullBody, `cannot remove body, does not exist: ${body.id}` );
const pointerConstraint = this.pointerConstraintMap[ body.id ];
this.world.removeConstraint( pointerConstraint );
Index: js/common/view/BackgroundEventTargetListener.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/BackgroundEventTargetListener.ts b/js/common/view/BackgroundEventTargetListener.ts
--- a/js/common/view/BackgroundEventTargetListener.ts (revision cac7c01941846b4cadb2b889e132646329499052)
+++ b/js/common/view/BackgroundEventTargetListener.ts (date 1724710325633)
@@ -43,6 +43,7 @@
tandem: Tandem ) {
this.startDragAction = new PhetioAction( ( mass: Mass, position: Vector2 ) => {
+ console.log( 'background start' );
mass.startDrag( position );
}, {
tandem: tandem.createTandem( 'startDragAction' ), |
zepumph
added a commit
to phetsims/scenery
that referenced
this issue
Aug 27, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
zepumph
added a commit
to phetsims/scenery-phet
that referenced
this issue
Aug 27, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
zepumph
added a commit
that referenced
this issue
Aug 27, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
zepumph
added a commit
that referenced
this issue
Aug 27, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
|
I believe this is all sorted. The actual fix above is 6db9aeb. @Matthew-Moore240, can you please test some more multi-input with the blocks and see if you run into any other trouble on Firefox or chrome. Thanks! |
zepumph
added
status:ready-for-review
and removed
dev:help-wanted
|
Firefox is working great, but Chrome has an issue where dragging a block with the mouse still allows control with arrow keys. If you move the mouse and release the arrow keys, the block jumps to the mouse pointer. This issue wasn't present in an earlier version of the sim, so it might be related to a recent fix. Should this be a separate issue? Chrome.Buoyancy.block.zooming.mp4 |
|
Yeah thanks. I was seeing that too. I think that it is good the way it is, at least for now. We generally don't have good patterns or precedent for how we want multi-input to behave (see phetsims/joist#750), and fixing the above seems more on the level of designing than bug fixing. The implementation also doesn't provide an easy solution here because the "draggable" state (allowing arrow key movement) is set when using the mouse as well as keyboard. I personally think the above behavior makes the most sense given our current code base, and haven't been able to find any bugs or assertions from it. Does that seem reasonable to you@ @Matthew-Moore240? Feel free to close. |
|
Sounds reasonable! Closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Test device
Desktop Win 11
Operating System
Windows 11 23H2
Browser
129.0.2
Problem description
Sim becomes unresponsive when any of the three blocks are grabbed using alt input and then using the mouse to move them. You can still tab through the sim using alt input but you still can't move or interact with anything. This occurs on all five screens whenever something is grabbed and you try to move it with the mouse while its in a grabbed state.
Steps to reproduce
Visuals
buoyancy.freezes.when.switching.from.alt.input.to.mouse.mp4
phetsims/qa#1136
The text was updated successfully, but these errors were encountered: