Zoom feature results in incorrect behavior of RewardNode.

[pixelzoom]

The general zoom feature is not working properly with RewardNode. RewardNode does not adjust to fill the visibile bounds of the ScreenView. See for example phetsims/equality-explorer#176. And this seems to be happening with all sims that use RewardNode.

I believe @jessegreenberg was the developer for the zoom feature, so I'll start by assigning to him. We'll start with high prioirity, because this impacts the RC testing for Equality Explorer 1.1, phetsims/qa#735. If you'd like to verify whether this needs to be fixed for Equality Explorer 1.1 (and possibly adjust the priority), please consult with @amanda-phet.

[kathy-phet]

Thanks for making the issue, I agree this should not stop publication releases at the moment, and while I think we should investigate this issue and fix it in the long run, this issue seems like something that can wait a bit since its consequences are pretty minor. I've added it to quarterly planning to discuss if we do it in Q1 2022.

[jessegreenberg]

RewardNode uses CanvasNode and sets canvasBounds for the content when initialized and when the ScreenView transform changes. In the case of pan/zoom an ancestor of the ScreenView has a transform that changes so updateBounds is never called.

[jessegreenberg]

We can use TransformTracker to watch transforms along the Trail to the RewardNode. This means using getUniqueTrail to get the Trail to the RewardNode, disallowing DAG. But RewardNode already uses getUniqueTrail to find the ScreenView to attach the listener to, so this has always been a limitation for RewardNode. Ill give this a try.

[jessegreenberg]

This is potentially something that shouldn't require too much work, and was marked as a goal for Q1 2022. Removing the deferred label.

[jessegreenberg]

The change to use TransformTracker to watch transforms of any ancestor (not just the ScreenView) to update RewardNode bounds was done in the above commit.

I tested this by zooming in and out of the RewardScreenView and also by verifying the case in phetsims/equality-explorer#176 was fixed (RewardNode looks correct when it starts animating while already zoomed in).

EDIT: An easier way to test this case is to add a conditional to RewardScreenView.step so you can control from the dev tools when the RewardNode starts animating and zoom in first.

  step( timeElapsed ) {
    if ( window.running ) {
      this.rewardNode.step( timeElapsed );
    }
  }

[jessegreenberg]

@samreid since you were the original author of RewardNode and previously worked on canvasDisplayBounds can you please review this change? Feel free to pass back to me if you don't have time at the moment and Ill solicit another reviewer.

[samreid]

Working copy patch for the review:

Index: js/demo/RewardScreenView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/demo/RewardScreenView.js b/js/demo/RewardScreenView.js
--- a/js/demo/RewardScreenView.js	(revision 45af27c74fe03a2908ecaf9caa778e2ce64c8e0e)
+++ b/js/demo/RewardScreenView.js	(date 1644789901330)
@@ -47,7 +47,9 @@
 
   // @public
   step( timeElapsed ) {
-    this.rewardNode.step( timeElapsed );
+    if ( window.running ) {
+      this.rewardNode.step( timeElapsed );
+    }
   }
 }
 
Index: js/RewardNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/RewardNode.js b/js/RewardNode.js
--- a/js/RewardNode.js	(revision 45af27c74fe03a2908ecaf9caa778e2ce64c8e0e)
+++ b/js/RewardNode.js	(date 1644790141455)
@@ -223,7 +223,8 @@
         // These bounds represent the full window relative to the scene. It is transformed by the inverse of the
         // ScreenView's matrix (globalToLocalBounds) because the RewardNode is meant to fill the ScreenView. RewardNode
         // nodes are placed within these bounds.
-        this.canvasDisplayBounds = trailFromScreenViewToThis.globalToLocalBounds( options.display.bounds );
+        console.log( options.display.bounds );
+        this.canvasDisplayBounds = trailFromScreenViewToThis.globalToLocalBounds( options.display.bounds.eroded( 100 ) );
 
         const local = this.globalToLocalBounds( options.display.bounds );
         this.setCanvasBounds( local );

[samreid]

At today's quarterly planning meeting, it was requested to increase the priority of this review. Maybe get to it within a few weeks?

[samreid]

I tested zooming in before enabling the reward node. I tried eroding the bounds of the reward node, so I could see the edge. Everything seemed to behave as expected.

From the code inspection:

• It looks like updateBounds can be converted from a class instance variable to a temporary closure variable, like const updateBounds = ...
• It seems that adjusting the position within the scene graph would cause incorrect transforms. Can we add (a) an assertion that the position within the scene graph doesn't change or (b) documentation about that constraint?

Everything else seems good, nice work. Back to @jessegreenberg

[jessegreenberg]

Great ideas, thanks. I added a note of doc and an assertion to make sure that the trail to the RewardNode hasn't changed between updateBounds calls. Thanks for reviewing, back to you @samreid.

I tested the assertion with this patch in RewardScreenView.ts:

Index: js/demo/RewardScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/demo/RewardScreenView.ts b/js/demo/RewardScreenView.ts
--- a/js/demo/RewardScreenView.ts	(revision efe7863e2835c3adfe5c40193e9fb22b2bc9eca2)
+++ b/js/demo/RewardScreenView.ts	(date 1657224362486)
@@ -15,10 +15,14 @@
 import RewardNode from '../RewardNode.js';
 import vegas from '../vegas.js';
 import Tandem from '../../../tandem/js/Tandem.js';
+import IntentionalAny from '../../../phet-core/js/types/IntentionalAny.js';
 
 export default class RewardScreenView extends ScreenView {
 
   private readonly rewardNode: RewardNode;
+  private readonly dummyNode: IntentionalAny;
+
+  private moveTest = true;
 
   public constructor() {
     super( {
@@ -29,6 +33,9 @@
     this.rewardNode = new RewardNode();
     this.addChild( this.rewardNode );
 
+    this.dummyNode = new phet.scenery.Node();
+    this.addChild( this.dummyNode );
+
     // RewardDialog
     const rewardDialogButton = new RectangularPushButton( {
       content: new Text( 'open RewardDialog', { font: new PhetFont( 20 ) } ),
@@ -52,6 +59,12 @@
 
   public override step( timeElapsed: number ): void {
     this.rewardNode.step( timeElapsed );
+
+    if ( this.moveTest ) {
+      this.moveTest = false;
+      this.removeChild( this.rewardNode );
+      this.dummyNode.addChild( this.rewardNode );
+    }
   }
 }
 

[samreid]

Looks great, thanks! Closing.

[pixelzoom]

The assertion added in efe7863 is causing a new failure in CT. See #111.