Tool position changes if sim is launched while zoomed out

[Nancy-Salpepi]

Test device MacBook Air M1 chip

Operating System 13.3.1

Browser Safari/Chrome

Problem description For https://github.com/phetsims/qa/issues/938 and https://github.com/phetsims/qa/issues/939 in studio: the tools will change position if the sim was launched while zoomed in.

Steps to reproduce

1. On either screen, press Zoom out button once
2. Place tools in the scene
3. Press Launch

Visuals Studio:

Standard Wrapper:

[pixelzoom]

I think that @Nancy-Salpepi means "while zoomed out", not "while zoomed in". Because the steps to reproduce say to "press Zoom out button".

Reproduced in master. Verified that this is a problem for all tools -- rulers and position markers. We're probably not applying the zoom transform. Or maybe we are applying the zoom transform and shouldn't be?

[pixelzoom]

This may be a problem with the order in which state is restored. In PositionMarkerNode.ts, I see:

    // Origin is at centerTop.
    positionMarker.positionProperty.link( position => {
      this.centerTop = zoomTransformProperty.value.modelToViewPosition( position );
    } );

    // Update the marker's model position to match this Node's view position, so that the marker remains stationary
    // in the view, and the model position is correct.
    zoomTransformProperty.lazyLink( ( zoomTransform: ModelViewTransform2 ) => {
      positionMarker.positionProperty.value = zoomTransform.viewToModelPosition( this.centerTop );
    } );

And in GORulerNode.ts, I see something similar with ruler.positionProperty and zoomTransformProperty. So depending on the order in which positionProperty and zoomTransformProperty are restored, this could end up with an incorrect result.

[pixelzoom]

This change did not fix the problem for PositionMarkerNode.ts:

    zoomTransformProperty.lazyLink( ( zoomTransform: ModelViewTransform2 ) => {
+     if ( !phet.joist.sim.isSettingPhetioStateProperty.value ) {
        positionMarker.positionProperty.value = zoomTransform.viewToModelPosition( this.centerTop );
+     }
    } );

[pixelzoom]

This change (lazyLink => link) did not fix the problem for PositionMarkerNode.ts:

-   zoomTransformProperty.lazyLink( ( zoomTransform: ModelViewTransform2 ) => {
+   zoomTransformProperty.link( ( zoomTransform: ModelViewTransform2 ) => {
      ruler.positionProperty.value = ( this.ruler.orientation === 'vertical' ) ?
                                     zoomTransform.viewToModelPosition( this.leftBottom ) :
                                     zoomTransform.viewToModelPosition( this.leftTop );
    } );

[pixelzoom]

This change did not fix the problem for PositionMarkerNode.ts:

-    zoomTransformProperty.lazyLink( ( zoomTransform: ModelViewTransform2 ) => {
+    zoomTransformProperty.lazyLink( ( zoomTransform: ModelViewTransform2, oldZoomTransform: ModelViewTransform2 ) => {
+      if ( oldZoomTransform ) {
+        this.centerTop = oldZoomTransform.modelToViewPosition( positionMarker.positionProperty.value );
+      }
       positionMarker.positionProperty.value = zoomTransform.viewToModelPosition( this.centerTop );
     } );

[pixelzoom]

This does fix the reported problem in PositionMarkerNode.ts. But it introduces a new problem - the position marker does not remain stationary when zooming in/out. And it duplicates the listener for positionMarker.positionProperty.

    zoomTransformProperty.lazyLink( ( zoomTransform: ModelViewTransform2 ) => {
+     this.centerTop = zoomTransformProperty.value.modelToViewPosition( positionMarker.positionProperty.value );
      positionMarker.positionProperty.value = zoomTransform.viewToModelPosition( this.centerTop );
    } );

[pixelzoom]

This fixes the problem and keeps the marker stationary when zooming in/out. But it still duplicates the listener for positionMarker.positionProperty.

    zoomTransformProperty.lazyLink( ( zoomTransform: ModelViewTransform2 ) => {
+     if ( phet.joist.sim.isSettingPhetioStateProperty.value ) {
+       this.centerTop = zoomTransformProperty.value.modelToViewPosition( positionMarker.positionProperty.value );
+     }
      positionMarker.positionProperty.value = zoomTransform.viewToModelPosition( this.centerTop );
    } );

A similar change fixes GORulerNode.ts, duplicating the listener for ruler.positionProperty:

     zoomTransformProperty.lazyLink( ( zoomTransform: ModelViewTransform2 ) => {
+      if ( phet.joist.sim.isSettingPhetioStateProperty.value ) {
+        const viewPosition = zoomTransformProperty.value.modelToViewPosition( ruler.positionProperty.value );
+        if ( this.ruler.orientation === 'vertical' ) {
+          this.leftBottom = viewPosition;
+        }
+        else {
+          this.leftTop = viewPosition;
+        }
+      }
       ruler.positionProperty.value = ( this.ruler.orientation === 'vertical' ) ?
                                      zoomTransform.viewToModelPosition( this.leftBottom ) :
                                      zoomTransform.viewToModelPosition( this.leftTop );
    } );

Making the above changes would be an awful hack. It exploits the fact that zoomTransformProperty is being restored before positionProperty. If that were to change, then the problem would occur again.

[pixelzoom]

Addressed in the above commit. @KatieWoe since @Nancy-Salpepi is out, could you please verify? Verify in master using the steps reported in https://github.com/phetsims/geometric-optics/issues/467#issue-1707547723. If it checks out OK, please close.

[KatieWoe]

This looks good in master, but https://github.com/phetsims/geometric-optics/issues/469 still occurs. If these are caused by the same issue, do you still want this closed?

[pixelzoom]

Thanks @KatieWoe. It turns out that #469 is somewhat different, so it's OK to close this issue.

[pixelzoom]

In the above commits, I improved the documentation to better describe why this fix is needed.

I also ensured that positionProperty is adjusted by zoomTranformProperty's listener only when we are not restoring state. When we are restoring state, the value of positionProperty is correct and should not be disturbed.