phetsims / circuit-construction-kit-common

"Circuit Construction Kit: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
10 stars 10 forks source link

Remove instances of over-instrumentation #855

Closed arouinfar closed 1 year ago

arouinfar commented 2 years ago

@samreid I found a few cases of over-instrumentation or circuit elements with irrelevant properties instrumented. Please uninstrument the following:

samreid commented 2 years ago

I opted out of the tandems checked off above. For the others, I wanted to ask:

arouinfar commented 1 year ago

Is uninstrumentation of the archetypes permanently, or only until we implement https://github.com/phetsims/phet-io/issues/1856 ?

These requests all look permanent to me. Wires are not editable and have no value or label to display, so I see no reason for them to have isEditableProperty, isValueDisplayableProperty, or labelStringProperty. These properties do not translate to the series ammeter, either.

Do we really want to uninstrument the labels?

The "tiny" and "lots" labels do not need to be instrumented in the view. The strings will still be accessible under general.model.strings but we do not need to bring any attention to them in the view.

The thumb node and track node should remain instrumented for the data stream events, right?

This was a mistake on my part. The thumbNode and trackNode are normal children of a slider and exist throughout PhET-iO sims. I'll cross those off the list above. Thanks for checking first!

samreid commented 1 year ago

@matthew-blackman and I looked into this, and we additionally saw tandem.createTandem( 'tinyLabelText' ) in SourceResistanceControls. (Whatever we decide should apply equally to SourceResistanceControls and WireResistivityControls). But we were skeptical about uninstrumenting these tick mark label scenery nodes, thinking that would make it impossible to use studio autoselect to discover them. So we wanted to check about that before proceeding. @arouinfar what do you recommend?

arouinfar commented 1 year ago

But we were skeptical about uninstrumenting these tick mark label scenery nodes, thinking that would make it impossible to use studio autoselect to discover them.

I think there's a bit of a miscommunication here. Before the move to dynamic strings, the Wire Resistivity slider had children for the tiny and lots labels. Those no longer exist in the tree, so I don't think there's anything that needs to be done here. The select feature also seems to be working just fine in master.

image

I crossed the items off the list in the original comment, so we can go ahead and close.