Closed arouinfar closed 1 year ago
I committed a draft of the phetioFeatured overrides. I think it's good enough for the client preview, but it's still rough.
phetioReadOnly: true
.I will open up separate issues for items 2-4.
Finalizing the overrides should wait until the design is complete.
Note: @arouinfar and I will revisit the view.circuitElementToolbox components after the carousel-dynamic branch has been merged back into master.
@arouinfar and I reviewed the featured overrides of all introScreen.model and labScreen.model components, and through the introScreen.view.displayOptionsPanel components. We skipped over view.circuitElementToolbox and still need to pick up where we left off with introScreen.view.meterNodes.
@arouinfar and I completed the overrides, with the exception of the following:
Once everything is wrapped up for CCK: DC, we should propagate the overrides to circuit-construction-kit-dc-virtual-lab. We can copy over the overrides from circuit-construction-kit-dc, but with these changes:
introScreen
phetioID
with circuitConstructionKitDcVirtualLab
.Once the overrides are defined for Circuit Construction Kit: DC, I think it would be nice to propagate them out of the overrides file into the code. That will reduce overhead and hassles described in https://github.com/phetsims/phet-io/issues/1873, and will also make it so we don't need to copy paste overrides for dc-virtual lab, or AC or AC virtual lab.
I propagated the overrides to the code and it went well. Here is a patch for that:
This reduces the total number of lines of code from 19635 to 19003, a reduction of 632 lines of code = 3.2% of the code. This change also automatically handled the extremeResistorGroup
and householdObjectGroup
, for example.
However, I did not yet commit since this change introduced variations that deviate from the original API file, and it is difficult for me to tell whether this (a) correctly unified unintentional variations or (b) introduced undesirable problems. For other cases, it was unclear whether a new phetioFeatured: true
I introduced for simplicity would be acceptable, like for the circuitElements
. So I think it would be best to review with @arouinfar and @matthew-blackman. I would also note that using https://benjamine.github.io/jsondiffpatch/demo/index.html, and pasting in the old API and new API made it very simple to see what had changed. For example, it looked like this:
UPDATE: I'm also carefully tracking my time spent on this, in case that can inform value in using this strategy in other sims. So far, I'm up to 46 minutes (includes the time to write this issue comment). Also, for future ones, I'm wondering if we can save even more time by collaboratively writing the phetioFeatured metadata. As it is now, there is cost to create the whole overrides file (with a lot of redundancy), then I have to deal with the redundancy when I propagate it back to the code (in keeping track of which things have been moved). While this may work well for asynchronous collaboration, perhaps synchronous development of this part may be more efficient overall.
@matthew-blackman and @arouinfar and I discussed this, and agree we want to move forward with the overrides in the code.
@matthew-blackman asks: What if one screen wants to have something featured, and another screen wants it non-featured? @samreid says: There are 3 lines of defense:
@arouinfar also pointed out that it is a time-consuming process to specify the overrides in the first place, where things have to be propagated to other screens/sims, and there is a lot of duplication in the values. We could save time on this aspect as well.
@matthew-blackman wants to specify criteria for deciding level 2 vs level 3 above. One idea is: if something is screen specific (featured on one screen but not the other), put it in the overrides. @samreid: I would like to see all the overrides files disappear. But ultimately may be developer discretion.
@matthew-blackman also said we will have to redefine the workflow for this process. @samreid: Probably a working design meeting where the developer adds metadata as we go. @arouinfar: We should ask @pixelzoom what part of the existing process he wants to keep. If it is the decision-making, then the designer can save time by reporting like "feature the currentProperty of all circuit elements" instead of having to select that 16 times throughout the sim.
@matthew-blackman asks why we do this for phetioFeatured
but not for other metadata, such as phetioReadOnly
and phetioDocumentation
.
@samreid and @arouinfar weren't sure about this exactly, but I bet there is a paper trail.
@arouinfar says: It is important for things like phetioReadOnly
to be in the code at the declaration site, so it is clear to developers.
@matthew-blackman says it seems straightforward to treat phetioFeatured
as we treat other metadata.
We will schedule a >=15 minute meeting to review the differences
After this is done, @arouinfar will review the featured tree asynchronously.
Regarding https://github.com/phetsims/circuit-construction-kit-common/issues/934, @samreid @arouinfar and I agree that the sensor connection axon properties should be featured so that a client can attach logic to the usage of the specific sensors.
@matthew-blackman @samreid I took a another pass through the tree and found one issue related to the phetioFeatured
elements. Otherwise, things are looking good!
visibleProperty
of the icons in the sensor toolboxes should be featured, not the ToolNode itself. For example: view.sensorToolbox.noncontactAmmeterToolNode
is currently featured, but its visibleProperty
is actually what we want to feature. This is true for all sensor icons.We should also do a pass over sensor instrumentation after https://github.com/phetsims/circuit-construction-kit-common/issues/934 is closed.
I fixed the request from https://github.com/phetsims/circuit-construction-kit-common/issues/836#issuecomment-1421603219
We should also do a pass over sensor instrumentation after https://github.com/phetsims/circuit-construction-kit-common/issues/934 is closed.
Also, I realized it is awkward you cannot autoselect a red/black probe itself. Should we add that?
In discussion, I mentioned that it could take 30-60 minutes to instrument blackProbeNode and link it to a new model element blackProbe. We discussed and agreed the current behavior is OK. Autoselect on a probe takes you to the whole voltmeter node or voltmeter, and we agree that is nice and ok.
@samreid @matthew-blackman I verified the change in https://github.com/phetsims/circuit-construction-kit-common/issues/836#issuecomment-1421603219 and those related to #934. The only thing missing that should be featured is model.meters.ammeter*.probeConnectionProperty
.
Once that's addressed, we should be good to close this issue.
OK I featured that in the commits, closing.
Self-assigning to take care of the phetioFeatured overrides.
@samreid I added this to the client preview milestone, but it can be done concurrently with QA testing.