Do all types that require a dispose function have one?

[pixelzoom]

Related to #1 (code review):

• [ ] Do all types that require a dispose function have one? This should expose a public dispose function that calls this.disposeMyType(), where disposeMyType is a private function declared in the constructor. MyType should exactly match the filename.

dispose makes an object instance eligible for garbage collection, to reclaim storage for things that are no longer needed by a program.

I don't see any implementation of dispose in this sim. Is that because all object instances exist for the lifetime of the sim? Or are these things that should be disposed that are not currently being cleaned up?

In any case, this would be a good thing to document in implementation-notes.md. Something like this paragraph in https://github.com/phetsims/vector-addition/blob/master/doc/implementation-notes.md:

Memory Management

The dynamic objects in the sim are the vectors, and their model and view classes implement dispose. On the model side, that includes RootVector and its subclasses; on the view side, RootVectorNode and its subclasses.

All other objects are instantiated at startup, and exist for the lifetime of the sim. Classes that are not intended (and in fact, not designed) to be disposed have a dispose method that fails an assertion if called. For example:

[heldentodd]

This is a difficult one for me. For the most part, things stay for the life of the sim.

I do use removeChild (a lot), and I am assuming that this will dispose of anything in the removed node and all its children. Do you know if this assumption is correct? Also, I am curious if I need to run removeChild before I reassign a value. For example, do I need the removeChild in the following?:

        this.removeChild( this.crystalNode );
        this.crystalNode = new CrystalNode( model.lattice );

I do something similar to animate the light rays. I hadn't thought too much about this because I always thought I might have to rewrite the code to improve performance.

Also, lattice.sites is an array that can change in size, but I always set its length to zero before repopulating it on updates.

[pixelzoom]

The general rule is that memory will be cleaned up ("garbage collected") if nothing has a reference to it.

removeChild will take care of removing a Node from the scene graph, so that the scene graph no longer has a reference to it. If your program has a reference to it elsewhere, that will not be cleaned up by removeChild.

The way that you're managing this.crystalNode and this.lightPathsNode in XrayDiffractionScreenView looks correct. That is, remove the previous instance before creating and adding a new instance.

The way that you're managing this.sites in Lattice.js is perfect. Setting an array's length to 0 is the most efficient way of emptying it, and removing its references to its elements.

[heldentodd]

Thank you. That clarifies things. I think the simulation is good with memory, but I'll keep an eye out for potential problems. I also put something into implementation.md to document that most things are for the life of the sim.