AC-specific circuit elements and features should not appear in the DC studio tree

[samreid]

AC-specific circuit elements and features should not appear in the DC studio tree

[samreid]

I wrote on slack:

I was going to have the capacitorGroup OPT_OUT of tandem, but tandem is required in PhetioGroup. Should I skip creating the group altogether, or make tandem optional for PhetioGroup?

It's worked well to have AC and DC share as much code as possible, and I don't want to complicate the type hierarchy unless necessary. @zepumph can you please advise?

[samreid]

Also, @arouinfar can you comment what should be excluded from DC? Anything other than capacitors, inductors, AC sources and their corresponding controls?

[zepumph]

I was going to recommend passing an option through to the Class creating these groups, so that AC can tell it to create and instrument these.

and I don't want to complicate the type hierarchy unless necessary.

Does this mean would prefer not to do this?

If you want to make PhetioGroup Tandem.OPTIONAL, go for it.

[samreid]

I was going to recommend passing an option through to the Class creating these groups, so that AC can tell it to create and instrument these.

So the type would be PhetioGroup<Capacitor> | null ? It sounds undesirable to have to guard on capacitorGroup! at many usage sites. I considered creating ACCircuit extends Circuit but what if in the future we have a variant of CCK that has capacitors but not inductors? We have talked about CCK "Intro" and energy-focused CCK variants, so we may need to be able to support arbitrary combinations.

If you want to make PhetioGroup Tandem.OPTIONAL, go for it.

Does that sound like the appropriate solution to you?

[samreid]

The real light bulb is only available on the Lab screen. Do we need to omit the realLightBulbGroup from the intro screen?

I am blocked until we discuss the above topics. Here is my patch so far:

Index: main/circuit-construction-kit-ac/js/lab/LabScreen.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-ac/js/lab/LabScreen.js b/main/circuit-construction-kit-ac/js/lab/LabScreen.js
--- a/main/circuit-construction-kit-ac/js/lab/LabScreen.js	(revision 8e7b0caf170b0daa2e489e8ff72805c98d9682e4)
+++ b/main/circuit-construction-kit-ac/js/lab/LabScreen.js	(date 1643384935388)
@@ -46,7 +46,7 @@
     }, options );
 
     super(
-      () => new CircuitConstructionKitModel( tandem.createTandem( 'model' ) ),
+      () => new CircuitConstructionKitModel( true,tandem.createTandem( 'model' ) ),
       model => new LabScreenView( model, tandem.createTandem( 'view' ), options.labScreenViewOptions ),
       options
     );
Index: main/circuit-construction-kit-ac/js/rlc/RLCScreen.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-ac/js/rlc/RLCScreen.js b/main/circuit-construction-kit-ac/js/rlc/RLCScreen.js
--- a/main/circuit-construction-kit-ac/js/rlc/RLCScreen.js	(revision 8e7b0caf170b0daa2e489e8ff72805c98d9682e4)
+++ b/main/circuit-construction-kit-ac/js/rlc/RLCScreen.js	(date 1643384935386)
@@ -41,7 +41,7 @@
     };
 
     super(
-      () => new CircuitConstructionKitModel( tandem.createTandem( 'model' ) ),
+      () => new CircuitConstructionKitModel( true, tandem.createTandem( 'model' ) ),
       model => new RLCScreenView( model, tandem.createTandem( 'view' ) ),
       options
     );
Index: main/circuit-construction-kit-dc/js/lab/model/LabModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-dc/js/lab/model/LabModel.js b/main/circuit-construction-kit-dc/js/lab/model/LabModel.js
--- a/main/circuit-construction-kit-dc/js/lab/model/LabModel.js	(revision a9354385273a38513bbcd4786fcd04ec355f5c90)
+++ b/main/circuit-construction-kit-dc/js/lab/model/LabModel.js	(date 1643384979034)
@@ -11,11 +11,8 @@
 
 class LabModel extends CircuitConstructionKitModel {
 
-  /**
-   * @param {Tandem}
-   */
   constructor( tandem ) {
-    super( tandem );
+    super( false, tandem );
   }
 }
 
Index: main/circuit-construction-kit-ac/js/ac-voltage/ACVoltageScreen.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-ac/js/ac-voltage/ACVoltageScreen.js b/main/circuit-construction-kit-ac/js/ac-voltage/ACVoltageScreen.js
--- a/main/circuit-construction-kit-ac/js/ac-voltage/ACVoltageScreen.js	(revision 8e7b0caf170b0daa2e489e8ff72805c98d9682e4)
+++ b/main/circuit-construction-kit-ac/js/ac-voltage/ACVoltageScreen.js	(date 1643384935390)
@@ -40,7 +40,7 @@
     };
 
     super(
-      () => new CircuitConstructionKitModel( tandem.createTandem( 'model' ) ),
+      () => new CircuitConstructionKitModel( true,tandem.createTandem( 'model' ) ),
       model => new ACVoltageScreenView( model, tandem.createTandem( 'view' ) ),
       options
     );
Index: main/circuit-construction-kit-common/js/model/Circuit.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-common/js/model/Circuit.ts b/main/circuit-construction-kit-common/js/model/Circuit.ts
--- a/main/circuit-construction-kit-common/js/model/Circuit.ts	(revision 5a8d190e29ddcf20b9fe02bb5df8807c571fab27)
+++ b/main/circuit-construction-kit-common/js/model/Circuit.ts	(date 1643387675760)
@@ -68,7 +68,8 @@
 const trueFunction = _.constant( true ); // Lower cased so IDEA doesn't think it is a constructor
 
 type CircuitOptions = {
-  blackBoxStudy: boolean
+  blackBoxStudy: boolean,
+  includeACElements: boolean
 };
 
 type Pair = { v1: Vertex, v2: Vertex };
@@ -378,7 +379,7 @@
       return new ACVoltage( startVertex, endVertex, this.sourceResistanceProperty, tandem );
     }, () => createVertices( CCKCConstants.AC_VOLTAGE_LENGTH ), {
       phetioType: PhetioGroup.PhetioGroupIO( CircuitElement.CircuitElementIO ),
-      tandem: tandem.createTandem( 'acVoltageGroup' )
+      tandem: options.includeACElements ? tandem.createTandem( 'acVoltageGroup' ) : Tandem.OPT_OUT
     } );
 
     // @public {PhetioGroup}
@@ -428,7 +429,7 @@
       ( tandem, startVertex, endVertex ) => new Capacitor( startVertex, endVertex, tandem ),
       () => createVertices( CCKCConstants.CAPACITOR_LENGTH ), {
         phetioType: PhetioGroup.PhetioGroupIO( CircuitElement.CircuitElementIO ),
-        tandem: tandem.createTandem( 'capacitorGroup' )
+        tandem: options.includeACElements ? tandem.createTandem( 'capacitorGroup' ) : Tandem.OPT_OUT
       } );
 
     // @public {PhetioGroup}
@@ -436,7 +437,7 @@
       ( tandem, startVertex, endVertex ) => new Inductor( startVertex, endVertex, tandem ),
       () => createVertices( CCKCConstants.INDUCTOR_LENGTH ), {
         phetioType: PhetioGroup.PhetioGroupIO( CircuitElement.CircuitElementIO ),
-        tandem: tandem.createTandem( 'inductorGroup' )
+        tandem: options.includeACElements ? tandem.createTandem( 'inductorGroup' ) : Tandem.OPT_OUT
       } );
 
     // @public {PhetioGroup}
Index: main/circuit-construction-kit-dc/js/intro/model/IntroModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-dc/js/intro/model/IntroModel.js b/main/circuit-construction-kit-dc/js/intro/model/IntroModel.js
--- a/main/circuit-construction-kit-dc/js/intro/model/IntroModel.js	(revision a9354385273a38513bbcd4786fcd04ec355f5c90)
+++ b/main/circuit-construction-kit-dc/js/intro/model/IntroModel.js	(date 1643384979031)
@@ -16,7 +16,7 @@
    * @constructor
    */
   constructor( tandem ) {
-    super( tandem );
+    super( false, tandem );
   }
 }
 
Index: main/circuit-construction-kit-black-box-study/js/blackbox/model/BlackBoxSceneModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-black-box-study/js/blackbox/model/BlackBoxSceneModel.js b/main/circuit-construction-kit-black-box-study/js/blackbox/model/BlackBoxSceneModel.js
--- a/main/circuit-construction-kit-black-box-study/js/blackbox/model/BlackBoxSceneModel.js	(revision 758895f2931431e38d6bbca3f3a09c463a503f95)
+++ b/main/circuit-construction-kit-black-box-study/js/blackbox/model/BlackBoxSceneModel.js	(date 1643384979029)
@@ -27,7 +27,7 @@
    * @param {Tandem} tandem
    */
   constructor( trueBlackBoxCircuitObject, tandem ) {
-    super( tandem, {
+    super( true, tandem, {
       revealing: false,
       blackBoxStudy: true
     } );
Index: main/circuit-construction-kit-common/js/model/CircuitConstructionKitModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-common/js/model/CircuitConstructionKitModel.ts b/main/circuit-construction-kit-common/js/model/CircuitConstructionKitModel.ts
--- a/main/circuit-construction-kit-common/js/model/CircuitConstructionKitModel.ts	(revision 5a8d190e29ddcf20b9fe02bb5df8807c571fab27)
+++ b/main/circuit-construction-kit-common/js/model/CircuitConstructionKitModel.ts	(date 1643387593394)
@@ -57,11 +57,7 @@
   readonly stepEmitter: Emitter<[ number ]>;
   private readonly zoomProperty: Property<ZoomLevel>;
 
-  /**
-   * @param {Tandem} tandem
-   * @param {Object} [providedOptions]
-   */
-  constructor( tandem: Tandem, providedOptions?: Partial<CircuitConstructionKitModelOptions> ) {
+  constructor( includeACElements: boolean, tandem: Tandem, providedOptions?: Partial<CircuitConstructionKitModelOptions> ) {
 
     const options = merge( {
 
Index: main/circuit-construction-kit-black-box-study/js/explore/model/ExploreModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-black-box-study/js/explore/model/ExploreModel.js b/main/circuit-construction-kit-black-box-study/js/explore/model/ExploreModel.js
--- a/main/circuit-construction-kit-black-box-study/js/explore/model/ExploreModel.js	(revision 758895f2931431e38d6bbca3f3a09c463a503f95)
+++ b/main/circuit-construction-kit-black-box-study/js/explore/model/ExploreModel.js	(date 1643384979033)
@@ -11,7 +11,7 @@
 
 class ExploreModel extends CircuitConstructionKitModel {
   constructor( tandem ) {
-    super( tandem, { blackBoxStudy: true } );
+    super( true, tandem, { blackBoxStudy: true } );
   }
 }
 

[zepumph]

It does not bother me to have PhetioGroup tandem optional. This of course means making if Tandem.OPTIONAL in PhetioDynamicElementContainer. Will you just document the reason to avoid future confusion, something like, "Many PhET-iO instrumented types live in common code used by multiple sims, and may only be instrumented in a subset of their usages."

[samreid]

The real light bulb is only available on the Lab screen. Do we need to omit the realLightBulbGroup from the intro screen?

@arouinfar and I discussed it, the realLightBulb shouldn't appear in the tree in screen 1. Same with the high voltage battery and high resistance resistor and bulb, they shouldn't be in the intro screen. The "add real bulbs" button shouldn't be in screen 1 either.

Anything other than capacitors, inductors, AC sources and their corresponding controls?

@arouinfar indicated the charts should be excluded from DC.

[samreid]

@zepumph can you please review phetsims/tandem@3750a57 ? It was failing with an assertion error before that commit.

[arouinfar]

@samreid here are the irrelevant elements that I'm currently seeing in the tree (working copy last pulled ~45 min ago):

AC-Specific elements:

• circuitConstructionKitDc.*Screen.model.isPlayingProperty
• circuitConstructionKitDc.*Screen.model.stopwatch
• circuitConstructionKitDc.*Screen.view.circuitElementEditContainerNode.frequencyControl
• circuitConstructionKitDc.*Screen.view.circuitElementEditContainerNode.phaseShiftControl

Lab Screen-specific elements:

• circuitConstructionKitDc.introScreen.model.addRealBulbsProperty
• circuitConstructionKitDc.introScreen.model.circuit.highResistanceLightBulbGroup
• circuitConstructionKitDc.introScreen.model.circuit.highVoltageBatteryGroup
• circuitConstructionKitDc.introScreen.model.circuit.realLightBulbGroup
• circuitConstructionKitDc.introScreen.model.circuit.seriesAmmeterGroup
• circuitConstructionKitDc.introScreen.model.sourceResistanceProperty
• circuitConstructionKitDc.introScreen.model.wireResistivityProperty
• circuitConstructionKitDc.introScreen.view.circuitElementEditContainerNode.highResistanceNumberControl
• circuitConstructionKitDc.introScreen.view.circuitElementEditContainerNode.highVoltageNumberControl
• circuitConstructionKitDc.introScreen.view.circuitElementEditContainerNode.lightBulbHighResistanceNumberControl
• circuitConstructionKitDc.introScreen.view.circuitNode.highVoltageBatteryNodeGroup
• circuitConstructionKitDc.introScreen.view.circuitNode.seriesAmmeterNodeGroup

[zepumph]

Looks great! Thanks.

[arouinfar]

@samreid I verified the list in #824 (comment) but found a few more Lab-only elements to remove from the Intro Screen:

• introScreen.view.circuitNode.highResistanceLightBulbNodeGroup
• introScreen.view.sensorToolbox.seriesAmmeterNodeIcon
• introScreen.view.sensorToolbox.seriesAmmeterToolNode.visibleProperty

I also realized that these properties still provide important information on the Intro Screen and I should not have asked you to remove them. Please return these to the Intro Screen:

• model.sourceResistanceProperty
• model.wireResistivityProperty

[samreid]

I adjusted the five requests in the commit. @arouinfar can you please review/test?

[arouinfar]

Looks good, thanks @samreid

[Nancy-Salpepi]

For phetsims/qa#772

The clear button (view.circuitElementEditContainerNode.clearButton) should be removed from the studio tree (Confirmed with @arouinfar that this button is only for inductors/capacitors which aren't present in DC).

[arouinfar]

This is something that should be cleaned up for publication, but I don't think it will have any ill effects on the client, so I switched the milestone.

[samreid]

Today we discussed reverting the "lab" specific features, so those are more unified. I can revert those commits. This is based on part in the discussion in phetsims/geometric-optics-basics#3

[samreid]

Just wanting to clarify the convention before I start on this issue. In Circuit Construction Kit: DC, the Intro screen should support natural features from the Lab screen? But none of the features from AC?

[arouinfar]

In Circuit Construction Kit: DC, the Intro screen should support natural features from the Lab screen? But none of the features from AC?

Yes, that seems reasonable @samreid. My takeaway from phetsims/geometric-optics-basics#3 is that we do not need to aggressively prune the tree to remove features that could safely be turned on in other screens/flavors of a sim, especially when the differences are largely subtractive in nature. In the future, derivative simulations could potentially be created by clever use of visibleProperty, much like CM did when creating Geometric Optics: Basics.

That said, we should not add AC-only features to DC (AC sources, capacitors, inductors, charting tools) and similarly, we shouldn't include DC-only components (high-voltage and high-resistance components) in AC because they have not been tested in an AC environment and go beyond the scope of the learning goals.

I think it's fine for Lab screen components to make an appearance in the tree of the Intro screen, so long as the feature is fully functional. I see no harm in PhET-iO clients adding series ammeters or high-V batteries to the Intro screen, for example.

[matthew-blackman]

The clear button no longer appears in studio in DC mode. @samreid and I confirmed that DC studio does not show any inductors or capacitors.

[matthew-blackman]

Cross-linking issue #900 in separate milestone for AC release