Closed pixelzoom closed 2 years ago
Lot of work, many changes here. But the end result is much better, the Studio tree makes more sense, and there are fewer dynamic elements.
The first checklist item above was a bit of a compromise, but as I noted in SnapshotNode.ts:
// It's unfortunate that EquationNode is not created from snapshotProperty. But creating it from what's
// currently on the balance scale allows us to reuse the same code that displays the equation for the
// balance scale. And the end result is correct.
const equationNode = new EquationNode( scene.leftTermCreators, scene.rightTermCreators, {
Closing.
Reopening. There are a couple of additional tasks to do:
Using copies for Term for snapshots uses more memory than object literals, but still seems reasonable. Since it simplifies the implementation (and eventually the IOType), I think it's worth the extra memory resources.
Closing.
Reopening. I neglected to dispose of the Terms created for the snapshots.
Summary of the above changes:
Closing.
The snapshot feature currently works correctly in the sim. But there are multiple deficiencies that are exposed by PhET-iO, including:
[x] The model (Snapshot) is not used by the view (SnapshotNode).
[x] The snapshot of a Term (TermSnapshot) is rather odd, and is created dynamically. A better approach would be to save a copy of the original Term, which is what is eventually done in
TermCreator.restoreSnapshot
. It would also result in one less IOType, and one less PhetioGroup. This likely also make PlateSnapshot (inner class of Snapshot) unnecessary.[x] Responsibility for a restoring a snapshot seems misplaced. It's current in
Snapshot.restore
. Investigating moving this to EqualityExplorerScene.[x] On the view side, a SnapshotNode is created every time snapshotProperty changes. To reduce PhET-iO implementation cost, it would be preferable to instantiate SnapshotNode once, and have it mutate to match snapshotProperty.