phetsims / mean-share-and-balance

"Mean: Share and Balance" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 1 forks source link

Unlink visibleProperty for pipeNodes #245

Closed marlitas closed 4 months ago

marlitas commented 4 months ago

meanShareAndBalance.levelOutScreen.model.pipes.pipe1.isActiveProperty has meanShareAndBalance.levelOutScreen.view.pipes.pipeNode1.visibleProperty as a linked property Seems like redundant information Also seems we don’t need to link this visibleProperty

Does Node allow us to remove this link? If yes, do it, if not don't do it.

marlitas commented 4 months ago

I'm a little confused when looking at the studio tree, so want to check in with @amanda-phet before moving forward.

marlitas commented 4 months ago

@amanda-phet and I talked through this, and I have a solution, but I'm not sure how hacky it is... Maybe it's fine and I'm overthinking it? I posted a question in the developer slack channel. We'll see what they say.

Subject: [PATCH] visibleProperty linkedElement
---
Index: js/level-out/view/PipeNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/level-out/view/PipeNode.ts b/js/level-out/view/PipeNode.ts
--- a/js/level-out/view/PipeNode.ts (revision 2437c6036903c0cd4a49d9402c2d167aaa162c70)
+++ b/js/level-out/view/PipeNode.ts (date 1715974121725)
@@ -97,12 +97,16 @@
     valveNode.addInputListener( valveRotationFireListener );

     const combinedOptions = combineOptions<NodeOptions>( {
-      visibleProperty: pipe.isActiveProperty,
+      phetioVisiblePropertyInstrumented: false,
       enabledProperty: arePipesEnabledProperty,
       children: [ pipeRectangle, pipeStrokeLeft, pipeStrokeBottom, pipeStrokeTop, pipeStrokeRight, valveNode ]
     }, options );
     super( combinedOptions );

+    pipe.isActiveProperty.link( isActive => {
+      this.visible = isActive;
+    } );
+
     this.valveNode = valveNode;
     // Set position related to associated cup
     this.x = pipe.position.x + MeanShareAndBalanceConstants.CUP_WIDTH + LINE_WIDTH;
marlitas commented 4 months ago

I met with @samreid and @zepumph and they were not fans of the above workaround.

We agreed that this is not worth pursuing further, but if designers have questions or concerns are happy to have a meeting to talk more.

marlitas commented 4 months ago

@amanda-phet feel free to close if there are no further requests here.