phetsims / blackbody-spectrum

"Blackbody Spectrum" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 3 forks source link

PhET-iO instrumentation #83

Open chrisklus opened 5 years ago

chrisklus commented 5 years ago

Before instrumenting

Code Review

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.

Instrumentation

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 and its PhET-iO instrumentation or reach out to teammates who may have come this way before.

Initial Setup

Visit Objects that Should be Instrumented

Consult the PhET-iO design issue to see what features the sim should support. See https://github.com/phetsims/tandem/tree/master/js/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.

Creating and Naming Tandems

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."

Feature Support

Create new IO types

If necessary, create new IO types to support desired feature set. Generally we don't want to be locked in to coupling IO Types 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. Still, for a well-designed simulation, IO Types will often match closely with the sim types. To ensure good IO type inheritance hierarchies follow these principles:

The Data Stream

Post Instrumentation and Checks

Support dynamic state

For simulations that have static content (such as a fixed number of objects and properties), instrumentation is complete after you have completed the preceding steps. 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".

Details about how to support dynamic state. Beer's Law Lab and Charges and Fields demonstrates how this may be done. A container class defines two methods: `clearChildInstances` which empties a container and `addChildInstance` which repopulates a container one element at a time. For example, see ShakerParticlesIO in the beers-law-lab instrumentation. When state is set, first the container is cleared, then children are created. Child states can be obtained from `toStateObject` and set back with `fromStateObject`, with an additional call to `setValue` in case additional data is supplied, or custom code can be used. Dispose must be implemented properly on all dynamic instances, or else it will result in stale values in the playback sim. For example, if a simulation is sending the position of a particle as a property, if the particle position property hasn't been disposed of, the simulation will try to create a new property with the same id and hence throw an assertion error because that tandem is already registered. On January 11, 2017 ControlPoints were not being disposed correctly in Energy-skate-park-basics, causing a mysterious bug (impossible set state), make sure that children are being disposed correctly before creating them in the downstream sim! Other tips and tricks for "impossible set state": * addChildInstance must return the instance, it is used as a flag to determine whether addition was successful * the given tandems must be reused. Do not use GroupTandem to assign a new tandem, use the specified tandem so the object can be addressed the same way Dispose functions must be added to types that are instrumented. But that's only half of the memory management issue. The other half is revisiting memory management for all instances that don't exist for the lifetime of the sim, and verifying that tandems are properly cleaned up.

Tips, Tricks, Notes, Misc

Two types of serialization

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 IO Types are automatically called by PhET-iO to restore dynamic characteristics of reference-type serialized instances. Search for toStateObject in *IO.js files for examples.

Review and Publication

Happy instrumenting!

chrisklus commented 5 years ago

Benchmark sim prior to instrumentation: https://phet-dev.colorado.edu/html/blackbody-spectrum/1.0.0-dev.14/phet/blackbody-spectrum_en_phet.html

chrisklus commented 5 years ago

@arnabp the instrumentation process required a non-trivial refactor for item number 5 on the list in https://github.com/phetsims/blackbody-spectrum/issues/36#issuecomment-425247890. The relevant changes are in https://github.com/phetsims/blackbody-spectrum/commit/1dc0c91ebb2103fbec6dcefdd3d76ec1cb44a87a, could you please review?

arnabp commented 5 years ago

@chrisklus reviewed BGRAndStarDisplay. I put the node's initial layout placement back in BlackbodySpectrumScreenView, everything else looks good.

chrisklus commented 5 years ago

@arnabp sounds good, thanks.