Error: Assertion failed: Did not find press (phet-io pan/zoom)

[zepumph]

While working on #1048 and #1119

Steps to reproduce:

1. Gravity Force lab in studio
2. Zoom in to the reset all button
3. Press launch
4. Bug number 1: the zoom isn't on the reset all button
5. When I drag around a bit with the mouse I get the following error:

Uncaught Error: Assertion failed: Did not find press
    at window.assertions.assertFunction (assert.js:25)
    at AnimatedPanZoomListener.findPress (MultiListener.js:235)
    at Object.move (MultiListener.js:198)
    at Input.dispatchToListeners (Input.js:1822)
    at Input.dispatchEvent (Input.js:1776)
    at Input.branchChangeEvents (Input.js:1678)
    at Input.moveEvent (Input.js:1613)
    at Input.mouseMoveAction.Action.phetioPlayback (Input.js:312)
    at Action.execute (Action.js:225)
    at Input.mouseMove (Input.js:1122)

[jessegreenberg]

Thanks, also from https://github.com/phetsims/phet-io/issues/1705 - I think something is missing in the instrumentation.

[zepumph]

Let me know if I can help.

[jessegreenberg]

OK I think I have a potential solution to this using stateSetEmitter after discussing with @zepumph. Here is the patch

Index: js/listeners/PanZoomListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/listeners/PanZoomListener.js	(revision 0860467717df0414d77805e18cd7cefdf9c0b73f)
+++ js/listeners/PanZoomListener.js	(date 1607365997649)
@@ -8,8 +8,12 @@
  * @author Jesse Greenberg (PhET Interactive Simulations)
  */
 
+import Property from '../../../axon/js/Property.js';
 import Bounds2 from '../../../dot/js/Bounds2.js';
+import Matrix3 from '../../../dot/js/Matrix3.js';
 import merge from '../../../phet-core/js/merge.js';
+import ModelViewTransform2 from '../../../phetcommon/js/view/ModelViewTransform2.js';
+import Tandem from '../../../tandem/js/Tandem.js';
 import scenery from '../scenery.js';
 import MultiListener from './MultiListener.js';
 
@@ -49,6 +53,26 @@
     this._panBounds = options.panBounds;
     this._targetBounds = options.targetBounds || targetNode.globalBounds.copy();
 
+    // @public {Property.<Bounds2>} - For PhET-iO stateSetEmitter below. The pan bounds of the source
+    // so if the destination bounds are different due to a differently sized iframe or window,
+    // this can be used to determine a correction for the destination targetNode transform.
+    this.sourcePanBoundsProperty = new Property( this._panBounds, {
+      tandem: options.tandem.createTandem( 'sourceBoundsProperty' ),
+      phetioType: Property.PropertyIO( Bounds2.Bounds2IO )
+    } );
+
+    Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( ( state, scopeTandem ) => {
+
+      // The matrixProperty has transformations relative to the global view coordinates of the source simulation,
+      // so it will not be correct if source and destination frames are different sizes. This will map transforamtions
+      // if destination frame has different size.
+      const sourceDestinationTransform = ModelViewTransform2.createRectangleMapping( this.sourcePanBoundsProperty.get(), this._panBounds );
+
+      const newTranslation = this._targetNode.matrix.translation.componentMultiply( sourceDestinationTransform.matrix.getScaleVector() );
+      const scale = this.matrixProperty.get().getScaleVector();
+      this._targetNode.matrix = Matrix3.translationFromVector( newTranslation ).timesMatrix( Matrix3.scaling( scale.x, scale.y ) );
+    } );
+
     // @protected {number}
     this._targetScale = options.targetScale;
   }
@@ -130,6 +154,7 @@
    */
   setPanBounds( panBounds ) {
     this._panBounds = panBounds;
+    this.sourcePanBoundsProperty.set( this._panBounds );
     this.correctReposition();
   }
 

This creates a mapping that will correct the transformation when going from source window to destination window if they are of different sizes. This is mostly correct but have a couple of questions.

• matrixProperty no longer describes the transformation matrix of the targetNode in the downstream sim. Is that problematic?
• There is still some weirdness after the correction because of Sim layout. For example, consider the following where the downstream sim is scaled way down due to aspect ratio, but the navigation bar buttons extend the full width of the screen.
  

If I zoom into the PhetButton, the mapping works well. But notice how the simulation components do not match because the map goes from source bounds -> destination bounds rather than source ScreenView bounds -> destination ScreenView bounds.

I believe that this behavior is correct, but could be strange because the downstream sim doesn't display what you are zoomed in on in the upstream sim. Honestly, this complexity makes me question whether zoom should be in state. Can we convey the transformation matrix in state for the API but not have it carry over to set state of new sim in state wrapper or on launch?

[zepumph]

matrixProperty no longer describes the transformation matrix of the targetNode in the downstream sim. Is that problematic?

Isn't it better though, because the matrix now holds that actual coordinates, taking into consideration the targetScale etc, so that when you transform a global point using it, we won't have the small differences (that looked like rounding problems) that we did last time?

[jessegreenberg]

Dicussing with @zepumph he recommended a way to do this without using stateSetEmitter, in this patch:

Index: js/listeners/PanZoomListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/listeners/PanZoomListener.js	(revision 0860467717df0414d77805e18cd7cefdf9c0b73f)
+++ js/listeners/PanZoomListener.js	(date 1607462937596)
@@ -8,8 +8,12 @@
  * @author Jesse Greenberg (PhET Interactive Simulations)
  */
 
+import Property from '../../../axon/js/Property.js';
 import Bounds2 from '../../../dot/js/Bounds2.js';
+import Matrix3 from '../../../dot/js/Matrix3.js';
 import merge from '../../../phet-core/js/merge.js';
+import ModelViewTransform2 from '../../../phetcommon/js/view/ModelViewTransform2.js';
+import Tandem from '../../../tandem/js/Tandem.js';
 import scenery from '../scenery.js';
 import MultiListener from './MultiListener.js';
 
@@ -49,6 +53,32 @@
     this._panBounds = options.panBounds;
     this._targetBounds = options.targetBounds || targetNode.globalBounds.copy();
 
+    // @public {Property.<Bounds2>} - For PhET-iO stateSetEmitter below. The pan bounds of the source
+    // so if the destination bounds are different due to a differently sized iframe or window,
+    // this can be used to determine a correction for the destination targetNode transform.
+    this.sourcePanBoundsProperty = new Property( this._panBounds, {
+      tandem: options.tandem.createTandem( 'sourceBoundsProperty' ),
+      phetioType: Property.PropertyIO( Bounds2.Bounds2IO )
+    } );
+
+    this.sourcePanBoundsProperty.lazyLink( () => {
+      if ( ( _.hasIn( window, 'phet.joist.sim' ) && phet.joist.sim.isSettingPhetioStateProperty.value ) ) {
+
+        // The matrixProperty has transformations relative to the global view coordinates of the source simulation,
+        // so it will not be correct if source and destination frames are different sizes. This will map transforamtions
+        // if destination frame has different size.
+        const sourceDestinationTransform = ModelViewTransform2.createRectangleMapping( this.sourcePanBoundsProperty.get(), this._panBounds );
+
+        const newTranslation = this._targetNode.matrix.translation.componentMultiply( sourceDestinationTransform.matrix.getScaleVector() );
+        const scale = this.matrixProperty.get().getScaleVector();
+        this._targetNode.matrix = Matrix3.translationFromVector( newTranslation ).timesMatrix( Matrix3.scaling( scale.x, scale.y ) );
+
+        assert && assert( this._targetNode );
+      }
+    }, {
+      phetioDependencies: [ this.matrixProperty ]
+    } );
+
     // @protected {number}
     this._targetScale = options.targetScale;
   }
@@ -130,6 +160,7 @@
    */
   setPanBounds( panBounds ) {
     this._panBounds = panBounds;
+    this.sourcePanBoundsProperty.set( this._panBounds );
     this.correctReposition();
   }

[jessegreenberg]

Yestereday @zepumph and I also worked on a modified implementation of PanZoomListener that did not instrument the matrixProperty, but created a "model" for the bounds of the targetNode that are displayed within the panBounds, with an instrumented Property for this that can be used in the downstream sim so that it doesn't need to know the bounds of the upstream sim. This is a pretty big improvement over #1120 (comment) in terms of PhET-iO, but is also a cool abstraction that I would like to see working. We got pretty far with it, but I cannot spend more time on it now unfortunately. I am going to create a new issue to explore this soon. Here is the patch:

Index: js/listeners/AnimatedPanZoomListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/listeners/AnimatedPanZoomListener.js	(revision 0860467717df0414d77805e18cd7cefdf9c0b73f)
+++ js/listeners/AnimatedPanZoomListener.js	(date 1607547478469)
@@ -628,6 +628,7 @@
 
     this.sourcePosition = this._transformedPanBounds.center;
     this.sourceScale = this.getCurrentScale();
+    console.log( 'setting source scale' );
   }
 
   /**
@@ -782,7 +783,8 @@
         // after applying the scale, the source position has changed, update destination to match
         this.setDestinationPosition( this.sourcePosition );
       }
-      else if ( this.destinationScale !== this.sourceScale ) {
+      else if ( this.destinationScale !== this.sourceScale && this.scaleGestureTargetPosition ) {
+        assert && assert( this.scaleGestureTargetPosition, 'TODO: scaleGestureTargetPosition required' );
 
         // not far enough to animate but close enough that we can set destination equal to source to avoid further
         // animation steps
@@ -869,6 +871,7 @@
    * @param {number} scale
    */
   setDestinationScale( scale ) {
+    console.log( 'setting destination scale' );
     this.destinationScale = this.limitScale( scale );
   }
 
Index: js/listeners/PanZoomListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/listeners/PanZoomListener.js	(revision 0860467717df0414d77805e18cd7cefdf9c0b73f)
+++ js/listeners/PanZoomListener.js	(date 1607547478478)
@@ -8,8 +8,10 @@
  * @author Jesse Greenberg (PhET Interactive Simulations)
  */
 
+import Property from '../../../axon/js/Property.js';
 import Bounds2 from '../../../dot/js/Bounds2.js';
 import merge from '../../../phet-core/js/merge.js';
+import ModelViewTransform2 from '../../../phetcommon/js/view/ModelViewTransform2.js';
 import scenery from '../scenery.js';
 import MultiListener from './MultiListener.js';
 
@@ -49,6 +51,31 @@
     this._panBounds = options.panBounds;
     this._targetBounds = options.targetBounds || targetNode.globalBounds.copy();
 
+    // @protected {Bounds2} - Noramlized model bounds representing the full area
+    // within the pan bounds. The pan bounds in model coordinates extend from (0, 0)
+    // to (1,1).
+    this.modelBounds = new Bounds2( 0, 0, 1, 1 );
+
+    // @protected {ModelViewTransform2} - The transform from model coordinates to view
+    // coordinates, specifically this.modelBounds -> this._panBounds
+    this.modelViewTransform = ModelViewTransform2.createIdentity();
+
+    // @protected {Property.<Bounds2>} - The bounds of the targetNode that is displayed within
+    // this._panBounds, in model coordinates.
+    this.transformedModelBoundsProperty = new Property( this.modelBounds, {
+      tandem: options.tandem.createTandem( 'transformedModelBoundsProperty' ),
+      phetioType: Property.PropertyIO( Bounds2.Bounds2IO )
+    } );
+
+    // when the transformedModelBoundsChange, determine the matrix to apply to the targetNode
+    this.transformedModelBoundsProperty.lazyLink( modelBounds => {
+      if ( this._panBounds.isFinite() && this._transformedPanBounds.isFinite() ) {
+        const viewBounds = this.modelViewTransform.modelToViewBounds( modelBounds );
+        const transform = ModelViewTransform2.createRectangleMapping( this._panBounds, viewBounds );
+        this._targetNode.matrix = transform.matrix.inverted().copy();
+      }
+    } );
+
     // @protected {number}
     this._targetScale = options.targetScale;
   }
@@ -78,10 +105,10 @@
       this._targetNode.bottom = this._panBounds.bottom + ( this._targetNode.bottom - transformedBounds.bottom );
     }
 
-    // update Property with matrix once position has been corrected, using notifyListenersStatic
-    // to avoid creating a new Matrix3 instance
-    this.matrixProperty.set( this._targetNode.matrix );
-    this.matrixProperty.notifyListenersStatic();
+    // the bounds of the targetNode that are visible relative to the global view bounds in model coordinates
+    const scratchViewBounds = this._panBounds.copy();
+    scratchViewBounds.transform( this._targetNode.matrix.inverted() );
+    this.transformedModelBoundsProperty.value = this.modelViewTransform.viewToModelBounds( scratchViewBounds );
   }
 
   /**
@@ -117,7 +144,7 @@
    * @override
    */
   resetTransform() {
-    MultiListener.prototype.resetTransform.call( this );
+    super.resetTransform();
     this.correctReposition();
   }
 
@@ -130,6 +157,7 @@
    */
   setPanBounds( panBounds ) {
     this._panBounds = panBounds;
+    this.modelViewTransform = ModelViewTransform2.createRectangleMapping( this.modelBounds, this._panBounds );
     this.correctReposition();
   }
 
Index: js/listeners/MultiListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/listeners/MultiListener.js	(revision 0860467717df0414d77805e18cd7cefdf9c0b73f)
+++ js/listeners/MultiListener.js	(date 1607531693333)
@@ -110,18 +110,6 @@
     // other Pointer listeners are interrupted in these cases.
     this._backgroundPresses = [];
 
-    // @protected {Property.<Matrix3>} - The matrix applied to the targetNode in response to various
-    // input for the MultiListener
-    this.matrixProperty = new Property( targetNode.matrix.copy(), {
-      phetioType: Property.PropertyIO( Matrix3.Matrix3IO ),
-      tandem: options.tandem.createTandem( 'matrixProperty' )
-    } );
-
-    // assign the matrix to the targetNode whenever it changes
-    this.matrixProperty.link( matrix => {
-      this._targetNode.matrix = matrix;
-    } );
-
     // @private - attached to the Pointer when a Press is added
     this._pressListener = {
       move: event => {
@@ -486,7 +474,7 @@
     sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( 'MultiListener reposition' );
     sceneryLog && sceneryLog.InputListener && sceneryLog.push();
 
-    this.matrixProperty.set( this.computeMatrix() );
+    this._targetNode.matrix = this.computeMatrix();
 
     sceneryLog && sceneryLog.InputListener && sceneryLog.pop();
   }
@@ -737,7 +725,6 @@
    */
   resetTransform() {
     this._targetNode.resetTransform();
-    this.matrixProperty.set( this._targetNode.matrix.copy() );
   }
 }

Mostly, this isn't working with AnimatedPanZoomListener. I think to get it working in full, we need the changes from various input in that class to make changes in model coordinates. That way the matrix for the targetNode can be set only in response to changes to transformedModelPanBoundsProperty.

I am going to clean up #1120 (comment) now to remove the blocking-publication label.

[jessegreenberg]

Commit made above and it is working for me in state wrapper and sim launched from studio. @zepumph can you please review the change? Can we make sourceFramePanBoundsProperty private or something so it is stateful but doesn't show up in studio? Is there anything else that is blocking for this issue?

[jessegreenberg]

Back to me, #1123 popped up after this.

[jessegreenberg]

#1123 is closed, back to @zepumph for #1120 (comment)

[zepumph]

Mostly, this isn't working with AnimatedPanZoomListener. I think to get it working in full, we need the changes from various input in that class to make changes in model coordinates. That way the matrix for the targetNode can be set only in response to changes to transformedModelPanBoundsProperty.

Dang, I'm sorry it didn't work out. Thanks for diving in with me though.

Your commit looks good, I just updated doc. Anything else here?

[jessegreenberg]

I don't think so, thanks @zepumph. Closing this issue.