Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some nodes not instrumented #78

Closed
samreid opened this issue Aug 28, 2017 · 5 comments
Closed

Some nodes not instrumented #78

samreid opened this issue Aug 28, 2017 · 5 comments

Comments

@samreid
Copy link
Member

samreid commented Aug 28, 2017

From https://github.com/phetsims/phet-io/issues/754

@samreid, can you investigate what isn't instrumented with just a screenshot? Everything I could make invisible via the instance proxies is hidden, and these were left over:
screen shot 2016-10-26 at 4 19 46 pm

For phetsims/tasks/issues/718.

Edit: now also for phetsims/tasks/issues/724.

@zepumph can you please investigate?

@zepumph
Copy link
Member

zepumph commented Aug 28, 2017

I am able to instrument everything in the sim easily because they are all nodes. There is one exception. There is hacky behavior with the coils to get the left "front" half to be on top of the wires and the "back" half behind the wires. Here is the trouble in the screen view:

    // move coils to front
    bottomCoilNode.frontImage.detach();
    this.addChild( bottomCoilNode.frontImage );
    bottomCoilNode.frontImage.center = model.bottomCoil.position.plus( new Vector2( CoilNode.xOffset, 0 ) );
    
    
    topCoilNode.frontImage.detach();
    this.addChild( topCoilNode.frontImage );
    topCoilNode.frontImage.center = model.topCoil.position.plus( new Vector2( CoilNode.xOffset + CoilNode.twoOffset, 0 ) );
    model.showSecondCoilProperty.linkAttribute( topCoilNode.frontImage, 'visible' );

After top and bottomCoilNode are added, then a piece of them are detached and moved to the center. This breaks the object oriented nature of having a CoilNode type. It seems sloppy to me, and I'm not sure what is best.

Here is a diff of my changes, I will stash them locally until we figure this out:

$ git diff
diff --git a/js/faradays-law/view/BulbNode.js b/js/faradays-law/view/BulbNode.js
index bdd874c..2084912 100644
--- a/js/faradays-law/view/BulbNode.js
+++ b/js/faradays-law/view/BulbNode.js
@@ -37,7 +37,7 @@ define( function( require ) {
    * @param {Object} [options]
    * @constructor
    */
-  function BulbNode( needleAngleProperty, options ) {
+  function BulbNode( needleAngleProperty, tandem, options ) {
     Node.call( this );

     // Create the base of the bulb
@@ -134,6 +134,7 @@ define( function( require ) {
     bulbBase.left = 0;
     haloNode.center = filament.center;

+    options.tandem = tandem;
     this.mutate( options );

     this.centerX = this.centerX + BULB_X_DISPLACEMENT;
diff --git a/js/faradays-law/view/CoilNode.js b/js/faradays-law/view/CoilNode.js
index 262a5b5..d92f8de 100644
--- a/js/faradays-law/view/CoilNode.js
+++ b/js/faradays-law/view/CoilNode.js
@@ -66,7 +66,7 @@ define( function( require ) {
    * @param {Object} [options]
    * @constructor
    */
-  function CoilNode( coilType, options ) {
+  function CoilNode( coilType, tandem, options ) {
     Node.call( this );

     // support smaller images, so it isn't crazily aliased in Firefox. They are 1/6th the size of the normal images.
@@ -93,6 +93,7 @@ define( function( require ) {

     this.endRelativePositions = coilEndCoordinatesMap[ coilType ];

+    options.tandem = tandem;
     this.mutate( options );
   }

diff --git a/js/faradays-law/view/CoilsWiresNode.js b/js/faradays-law/view/CoilsWiresNode.js
index c11ac7c..7e8c46a 100644
--- a/js/faradays-law/view/CoilsWiresNode.js
+++ b/js/faradays-law/view/CoilsWiresNode.js
@@ -24,7 +24,7 @@ define( function( require ) {
    * @param showSecondCoilProperty
    * @constructor
    */
-  function CoilsWiresNode( aligner, showSecondCoilProperty ) {
+  function CoilsWiresNode( aligner, showSecondCoilProperty, tandem ) {

     Node.call( this );

@@ -118,9 +118,10 @@ define( function( require ) {
     showSecondCoilProperty.linkAttribute( topCoilsWire1, 'visible' );
     showSecondCoilProperty.linkAttribute( topCoilsWire2, 'visible' );

+    this.mutate( { tandem: tandem } );
   }

   faradaysLaw.register( 'CoilsWiresNode', CoilsWiresNode );
-
+
   return inherit( Node, CoilsWiresNode );
 } );
diff --git a/js/faradays-law/view/ControlPanelNode.js b/js/faradays-law/view/ControlPanelNode.js
index 4a567d9..41ccdcd 100644
--- a/js/faradays-law/view/ControlPanelNode.js
+++ b/js/faradays-law/view/ControlPanelNode.js
@@ -65,8 +65,8 @@ define( function( require ) {
       value: false,
       node: new VBox( _.extend( {
         children: [
-          new CoilNode( CoilTypeEnum.TWO_COIL, { isSmall: true, visible: false } ),
-          new CoilNode( CoilTypeEnum.FOUR_COIL, { isSmall: true } )
+          new CoilNode( CoilTypeEnum.TWO_COIL, tandem.createTandem( 'singleCoilRadioButton.twoCoilNode' ), { isSmall: true, visible: false } ),
+          new CoilNode( CoilTypeEnum.FOUR_COIL, tandem.createTandem( 'singleCoilRadioButton.fourCoilNode' ), { isSmall: true } )
         ]
       }, coilButtonGroupOptions ) ),
       tandemName: 'singleCoilRadioButton'
@@ -74,8 +74,8 @@ define( function( require ) {
       value: true,
       node: new VBox( _.extend( {
         children: [
-          new CoilNode( CoilTypeEnum.TWO_COIL, { isSmall: true } ),
-          new CoilNode( CoilTypeEnum.FOUR_COIL, { isSmall: true } )
+          new CoilNode( CoilTypeEnum.TWO_COIL, tandem.createTandem( 'doubleCoilRadioButton.twoCoilNode' ), { isSmall: true } ),
+          new CoilNode( CoilTypeEnum.FOUR_COIL, tandem.createTandem( 'doubleCoilRadioButton.fourCoilNode' ), { isSmall: true } )
         ]
       }, coilButtonGroupOptions ) ),
       tandemName: 'doubleCoilRadioButton'
diff --git a/js/faradays-law/view/FaradaysLawView.js b/js/faradays-law/view/FaradaysLawView.js
index 91f33c6..80a3bd9 100644
--- a/js/faradays-law/view/FaradaysLawView.js
+++ b/js/faradays-law/view/FaradaysLawView.js
@@ -35,12 +35,12 @@ define( function( require ) {
     } );

     // coils
-    var bottomCoilNode = new CoilNode( CoilTypeEnum.FOUR_COIL, {
+    var bottomCoilNode = new CoilNode( CoilTypeEnum.FOUR_COIL, tandem.createTandem( 'fourCoilNode' ), {
       x: model.bottomCoil.position.x,
       y: model.bottomCoil.position.y
     } );

-    var topCoilNode = new CoilNode( CoilTypeEnum.TWO_COIL, {
+    var topCoilNode = new CoilNode( CoilTypeEnum.TWO_COIL, tandem.createTandem( 'twoCoilNode' ), {
       x: model.topCoil.position.x,
       y: model.topCoil.position.y
     } );
@@ -50,14 +50,14 @@ define( function( require ) {

     // voltmeter and bulb created
     var voltmeterNode = new VoltmeterNode( model.voltmeterModel.thetaProperty, tandem.createTandem( 'voltmeterNode' ), {} );
-    var bulbNode = new BulbNode( model.voltmeterModel.thetaProperty, {
+    var bulbNode = new BulbNode( model.voltmeterModel.thetaProperty, tandem.createTandem( 'bulbNode' ), {
       centerX: this.aligner.bulbPosition.x,
       centerY: this.aligner.bulbPosition.y
     } );

     // wires
-    this.addChild( new CoilsWiresNode( this.aligner, model.showSecondCoilProperty ) );
-    this.addChild( new VoltmeterWiresNode( this.aligner, voltmeterNode ) );
+    this.addChild( new CoilsWiresNode( this.aligner, model.showSecondCoilProperty, tandem.createTandem( 'CoilsWiresNode' ) ) );
+    this.addChild( new VoltmeterWiresNode( this.aligner, voltmeterNode, tandem.createTandem( 'voltmeterWithWiresNode' ) ) );

     // bulb added
     this.addChild( bulbNode );
@@ -82,6 +82,7 @@ define( function( require ) {
     this.addChild( bottomCoilNode.frontImage );
     bottomCoilNode.frontImage.center = model.bottomCoil.position.plus( new Vector2( CoilNode.xOffset, 0 ) );

+
     topCoilNode.frontImage.detach();
     this.addChild( topCoilNode.frontImage );
     topCoilNode.frontImage.center = model.topCoil.position.plus( new Vector2( CoilNode.xOffset + CoilNode.twoOffset, 0 ) );
diff --git a/js/faradays-law/view/VoltmeterNode.js b/js/faradays-law/view/VoltmeterNode.js
index beefef0..6924e3a 100644
--- a/js/faradays-law/view/VoltmeterNode.js
+++ b/js/faradays-law/view/VoltmeterNode.js
@@ -119,6 +119,8 @@ define( function( require ) {
     } ) );
     this.addChild( this.minusNode );
     this.minusNode.center = new Vector2( -options.terminalSize, options.rectangleHeight / 2 + options.terminalSize / 2 );
+
+    this.mutate( { tandem: tandem } );
   }

   faradaysLaw.register( 'VoltmeterNode', VoltmeterNode );
diff --git a/js/faradays-law/view/VoltmeterScale.js b/js/faradays-law/view/VoltmeterScale.js
index df27c6d..4b61a2f 100644
--- a/js/faradays-law/view/VoltmeterScale.js
+++ b/js/faradays-law/view/VoltmeterScale.js
@@ -86,6 +86,7 @@ define( function( require ) {
       needleArrowNode.rotation = Util.clamp( angle, -Math.PI / 2, Math.PI / 2 );
     } );

+    options.tandem = tandem;
     this.mutate( options );
   }

diff --git a/js/faradays-law/view/VoltmeterWiresNode.js b/js/faradays-law/view/VoltmeterWiresNode.js
index 5cd320d..46de975 100644
--- a/js/faradays-law/view/VoltmeterWiresNode.js
+++ b/js/faradays-law/view/VoltmeterWiresNode.js
@@ -62,7 +62,7 @@ define( function( require ) {
    * @param voltmeterBottom - y coordinate of the bottom of voltmeter
    * @constructor
    */
-  function VoltmeterWiresNode( aligner, voltmeterNode ) {
+  function VoltmeterWiresNode( aligner, voltmeterNode, tandem ) {
     Node.call( this );

     var wireColor = '#353a89';
@@ -101,9 +101,10 @@ define( function( require ) {
       centerY: rightWireBottom
     } ) );

+    this.mutate( { tandem: tandem } );
   }

   faradaysLaw.register( 'VoltmeterWiresNode', VoltmeterWiresNode );
-
+
   return inherit( Node, VoltmeterWiresNode );
 } );

@samreid
Copy link
Member Author

samreid commented Sep 5, 2017

After top and bottomCoilNode are added, then a piece of them are detached and moved to the center. This breaks the object oriented nature of having a CoilNode type. It seems sloppy to me, and I'm not sure what is best.

I have seen a similar pattern in other places where we need to deal with z-ordering. This may be a good one to collaborate on.

@samreid samreid removed their assignment Sep 5, 2017
@samreid
Copy link
Member Author

samreid commented Sep 23, 2017

If we decide to proceed with this, we should create a TType with a method that lets the client hide the node (both parts). But we are on hold until we have design goals for the PhET-iO version in #79

@samreid
Copy link
Member Author

samreid commented Sep 23, 2017

Unassigning until #79 is complete.

@samreid
Copy link
Member Author

samreid commented Nov 20, 2017

We are intentionally not instrumenting all nodes, the reinstrumentation will proceed in #79, closing.

@samreid samreid closed this as completed Nov 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants