How should we instrument the number controls for circuit elements?

[samreid]

For https://github.com/phetsims/circuit-construction-kit-common/issues/513, I set out to make the resistance controls static, so that there would be one resistance control, one capacitance control, etc., and each would switch which component it controls. This would not be trivial to implement since NumberControl, NumberDisplay and Slider do not support mutable attributes such as range, the property (would require bidirectional DynamicProperty), the numer of decimal places in the root, and in the numberDisplayOptions, delta, sliderOptions.constrainValue at the moment.

I made progress using this strategy (one control that switches which element it controls) for one of the 6-7 control types and it seemed OK. However, it revealed the following disadvantages:

• In the data stream, we would need to add separate metadata to indicate which capacitor is being controlled.
• It would be impossible or at least difficult to customized the controls differently. For instance, if we wanted to disable one capacitance control but enable a different capacitance control, we would instead add a listener to the selected component, and then update the singleton capacitance control accordingly. However, I realized that with the current implementation, a control is disposed as soon as the circuit element is deselected, so you wouldn't be able to control it anyways. If we wanted to customized them separately, we wouldn't be able to dispose them when a circuit element is deselected.

I originally thought the "one capacitance control" with a switching target would be more phet-io style. However, these constraints have me thinking maybe we should discuss or investigate the dynamic strategy. The dynamic strategy would match with our existing dynamic pattern for circuit elements and address the bullets above. I'll try it and see what overhead it introduces (above and beyond having to use PhetioGroup).

Since there are many ways to go here, and each of them a lot of work, I think I should wait until we decide on design consideration before spending too much time. Many of the other aspects of instrumentation are going well, so I'll mark this high priority.

UPDATE: This patch explores making the inductance control static, and switching which inductor it controls:

```diff Index: js/view/CircuitElementEditContainerNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/view/CircuitElementEditContainerNode.ts b/js/view/CircuitElementEditContainerNode.ts --- a/js/view/CircuitElementEditContainerNode.ts (revision 80211a2e06667f182d731626f801b2422fde804d) +++ b/js/view/CircuitElementEditContainerNode.ts (date 1638761162667) @@ -8,7 +8,9 @@ */ import Property from '../../../axon/js/Property.js'; +import DynamicProperty from '../../../axon/js/DynamicProperty.js'; import Bounds2 from '../../../dot/js/Bounds2.js'; +import Range from '../../../dot/js/Range.js'; import Utils from '../../../dot/js/Utils.js'; import merge from '../../../phet-core/js/merge.js'; import StringUtils from '../../../phetcommon/js/util/StringUtils.js'; @@ -41,6 +43,7 @@ import ReverseBatteryButton from './ReverseBatteryButton.js'; import SwitchReadoutNode from './SwitchReadoutNode.js'; import TrashButton from './TrashButton.js'; +import NumberProperty from '../../../axon/js/NumberProperty.js'; const capacitanceString = circuitConstructionKitCommonStrings.capacitance; const capacitanceUnitsString = circuitConstructionKitCommonStrings.capacitanceUnits; @@ -92,6 +95,31 @@ const clearDynamicsButton = new ClearDynamicsButton( circuit, tandem.createTandem( 'clearDynamicsButton' ) ); const reverseBatteryButton = new ReverseBatteryButton( circuit, tandem.createTandem( 'reverseBatteryButton' ) ); + const inductancePropertyPlaceholder = new NumberProperty( 0 ); + const currentInductanceProperty = new Property( inductancePropertyPlaceholder ); + const inductanceDynamicProperty = new DynamicProperty( currentInductanceProperty, { + bidirectional: true + } ); + const inductanceControl = new CircuitElementNumberControl( inductanceString, + + // Adapter to take from {{named}} to {{value}} for usage in common code + StringUtils.fillIn( inductanceUnitsString, { + inductance: SunConstants.VALUE_NAMED_PLACEHOLDER + } ), + inductanceDynamicProperty, + new Range( CCKCQueryParameters.inductanceMin, CCKCQueryParameters.inductanceMax ), + circuit, + CCKCQueryParameters.inductorNumberDecimalPlaces, + tandem.createTandem( 'inductanceControl' ), { + delta: CCKCQueryParameters.inductanceStep, + + // For dragging the slider knob + sliderOptions: { + constrainValue: ( value: number ) => Utils.roundToInterval( value, 0.1 ) + } + } + ); + const tapInstructionTextNode = new Text( tapCircuitElementToEditString, { fontSize: 24, maxWidth: 300 @@ -324,25 +352,8 @@ ] ); } else if ( isInductor ) { - const inductanceControl = new CircuitElementNumberControl( inductanceString, + currentInductanceProperty.value = selectedCircuitElement.inductanceProperty; - // Adapter to take from {{named}} to {{value}} for usage in common code - StringUtils.fillIn( inductanceUnitsString, { - inductance: SunConstants.VALUE_NAMED_PLACEHOLDER - } ), - selectedCircuitElement.inductanceProperty, - selectedCircuitElement.inductanceProperty.range!, - circuit, - selectedCircuitElement.numberOfDecimalPlaces, - Tandem.OPT_OUT, { - delta: CCKCQueryParameters.inductanceStep, - - // For dragging the slider knob - sliderOptions: { - constrainValue: ( value: number ) => Utils.roundToInterval( value, 0.1 ) - } - } - ); editNode = new EditPanel( [ clearDynamicsButton, inductanceControl, Index: js/view/CircuitElementNumberControl.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/view/CircuitElementNumberControl.ts b/js/view/CircuitElementNumberControl.ts --- a/js/view/CircuitElementNumberControl.ts (revision 80211a2e06667f182d731626f801b2422fde804d) +++ b/js/view/CircuitElementNumberControl.ts (date 1638761188877) @@ -75,7 +75,7 @@ // This prevents it from ending up in the state. Luckily somehow, it still works properly // in the state wrapper, probably from this code being called anyways from when the circuit element // is selected. - tandem: Tandem.OPTIONAL + tandem: tandem }, providedOptions ) ); // @private {function} - for disposal ```

[samreid]

@arouinfar and I discussed it, and are thinking the cleaner way to implement it is to have one resistance control, one voltage control, etc. This will allow you to listen/customize the entire family without having to wire up to each one individually. If a client wants to customize a control for a specific element, they would have to have a selectedCircuitElement listener and apply customizations when that changes. For the data stream, clients would need to look at the selectedCircuitElement changes to know which (e.g.,) battery is being edited.

@arouinfar says we may also need a way to make it so (e.g.,) a battery can be dragged but not have its voltage edited. Perhaps a new Property associated with the Circuit Element that makes it interactive (can be dragged around) but not editable. Perhaps isEditableProperty.

In part, in order to overcome the lack of mutability, we want to have one control for high voltage batteries, a separate control for regular batteries. Perhaps this will allow us to overcome the problem that ranges, decimals, tick marks are not mutable.

[kathy-phet]

This seems like a pretty big decision - and expensive to change - so shall we gather some feedback from clients before we go down one path or another? I would also like to understand the nuances here too .... happy to discuss before Thursday.

[samreid]

@zepumph indicated that Projectile Motion has different controls for each object type, like baseballNumberControl, tankShellNumberControl, etc. He said whichever is easier to support in the code may be preferable. Currently the code instantiates new controls each time you click on an object. @zepumph said he could join in the upcoming phet-io design meetings for this.

[samreid]

From today's discussion:

@ariel-phet: It seems you should have one control per item, for full flexibility and control. You might want to show the trash button for one circuit element but not another. @samreid: Yes, that sounds the most robust to me. @arouinfar: We could add other flags to the model element, indicating whether it's enabled, etc. @kathy-phet: If we have one control per circuit element, we would nest the control (in the tree) in proximity to the circuit element. If we layer it under, say, batteryNode_0, then we would have isEditable

@zepumph: Generally for projectile motion, we did it that way, where controls were newly created and not reused across targets.

Would eagerly creating the controls for each circuit element make the sim too memory intensive? That's a testable hypothesis. @zepumph: Perhaps we should check this first before letting it determine our path.

@kathy-phet: We should learn from clients what use cases we need to support.

@ariel-phet: It seems likely we will want to set items on the screen to be customizable. Items from the toolbox would be somewhat universal. Will a client want an easy way to show when a circuit element is movable vs non-movable?

@zepumph: It seems every item should be attached to each component. Customization will be time consuming, but it will be fully flexible, powerful and complete.

@arouinfar: It seems we will also need another layer of customization, like isEditable so you can't press "delete" or drop something back in the carousel.

@kathy-phet: It may be better to have a bunch of "is..." properties, so you can control things from the model, and it determines what happens when the edit panels are created. The edit panel respects the settings in the model. @arouinfar: This sounds reasonable to me

@zepumph: Knowing that we want to have one editor per component type, we can design the data stream like we want. A trash button could emit what the selected circuit element was.

[samreid]

From today's meeting, we would like to have one (e.g.,) resistance number control that is statically created and points to the currently selected resistor. Each element in the carousel will have its own number control. These will be statically created.

We will need model-based isEditable properties so that different model elements can be made editable vs non-editable. We will also need model-based isDeletable, which will determine whether it shows the trash button, can be dropped into the toolbox and deleted by the key button. Both isEditable and isDeletable would be true by default. SR: Should we used the term disposable? KP: Delete may be clearer for instructional designers. SR: Should we have a separate property for whether something can go into the carousel and come back out? KP/AR: No, we should combine these. SR: What about flipping the term, like isPersistent? KP: I considered isProtected but all of these sound more vague than isDeletable. SR: I'll start with isDeletable but will reach out to @pixelzoom and @zepumph to see if we should use isDisposable.

• So isEditable determines whether the number control is shown.
• isDeletable determines the various 3 ways of getting rid of an element.
• We need another property to determine whether its value is visible. isValueDisplayable. For all circuit elements, this would determine whether the "show values" checkbox shows anything for that circuit element.

SR: Is it ok that resistorNode_0.colorBandsNode is in the view, but other things are in the model? KP: Let's put isColorCodeVisible in the model, and uninstrument resistorNode_0.colorBandsNode.

There will be one trash button, which is shared across component types.

The visibility of the number control will depend on:

• The circuit element being selected
• The circuit element being editable
• Those will wire up to numberControl's parent, so the client can still set the numberControl itself to be invisible.

There will be only one reverseBattery button (which works for high voltage batteries and regular batteries), and one clearDynamicCircuitElement button. These will require isReversable and isClearable model properties. Also isRepairable. Buttons will be renamed accordingly.

CHECKPOINT JAN 19

For the AC Source: we would have isPhaseEditable, isVoltageEditable, isFrequencyEditable. It is the only type with multiple number controls, so it needs this additional layer of control.

For Vertex nodes, we will need an isCuttable model property.

Add CircuitElement.isMovable. Something should be selectable (so you can edit it), but not movable so it is in a fixed place on the screen. We cannot disable input in this case, because that would make it un-selectable. To accomplish this, we'll need to adjust the graph search that finds the "unmovable" component to see which subset of the graph drags. A vertex adjacent to an isMovable: false circuit element would not be movable. Vertices themselves won't have an isMovable property, but it will just be determined by the neighbors.

NOTE: All of the properties will have the Property suffix.

SR: Should CircuitElement startVertexProperty and endVertexProperty be instrumented? KP: That seems reasonable, if it isn't just needed as part of the state. If you could see it in the tree that would be great. That should be readonly.

We need a way to output the circuit so it can be processed by researchers. The state was broken at the moment, so we didn't get a chance to inspect it.

@kathy-phet says: Good to check in as you go, since we want to get it wrapped up as much as possible in January.

SR: If two vertices are joined, if both were isCuttable then the child can also be isCuttable, but it is unclear what to do if one parent is isCuttable:true and the other is isCuttable:false.

[pixelzoom]

SR: Should we used the term disposable? KP: Delete may be clearer for instructional designers. ... SR: I'll start with isDeletable but will reach out to @pixelzoom and @zepumph to see if we should use isDisposable.

Since we're exposing sim internals, I think it would be wise to consider what's clearer for PhET developers. And that would be isDisposable.

[samreid]

In the commit, I tried a pattern for static creation and reuse of a control for the fuse property, and it seems to be working OK. We can try that pattern for the different circuit elements.

[samreid]

I messaged on slack:

isDisposableProperty is implemented and ready for testing on phettest (please pull first). For instance: circuitConstructionKitDc.labScreen.model.circuit.batteryGroup.battery_0.isDisposableProperty. isEditableProperty is implemented and ready for testing too

[samreid]

On slack, @arouinfar said:

Preliminary review of isDisposableProperty and isEditableProperty went well, but didn’t get to test as thoroughly as I would have liked because I ran into a few bugs, including https://github.com/phetsims/studio/issues/247. I’ll pick up the review again on Tuesday.

[samreid]

In the commits, I've added the following:

• Resistor.isColorCodeVisibleProperty
• isValueDisplayableProperty
• Battery.isReversibleProperty
• DynamicCircuitElement.isClearableProperty
• Fuse.isRepairableProperty

Those parts are ready for review. Coming up next: I'm at https://github.com/phetsims/circuit-construction-kit-common/issues/797#issuecomment-1009311546 CHECKPOINT Jan 19.

[samreid]

I’m working on this request:

Add CircuitElement.isMovable. Something should be selectable (so you can edit it), but not movable so it is in a fixed place on the screen. We cannot disable input in this case, because that would make it un-selectable. To accomplish this, we’ll need to adjust the graph search that finds the “unmovable” component to see which subset of the graph drags. A vertex adjacent to an isMovable: false circuit element would not be movable. Vertices themselves won’t have an isMovable property, but it will just be determined by the neighbors.

This is a nontrivial request due to how the graph search identifies movable elements. However, I found that Vertex already supports a feature called Vertex.isDraggedProperty. I instrumented it and found that it works as a way to make neighboring CircuitElements non-movable. It isn’t the ideal API for clients, but it works out of the box and will not require rewriting any of the graph search functionality for the immovability and dragging. Can we document that as the way you make a CircuitElement non-movable?

UPDATE: In slack, @kathy-phet replied:

I think this proposal sounds great, Sam. I think it can be documented and folks will understand. That is using the vertices to keep things in place

and @arouinfar said:

Yes, agreed with Kathy about the vertex-based approach

[samreid]

I instrumented startVertexProperty and endVertexProperty, but it is using VertexIO which is data-type oriented instead of reference-type oriented. So I'm not sure it's correct. Also, we will need to edit the metadata, such as whether it is readonly.

[samreid]

SR: If two vertices are joined, if both were isCuttable then the child can also be isCuttable, but it is unclear what to do if one parent is isCuttable:true and the other is isCuttable:false.

I reviewed the current implementation and it looks as if the dragged vertex replaces the drop target vertex. That means if the dragged vertex was cuttable, then after it is dragged and attached to something else, it will remain cuttable. The "accepting" vertex is deleted. Is this acceptable for 1.0?

[samreid]

This issue is ready for review and testing. Also, this issue is getting long, so I recommend opening new issues where possible for discovered side problems (unless there is something foundational to this issue). Please note some questions in the preceding comments, and that I have already opened side issues for certain aspects.

[kathy-phet]

SR: If two vertices are joined, if both were isCuttable then the child can also be isCuttable, but it is unclear what to do if one parent is isCuttable:true and the other is isCuttable:false.

I reviewed the current implementation and it looks as if the dragged vertex replaces the drop target vertex. That means if the dragged vertex was cuttable, then after it is dragged and attached to something else, it will remain cuttable. The "accepting" vertex is deleted. Is this acceptable for 1.0?

Does this extend to other properties too? So if I had isMoveable:false on a vertex, and then I move a wire over an attached it, the vertex will now be movable ... because it was replaces?

[samreid]

Yes, the current replacement strategy applies to all Properties.

[kathy-phet]

This seems problematic for several use cases. I do think we should open a new issue for this though - the title here is unrelated. I think the issue can be documented, and noted as existing. If you see a clear pattern for improvement - and its doable - I would recommend it.

My recommendation would be ... Any property that is not the "default" setting should OVERWRITE/TAKE PRECEDENCE. So if there is a label on the existing vertex, it should persist onto the new vertex. Or we should make a hierarchy, e.g. if one vertex is not moveable, that vertex should remain. and so forth.

[samreid]

I moved the vertex-joining proposal to https://github.com/phetsims/circuit-construction-kit-common/issues/833. I think this issue is mainly open for @arouinfar review.

[arouinfar]

@samreid the contents of view.circuitElementEditContainerNode all look good as do the various component-specific properties. I've opened up a few other issues for the remaining work (all referenced above) and have added them to the appropriate milestone. Most of the issues can be deferred until full publication.

[samreid]

As discovered in https://github.com/phetsims/chipper/issues/946, there are TODOs in the code referring to this issue. Reopening.