Open chrisklus opened 5 years ago
Whoops!
@chrisklus is going to implement with basic tandems and then request a design meeting (current target sept 12 design meeting)
@ariel-phet when I said two weeks during our discussion I was intending for 09/05/19 design meeting, if that's a possibility. A dev test has been opened, so if nothing crazy comes from that then I'll be able to do initial tandem passing before Thursday next week.
[x] Create a "PhET-iO Instrumentation" issue in the simulation repository. Copy this checklist to the issue description (top issue comment) for tracking. Link back to this checklist via /blob/<SHA>/
so that the specific guide you used is preserved.
[x] Publish a pre-instrumentation dev release. This can be useful for identifying whether bugs or memory issues have been introduced during instrumentation, or were pre-existing. This also creates a benchmark to reference against for memory-leaks, sim size, performance, etc. Document the dev release in the sim's phet-io github issue.
pre-instrumentation dev version: https://phet-dev.colorado.edu/html/energy-forms-and-changes/1.2.0-dev.2/phet/energy-forms-and-changes_en_phet.html
[x] Understand the goal. Read through The PhET-iO Website Dev Guide including the link to Graphing Quadratics 1.1. Visit all of the linked wrappers and docs. Test each wrapper, investigate, report bugs, ask questions!
[x] Understand the complete PhET-iO feature set and api. In general it can be thought of as 4 items:
PhetioObject
instantiated in the sim along with its associated TypeIO. For example, SCENERY/Node
extends PhetioObject
and its default TypeIO is NodeIO
.[ ] Schedule a PhET-iO design meeting for the simulation to identify what should be customizable/interoperable/data stream and track it in an issue. For example, see how-to-design-phet-io-features-for-a-simulation.md. Think about how a researcher or 3rd party may wish to configure the simulation or collect data from it, and make sure that is supported by the instrumentation. For example, some simulations will need custom higher-level events (such as whether the user created a parallel circuit), for events that are useful, easy to compute in simulation code and difficult to compute in wrapper code. Or a simulation may need to be configurable in a way that is not already supported by the instrumentation you have already completed. These features should be determined in the PhET-iO design meeting.
[x] Typically it is best if the responsible developer for the simulation is available to perform the PhET-iO instrumentation. They have important insight into the structure, history, trade-offs and other important details of the simulation implementation that will facilitate the instrumentation. If the responsible developer is not available for instrumentation, it would be nice if they are available for consultation or support during instrumentation.
A high-quality code review will make instrumentation easier, promote long term maintainability for the simulation, and protect the simulation from a volatile API. If the simulation is already in good shape, the review will not take too long. If the simulation is not in good shape, then it needs your help.
Now that the simulation is in good shape and the PhET-iO design meeting is complete, we are ready to instrument the simulation. Follow the checklist below, and if you have questions you can review Faraday's Law or Graphing Quadratics and their PhET-iO instrumentations, or reach out to teammates who may have come this way before.
grunt update
in the sim repo. This will generate the needed phet-io api preload files to run in phet-io brand. Note that while validating the api, these preloads will need to be kept in sync with the current state of the repo. This may be more trouble than it's worth while iterating. You can use ?phetioValidateAPI=false
to turn off this validation.tandem
s to each screen using tandem.createTandem(...)
?phetioValidateTandems
query string to temporarily get past "Tandem was required but not supplied" issues.Consult the PhET-iO design issue to see what features the sim should support. See
PhetioObject.js for the
supported PhetioObject options. Not every node in the hierarchy must be instrumented, but every leaf is instrumented. For example the view
is rarely instrumented.
tandem
s and other PhetioObject
options into objects that should be instrumented. Do not instrument objects that are "implementation details" and do not over-instrument. The goal is a to design an API that balances the power of a broad feature set while still being maintainable.units
in NumberProperty
for example, and should be passed where appropriate.)Well-designed tandem names are important. Once the PhET-iO simulation is published, the API becomes public and therefore difficult to change. Sometimes PhET-iO design meetings can also help come up tandem names. NOTE: "Tandem" is a PhET internal name, publicly to clients the full strings are known as "phetioIDs" referring to PhET-iO elements.
Screen
tandems should end with a Screen
suffix.Property
suffix.model
and view
.Tandem.required
for constructors that already have an options parameter. This default can be helpful for identifying cases where you have neglected to pass a tandem in (because Tandem.required
will error loudly if validating tandems).createGroupTandem
for arrays, dynamic instances, or otherwise numbered tandems. See usages for examples.global
section, see Tandem.globalTandem
.Tandem
instance, each PhetioObject
should be provided a TypeIO. The TypeIO for a PhetioObject
indicates the public api for that PhetioObject
instance. Most instrumented common code Types already have a TypeIO provided as a default option for phetioType
. Property
instances to make it possible to get/set a value, so value changes will appear on the data stream and so the item can be stored and restored in save/load. This is preferred to creating a new TypeIO and implementing get/set within that type.validator
static that can be used to validate the type. When instrumenting a Property
, Emitter
, or other type that validates parameters in which that instance provided valueType
for validation, in most cases the TypeIO's validator
will be redundant to the valueType
field. If this is the case, the valueType
can and should be removed to keep the code simpler and more maintainable. Property
or Emitter
elements by composition, or subclass PhetioObject
.
Run phetmarks=>aqua=>Test Sims(Fast Build) with PhET-iO checked. This will help catch any simulations using the component you just instrumented.tandem: Tandem.required
or tandem: Tandem.optional
to the options accordingly. Here are some conventions to guide this decision:
Tandem.required
tandem: Tandem.required
to default options wherever you intend to pass tandem via options.Node
already extends PhetioObject
--its PhetioObject
options should be provided to the constructor or mutate
but not both.phetioPrintMissingTandems
flag if you want to collect a list of all required, optional, and uninstrumented common code classes instead of erroring out on the first missing tandem. Each occurrence is numbered to give a better idea of how many the sim has to do.phetioDocumentation
option:
phetioDocumentation
that is client facing to start with a capital letter.DEFAULT_OPTIONS
or at least be very careful about how it is done, see the concerns mentioned in https://github.com/phetsims/phet-io/issues/1179Property
via addLinkedElement
If necessary, create new TypeIOs to support desired feature set. Generally we don't want to be locked in to coupling TypeIOs to sim types. Instead, we decided that we want the PhET-iO API to be able to vary independently from the sim implementation instead of leaking sim implementation details (like MultilineText vs Text should both just be TextIO). Still, for a well-designed simulation, TypeIOs will often match closely with the sim types. To ensure good IO type inheritance hierarchies follow these principles:
See https://github.com/phetsims/beers-law-lab/issues/213 for more context on prior problems in this area and discussion about it.
Also note that the since work completed in https://github.com/phetsims/phet-io/issues/1398, PhET-iO interframe
communications run on structured cloning (via PostMessage
), and not just JSON strings. This means that a failure to
implement a proper toStateObject
in the TypeIO will result in a hard fail when trying to send instances of that type
to the wrapper side. The error will likely look something like this: "postMessage error from phetioCommandProcessor.send"
(from phetioCommandProcessor.send()
).
Emitter
instances as appropriate to augment the data stream.Emitters
and Property
instances naturally emit to a structured data stream and are probably what you need.
If you need something more custom, you can call PhetioObject.phetioStartEvent
and PhetioObject.phetioEndEvent
directly.Emitter.addListener
instead of Events.onStatic
Emitter.emit
argument, you may specify VoidIO
for its type, see PressListener.js. NOTE: this is
not ideal, and if you need to do this, please consult with a member of the phet-io team before proceeding.phetioEventType: 'user'
for pointer events, keyboard events and UI events
(like checkbox toggled, button pressed), and phetioEventType: 'model'
for model actions/responses. This is easiest to
test in the console: colorized wrapper. Model events will be logged in black, and user events will be logged blue. You can also
go to the data-stream wrapper to see events in JSON form. If your simulation only leverages existing model types (like
Property/Emitter) and UI types (like sun components), then you will not be instrumenting new types.For simulations that have static content (such as a fixed number of objects and properties), instrumentation is mostly complete and you can skip this section. For simulations that have a dynamic number of objects, such as Circuit Construction Kit circuits or Molecules and Light photons, the containers and elements must be instrumented. This is currently tricky with PhET-iO. Some sims may wish to avoid this entire hassle by pre-allocating all of the instrumented instances. Consider adding flags to indicate whether the objects are "alive" or "in the pool".
Data type serialization For example, numbers, strings, Vector2 instances fall into this category. These values
are instantiated by fromStateObject
.
Reference type serialization For example, Nodes and Properties. For example, if a simulation has one heightProperty
that exists for the lifetime of the sim then when we save the state of the sim, we save the dynamic characteristics of
the heightProperty
(rather than trying to serialize the entire list of listeners and phet-io metadata. Then the
PhET-iO library calls setValue()
to update the dynamic characteristics of the heightProperty
without dealing with
all of Property's many attributes. The static setValue
methods on TypeIOs are automatically called by PhET-iO to
restore dynamic characteristics of reference-type serialized instances. Search for toStateObject in *IO.js files for examples.
dispose
d, which unregisters the tandem.dt
values are used instead of Date.now() or other Date functions. This is necessary for
reproducible playback via input events. Perhaps try phet.joist.elapsedTime
.phet.joist.random
, and all doing so after modules are declared (non-statically)? For
example, the following methods (and perhaps others) should not be used: Math.random
, _.shuffle
, _.sample
, _.random
.undefined
values are omitted in the state--consider this when determining whether toStateObject should use null or undefined values.grunt --brands=phet-io
and test the built version by launching build/wrappers/index and testing all the links.?ea&brand=phet-io&phetioStandalone&fuzz
to
run with assertions, PhET-iO brand and fuzzing.phetmarks --> index --> desired wrapper
. Instead you
can use phetmarks to launch any individual wrapper. Note that the wrapper index in the build version is at the top level of the build dir (build/phet-io/
).Basic tandem passing is complete on branch phetioInstrumentation
.
First dev version: https://phet-dev.colorado.edu/html/energy-forms-and-changes/1.2.0-phetioInstrumentation.1/phet-io/wrappers/studio/
Some things I've noticed to discuss for 09/12/19 design meeting:
Intro Screen
The beakers are three separate pieces instead of one Node so that things like energy chunks and blocks can go between the front and back of the beaker. This doesn't look ideal in Studio (view), but there's no way around it that I'm aware of. Having BeakerView extend node doesn't help, because the BeakerView layers are added in the parent class, not as children to BeakerView (see https://github.com/phetsims/energy-forms-and-changes/issues/252).
UPDATE: I'll try adding Properties to link the node Properties of all three layers, then uninstrument the separate layers.
I didn't instrument the BurnerStandNodes or HeaterCoolerNodes because the collision logic relies on the burner stands being there after startup (for example, you would not be able to hide a BurnerStandNode and then just drop a block in its empty spot). The model is instrumented, though, so the heat-cool level can be controlled.
UPDATE: We will investigate adding a pre-launch dialog to select which elements you want to include, then update the model to respect those query params. These query params would actually remove items when setting up the sim at startup, not just hide them.
Possible sections of the dialog: global? screen1 screen2
Options for customization for this screen: item 1 and 2 (blocks only) item 3 and 4 (blocks or beakers)
Also instrument HeaterCoolerNode
and BurnerStandNode
, aka BurnerNode
. This likely means that these should be part of the customization dialog, but I'll double check.
I added some code to where the thermometers and corresponding nodes are created to give them unique names like temperatureAndColorSensor2
. Is this our usual pattern for fixed numbers of the same thing?
UPDATE: Yes, but let's one-index them instead of zero. We also want to rename the code instances and tandem names to thermometerN
and thermometerNodeN
. Also, these need some work for setting state.
Systems Screen
The model controls the opacity of the EnergySystemElements as they come in an out of view with an opacity Property. This duplicates the functionality of the opacity Property in the corresponding view node, so I'm thinking the opacity Properties should be uninstrumented, unless we find that causes unexpected problems.
UPDATE: Yep, uninstrument them.
I'm excited how nicely the three *Carousel.targetIndexProperty
works! @samreid suggested turning this into an EnumerationProperty
so you can select named EnergySystemElements instead of indices, which sounds very nice.
UPDATE: Yep, convert this to use EnumerationProperty.
The fan animation is a loop of ten images, and it looks like I programmed that as one full rotation of 2pi radians. But, since this is a side angle of the fan, the animation is actually only like < pi/4 radians, so the behavior when sliding the control for fan.bladePositionProperty
feels pretty strange. I feel like I should fix this so it does a full rotation over 2pi radians, and then will hopefully just need to decrease a constant elsewhere in order for the behavior to look the same as it currently does when using the sim.
There is currently not much customization for *SelectorPanel
s in the view. I think our desired behavior is to be able to remove specific buttons from the panels, not just the button group as a whole. RadioButtonGroup
is a common code component, so we'll have to make modifications if we want each of the buttons instrumented individually.
UPDATE: Yes, further instrument this common code component so that individual buttons can be removed from the button group. The combination of removing a button and setting its corresponding carousel element to not visible will be how we remove elements from the second screen. Right now, turning off an element's visibility only works until the selected element is changed, and then something is making it visible again. My plan is to remove whatever in the model is changing visibility, and only let the model control opacity.
I didn't instrument the model Belt
because it only has a visibleProperty
, which duplicates the visibleProperty
in BeltNode
.
I instrumented all of the systems element nodes as one unit, except SunNode
, which has a Clouds panel and slider. We can get more granular if we'd like (e.g. adding children to the BeakerHeater
like the beaker, thermometer, and heater).
We will investigate adding a pre-launch dialog to select which elements you want to include, then update the model to respect those query params. These query params would actually remove items when setting up the sim at startup, not just hide them.
@zepumph suggested moving this customization as an optional menu dialog located in the sim along with a button to relaunch the sim once the customizations are applied. This would allow for the customization to be confined to one place, instead of every entry point for the sim. In Studio, we would need to find a way to relaunch Studio, not just the sim itself.
This pattern would also make customizing for phet brand simpler, because the website's sim page wouldn't need to know about which query params are available for a given sim version.
We'll bring this up in design meeting and discuss further.
@zepumph what do you think about bypassing the normal sim construction and going straight to the query parameter UI via a query parameter, like: sim.html?configure . This would still version and deploy within the sim HTML.
That sounds like it is worth investigation! In the sim feels like it is most robust, and will be easiest to maintain. I still reserve the right to be outvoted if people think that this belongs outside the sim.
@zepumph what do you think about bypassing the normal sim construction and going straight to the query parameter UI via a query parameter, like: sim.html?configure . This would still version and deploy within the sim HTML.
Would this menu delay normal startup of the sim until the configure menu was submitted? If so, it seems nice that the sim wouldn't have to be reloaded for the customization to take place. It feels a little weird that you add a query parameter in order to add more query parameters, but I can picture an alternate link that would be labeled for customizations, so the client still doesn't need to know about query parameters.
How about a separate HTML page that is deployed alongside the sim HTML?
Latest dev: https://phet-dev.colorado.edu/html/energy-forms-and-changes/1.2.0-dev.6/phet-io/
Some features added since last comment:
Work done since 11/20/19 phet-io design meeting:
Notes for future work:
Latest dev:https://phet-dev.colorado.edu/html/energy-forms-and-changes/1.2.0-dev.7/phet-io/
EFAC with phet-io was published in 1.4.0. I think a lot of things can probably be checked off in this issue now, and we should consider just closing it once it has been updated.
Oops, I didn't mean to actually close it. It should probably be updated a bit before we do.
We are planning on having a design meeting for PhET-iO instrumentation of this sim while it's still fresh for @jbphet and me.