Non-contact ammeters are poorly supported in virtual-lab

[arouinfar]

Tangentially related to https://github.com/phetsims/qa/issues/900 and discovered while reviewing https://github.com/phetsims/circuit-construction-kit-common/issues/865.

CCK: DC - Virtual Lab is nearly identical to its parent sim. There are two primary differences:

1. It's a single-screen sim, consisting of just the Lab screen (screens=2)
2. Non-contact ammeters are excluded.

Non-contact ammeters are still visible in the Studio Tree and can be found under:

• circuitConstructionKitDcVirtualLab.labScreen.model.meters
• circuitConstructionKitDcVirtualLab.labScreen.view.meterNodes
• circuitConstructionKitDcVirtualLab.labScreen.view.sensorToolbox

Clients can use isActiveProperty to add the non-contact ammeter to the play area of the sim. However, it is not possible to make them visible in the toolbox. view.sensorToolbox.noncontactAmmeterToolNode.visibleProperty is true by default, but no icon appears in the toolbox. I assumed that the default would be false and clients could set it to true to give students access to non-contact ammeters.

I see two options

1. Fix view.sensorToolbox.noncontactAmmeterToolNode.visibleProperty so it has the expected behavior -- default false, but clients can set it to true to add non-contact ammeters to the toolbox. This would be consistent with how we generally handle the PhET-iO instrumentation of derivative sims.
2. Completely uninstrument everything related to the non-contact ammeters in virtual-lab. If clients want to use non-contact ammeters, they can use cck-dc with screens=2.

I am fine with either option, so I'll leave it up to @samreid and @matthew-blackman to decide.

[matthew-blackman]

@samreid @zepumph @arouinfar and I discussed this, and agree that all non-contact ammeter tandems should be removed from studio for CCK DC-Virtual Lab. We plan to uninstrument this so that users cannot encounter any possibly buggy behavior by adding these sensors in the Virtual Lab version of the sim.

If clients want to show non-contact ammeters in the Lab screen, they can use the standard version of the sim with screens=2.

[matthew-blackman]

This is ready for review. @arouinfar please confirm that all noncontact ammeter tandems are showing correctly in both sims and all screens.

[samreid]

For the code review, specifying showNoncontactAmmeters: true, js/intro/view/IntroScreenView.ts could lead to trouble since it allows a model to create noncontact ammeters but the view to not show them. So I think the solution for that part would be to assign it to the model and use something like model.isShowNoncontactAmmeters.

For this code: could we skip creating them altogether if showNoncontactAmmeters is false? Or does that cause problems?

    this.ammeters = [
      new Ammeter( providedOptions?.showNoncontactAmmeters ? metersTandem.createTandem( 'ammeter1' ) : Tandem.OPT_OUT, 1 ),
      new Ammeter( providedOptions?.showNoncontactAmmeters ? metersTandem.createTandem( 'ammeter2' ) : Tandem.OPT_OUT, 2 )
    ];

Everything else in the code review looks great. Reassigning to @matthew-blackman for the code review issues. Leaving assigned to @arouinfar to review the runtime behavior and API.

[matthew-blackman]

@samreid and I worked on a way to accomplish the second recommendation from the comment above, but ran into complexity creep with SensorToolbox. Pasting the patch that we worked on below (almost in working order but becoming overly complex) in case we ever want to pursue this approach in the future.

```diff Subject: [PATCH] Ugly patch --- Index: circuit-construction-kit-dc/js/lab/LabScreen.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/circuit-construction-kit-dc/js/lab/LabScreen.ts b/circuit-construction-kit-dc/js/lab/LabScreen.ts --- a/circuit-construction-kit-dc/js/lab/LabScreen.ts (revision b1e346952f64de297d9adc6d4eab3cf62394eb8d) +++ b/circuit-construction-kit-dc/js/lab/LabScreen.ts (date 1677075206098) @@ -31,7 +31,6 @@ super( () => new LabModel( tandem.createTandem( 'model' ), providedOptions ), model => new LabScreenView( model, tandem.createTandem( 'view' ), { - showNoncontactAmmeters: providedOptions.showNoncontactAmmeters, circuitElementToolboxOptions: { carouselScale: CCKCConstants.DC_CAROUSEL_SCALE } } ), { name: labStringProperty, Index: circuit-construction-kit-common/js/view/SensorToolbox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/circuit-construction-kit-common/js/view/SensorToolbox.ts b/circuit-construction-kit-common/js/view/SensorToolbox.ts --- a/circuit-construction-kit-common/js/view/SensorToolbox.ts (revision 7ebaa778f96c65b98cb3873ac2863c5b1f8dc856) +++ b/circuit-construction-kit-common/js/view/SensorToolbox.ts (date 1677080711479) @@ -117,28 +117,39 @@ } ); voltmeterToolIcon.addInputListener( createListenerMulti( voltmeterNodes ) ); - // Icon for the ammeter - const ammeter = new Ammeter( Tandem.OPTIONAL, 0 ); - const ammeterToolIcon = new AmmeterNode( ammeter, null, { - isIcon: true, - tandem: options.showNoncontactAmmeters ? tandem.createTandem( 'noncontactAmmeterToolNode' ) : Tandem.OPT_OUT, - phetioVisiblePropertyInstrumented: true, - visiblePropertyOptions: { - phetioFeatured: true - } - } ); - const allAmmetersInPlayAreaProperty = DerivedProperty.and( ammeterNodes.map( ammeterNode => ammeterNode.ammeter.isActiveProperty ) ); - allAmmetersInPlayAreaProperty.link( allInPlayArea => { + const ammeterToolNodeChildren = []; + + let ammeterToolIconVisibleProperty: ReadOnlyProperty | null = null; + let allAmmetersInPlayAreaProperty: ReadOnlyProperty | null = null; + + if ( options.showNoncontactAmmeters ) { + // Icon for the ammeter + const ammeter = new Ammeter( Tandem.OPTIONAL, 0 ); + const ammeterToolIcon = new AmmeterNode( ammeter, null, { + isIcon: true, + tandem: options.showNoncontactAmmeters ? tandem.createTandem( 'noncontactAmmeterToolNode' ) : Tandem.OPT_OUT, + phetioVisiblePropertyInstrumented: true, + visiblePropertyOptions: { + phetioFeatured: true + } + } ); + allAmmetersInPlayAreaProperty = DerivedProperty.and( ammeterNodes.map( ammeterNode => ammeterNode.ammeter.isActiveProperty ) ); + allAmmetersInPlayAreaProperty.link( allInPlayArea => { - // Simulate becoming visible: false, but without changing the dimensions - ammeterToolIcon.setOpacity( allInPlayArea ? 0 : 1 ); - ammeterToolIcon.setInputEnabled( !allInPlayArea ); - } ); - ammeterToolIcon.mutate( { - scale: TOOLBOX_ICON_HEIGHT / Math.max( ammeterToolIcon.width, ammeterToolIcon.height ) - } ); - ammeterToolIcon.addInputListener( createListenerMulti( ammeterNodes ) ); + // Simulate becoming visible: false, but without changing the dimensions + ammeterToolIcon.setOpacity( allInPlayArea ? 0 : 1 ); + ammeterToolIcon.setInputEnabled( !allInPlayArea ); + } ); + ammeterToolIcon.mutate( { + scale: TOOLBOX_ICON_HEIGHT / Math.max( ammeterToolIcon.width, ammeterToolIcon.height ) + } ); + ammeterToolIcon.addInputListener( createListenerMulti( ammeterNodes ) ); + ammeterToolIconVisibleProperty = ammeterToolIcon.visibleProperty; + + ammeterToolNodeChildren.push( ammeterToolIcon ); + } + // Icon for the series ammeter const seriesAmmeterIcon = new SeriesAmmeter( new Vertex( Vector2.ZERO, circuit.selectionProperty ), @@ -202,12 +213,20 @@ } } ); - Multilink.multilink( - [ circuitNode.model.showLabelsProperty, allAmmetersInPlayAreaProperty, allSeriesAmmetersInPlayAreaProperty, ammeterToolIcon.visibleProperty, seriesAmmeterNodeIcon.visibleProperty, seriesAmmeterToolNode.visibleProperty ], - ( showLabels, allAmmetersInPlayArea, allSeriesAmmetersInPlayArea, ammeterToolNodeVisible, seriesAmmeterNodeIconVisible, seriesAmmeterToolNodeVisible ) => { - - ammeterText.visible = showLabels && ( seriesAmmeterToolNodeVisible || ammeterToolNodeVisible ); - } ); + if ( options.showNoncontactAmmeters ) { + Multilink.multilinkAny( + [ circuitNode.model.showLabelsProperty, allSeriesAmmetersInPlayAreaProperty, ammeterToolIconVisibleProperty, seriesAmmeterNodeIcon.visibleProperty, seriesAmmeterToolNode.visibleProperty ], + ( showLabels, allSeriesAmmetersInPlayArea, ammeterToolNodeVisible, seriesAmmeterNodeIconVisible, seriesAmmeterToolNodeVisible ) => { + ammeterText.visible = showLabels && ( seriesAmmeterToolNodeVisible || ammeterToolNodeVisible ); + } ); + } + else { + Multilink.multilinkAny( + [ circuitNode.model.showLabelsProperty, allSeriesAmmetersInPlayAreaProperty, ammeterToolIconVisibleProperty, seriesAmmeterNodeIcon.visibleProperty, seriesAmmeterToolNode.visibleProperty ], + ( showLabels, allSeriesAmmetersInPlayArea, ammeterToolNodeVisible, seriesAmmeterNodeIconVisible, seriesAmmeterToolNodeVisible ) => { + ammeterText.visible = showLabels && ( seriesAmmeterToolNodeVisible || ammeterToolNodeVisible ); + } ); + } const voltmeterToolNode = new VBox( { tandem: tandem.createTandem( 'voltmeterToolNode' ), @@ -236,9 +255,7 @@ ] } ); - const children = []; - options.showNoncontactAmmeters && children.push( ammeterToolIcon ); - options.showSeriesAmmeters && children.push( seriesAmmeterToolNodeContainer ); + options.showSeriesAmmeters && ammeterToolNodeChildren.push( seriesAmmeterToolNodeContainer ); const ammeterToolNode = new VBox( { tandem: Tandem.OPT_OUT, @@ -247,7 +264,7 @@ new HBox( { spacing: 8, align: 'bottom', - children: children, + children: ammeterToolNodeChildren, excludeInvisibleChildrenFromBounds: true } ), ammeterText Index: circuit-construction-kit-common/js/view/CCKCScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/circuit-construction-kit-common/js/view/CCKCScreenView.ts b/circuit-construction-kit-common/js/view/CCKCScreenView.ts --- a/circuit-construction-kit-common/js/view/CCKCScreenView.ts (revision 7ebaa778f96c65b98cb3873ac2863c5b1f8dc856) +++ b/circuit-construction-kit-common/js/view/CCKCScreenView.ts (date 1677074071740) @@ -76,7 +76,6 @@ circuitElementToolboxOptions: CircuitElementToolboxOptions; showSeriesAmmeters?: boolean; showTimeControls?: boolean; - showNoncontactAmmeters?: boolean; showAdvancedControls?: boolean; showCharts?: boolean; blackBoxStudy?: boolean; @@ -121,7 +120,6 @@ showSeriesAmmeters: false, showTimeControls: false, - showNoncontactAmmeters: true, showAdvancedControls: true, showCharts: false, blackBoxStudy: false, @@ -164,7 +162,7 @@ const ammeterNodes = model.ammeters.map( ammeter => { const ammeterNode = new AmmeterNode( ammeter, this.circuitNode, { - tandem: options.showNoncontactAmmeters ? meterNodesTandem.createTandem( `ammeterNode${ammeter.phetioIndex}` ) : Tandem.OPT_OUT, + tandem: meterNodesTandem.createTandem( `ammeterNode${ammeter.phetioIndex}` ), showResultsProperty: model.isValueDepictionEnabledProperty, visibleBoundsProperty: this.circuitNode.visibleBoundsInCircuitCoordinateFrameProperty, blackBoxStudy: options.blackBoxStudy, @@ -237,7 +235,7 @@ [ this.currentChartNode1!, this.currentChartNode2! ], tandem.createTandem( 'sensorToolbox' ), { showSeriesAmmeters: options.showSeriesAmmeters, - showNoncontactAmmeters: options.showNoncontactAmmeters, + showNoncontactAmmeters: model.isShowNoncontactAmmeters, showCharts: options.showCharts, visiblePropertyOptions: { phetioFeatured: true Index: circuit-construction-kit-dc/js/intro/view/IntroScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/circuit-construction-kit-dc/js/intro/view/IntroScreenView.ts b/circuit-construction-kit-dc/js/intro/view/IntroScreenView.ts --- a/circuit-construction-kit-dc/js/intro/view/IntroScreenView.ts (revision b1e346952f64de297d9adc6d4eab3cf62394eb8d) +++ b/circuit-construction-kit-dc/js/intro/view/IntroScreenView.ts (date 1677070915784) @@ -48,7 +48,6 @@ super( model, circuitElementToolItems, tandem, { showAdvancedControls: false, - showNoncontactAmmeters: true, circuitElementToolboxOptions: { carouselScale: CCKCConstants.DC_CAROUSEL_SCALE } } ); } Index: circuit-construction-kit-common/js/model/CircuitConstructionKitModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/circuit-construction-kit-common/js/model/CircuitConstructionKitModel.ts b/circuit-construction-kit-common/js/model/CircuitConstructionKitModel.ts --- a/circuit-construction-kit-common/js/model/CircuitConstructionKitModel.ts (revision 7ebaa778f96c65b98cb3873ac2863c5b1f8dc856) +++ b/circuit-construction-kit-common/js/model/CircuitConstructionKitModel.ts (date 1677074717844) @@ -47,6 +47,9 @@ // whether the carousel shows real bulbs public readonly addRealBulbsProperty: BooleanProperty; + // whether the model has noncontact ammeters + public isShowNoncontactAmmeters: boolean; + // contains CircuitElements, Vertices, etc. public readonly circuit: Circuit; @@ -114,6 +117,8 @@ phetioFeatured: true } ); + this.isShowNoncontactAmmeters = options.showNoncontactAmmeters; + const circuitTandem = tandem.createTandem( 'circuit' ); this.circuit = new Circuit( this.viewTypeProperty, this.addRealBulbsProperty, circuitTandem, { blackBoxStudy: options.blackBoxStudy, @@ -127,10 +132,10 @@ new Voltmeter( metersTandem.createTandem( 'voltmeter2' ), 2 ) ]; - this.ammeters = [ - new Ammeter( providedOptions?.showNoncontactAmmeters ? metersTandem.createTandem( 'ammeter1' ) : Tandem.OPT_OUT, 1 ), - new Ammeter( providedOptions?.showNoncontactAmmeters ? metersTandem.createTandem( 'ammeter2' ) : Tandem.OPT_OUT, 2 ) - ]; + this.ammeters = this.isShowNoncontactAmmeters ? [ + new Ammeter( metersTandem.createTandem( 'ammeter1' ), 1 ), + new Ammeter( metersTandem.createTandem( 'ammeter2' ), 2 ) + ] : []; [ ...this.voltmeters, ...this.ammeters ].forEach( meter => { meter.isActiveProperty.link( isActive => { ```

[matthew-blackman]

@samreid I think this warrants a final review before closing, given the issues we explored in the patch above.

[arouinfar]

I no longer see any reference to the non-contact ammeters in the cck-dc-virtual-lab tree. So from a design perspective, everything looks good.

[matthew-blackman]

@samreid and I reviewed the most recent commit and agree that this issue can be closed.