Coalesce modelViewTransforms

[marlitas]

@samreid and I noticed that there are multiple modelViewTransforms and ChartTransforms being created for the NumberLineNode. These should be coalesced, but when we tried to do so the bounds for PlotNode in the AccordionBox got much wider. This is only happening in the first two screens, which may provide insight into the bug.

[marlitas]

Here's a patch:

Subject: [PATCH] Coalesce MVTs
---
Index: js/common/view/CAVPlotNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/CAVPlotNode.ts b/js/common/view/CAVPlotNode.ts
--- a/js/common/view/CAVPlotNode.ts	(revision b5bc622960b034f6d5eeefd130685775d55def32)
+++ b/js/common/view/CAVPlotNode.ts	(date 1683056506901)
@@ -56,7 +56,8 @@
       model.physicalRange,
       model.meanValueProperty,
       model.isShowingTopMeanProperty,
-      model.dataRangeProperty, {
+      model.dataRangeProperty,
+      modelViewTransform, {
         color: 'black',
         includeXAxis: true,
         includeMeanStroke: false,
Index: js/common/view/CAVScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/CAVScreenView.ts b/js/common/view/CAVScreenView.ts
--- a/js/common/view/CAVScreenView.ts	(revision b5bc622960b034f6d5eeefd130685775d55def32)
+++ b/js/common/view/CAVScreenView.ts	(date 1683055678980)
@@ -225,7 +225,9 @@
       model.physicalRange,
       model.meanValueProperty,
       model.isShowingPlayAreaMeanProperty,
-      model.dataRangeProperty, {
+      model.dataRangeProperty,
+      this.modelViewTransform,
+      {
         includeXAxis: false,
         includeMeanStroke: true,
         tandem: options.tandem.createTandem( 'playAreaNumberLineNode' ),
Index: js/common/view/NumberLineNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/NumberLineNode.ts b/js/common/view/NumberLineNode.ts
--- a/js/common/view/NumberLineNode.ts	(revision b5bc622960b034f6d5eeefd130685775d55def32)
+++ b/js/common/view/NumberLineNode.ts	(date 1683055520579)
@@ -43,6 +43,7 @@
     meanValueProperty: TReadOnlyProperty<number | null>,
     isShowingMeanIndicatorProperty: TReadOnlyProperty<boolean>,
     rangeProperty: TReadOnlyProperty<Range | null>,
+    modelViewTransform: ModelViewTransform2,
     providedOptions?: NumberLineNodeOptions
   ) {
 
@@ -86,8 +87,16 @@
       } );
       this.addChild( xAxisNode );
 
+      // TODO: Can we make a 1d MVT since that's all that's needed here, or should this be using the same MVT as the outer MVT?  Like the one that positions the number line node
+      // and puts objects in the right spots.
+      // const modelViewTransform = ModelViewTransform2.createRectangleInvertedYMapping(
+      //   new Bounds2( range.min, 0, range.max, range.getLength() ),
+      //   new Bounds2( 0, -CAVConstants.CHART_VIEW_WIDTH, CAVConstants.CHART_VIEW_WIDTH, CAVConstants.CHART_VIEW_WIDTH )
+      // );
+      // const chartTransform = new ChartTransform();
+
       // For the dot plot, when "mean" is selected, there is a purple overlay on the x-axis (if there is an x-axis)
-      const rangeNode = new Path( new Shape().moveTo( 0, 0 ).lineToRelative( 100, 0 ), {
+      const rangeOverlayNode = new Path( new Shape().moveTo( 0, 0 ).lineToRelative( 100, 0 ), {
         stroke: CAVColors.meanColorProperty,
         lineWidth: 3.2
       } );
@@ -96,22 +105,15 @@
           if ( range !== null ) {
 
             // Do not show any area or text above the data point if the range is 0
-            rangeNode.shape = new Shape()
+            rangeOverlayNode.shape = new Shape()
               .moveTo( modelViewTransform.modelToViewX( range.min ), 0 )
               .lineTo( modelViewTransform.modelToViewX( range.max ), 0 );
           }
-          rangeNode.visible = isShowingMeanIndicator && range !== null;
+          rangeOverlayNode.visible = isShowingMeanIndicator && range !== null;
         } );
 
-      this.addChild( rangeNode );
+      this.addChild( rangeOverlayNode );
     }
-
-    // TODO: Can we make a 1d MVT since that's all that's needed here, or should this be using the same MVT as the outer MVT?  Like the one that positions the number line node
-    // and puts objects in the right spots.
-    const modelViewTransform = ModelViewTransform2.createRectangleInvertedYMapping(
-      new Bounds2( range.min, 0, range.max, range.getLength() ),
-      new Bounds2( 0, -CAVConstants.CHART_VIEW_WIDTH, CAVConstants.CHART_VIEW_WIDTH, CAVConstants.CHART_VIEW_WIDTH )
-    );
 
     const meanIndicatorNode = NumberLineNode.createMeanIndicatorNode( options.includeMeanStroke, false );
     this.addChild( meanIndicatorNode );

[samreid]

These should be coalesced, but when we tried to do so the bounds for PlotNode in the AccordionBox got much wider. This is only happening in the first two screens, which may provide insight into the bug.

I believe that problem will be solved by #170 (comment)

I used the idea in the patch and think it is at a good commit point. I'll commit and request review.

[marlitas]

The changes here look good to me. And any other work with MVT's and AccordionBox will most likely be handled by the restructure in #170.

Closing as completed.