phetsims / john-travoltage

"John Travoltage" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/john-travoltage
GNU General Public License v3.0
4 stars 8 forks source link

Voicing options can't be mutated until after initialization #456

Closed zepumph closed 1 year ago

zepumph commented 2 years ago

From https://github.com/phetsims/scenery/issues/1283#issuecomment-992800551, we need to call initializeVoicing before mutating any voicing options.

zepumph commented 2 years ago

@jessegreenberg, it's not clear to me if this is the long-term solution here, can you please recommend? Perhaps, if possible, it is best to mutate at the end of the constructor.

jessegreenberg commented 2 years ago

This change makes sense to me and is in line with https://github.com/phetsims/scenery/issues/1316. I am good with committing to it. I don't feel too strongly about where the mutate is called, though often it makes sense at the end of the constructor after fields have been calculated. I could also see using setters explicitly instead of mutate sometimes.

I am not sure I got to the crux of your question, feel free to pass it back if there is something I should consider more deeply.

zepumph commented 2 years ago

though often it makes sense at the end of the constructor after fields have been calculated

I feel like this is most-often preferred, but I lacked the sim-specific knowledge to do this myself. If moving the mutate to the end works in this case, would you please do so and feel free to close?

jessegreenberg commented 1 year ago

This issue is stale, initializeVoicing is no longer needed with the Voicing trait and the mutate call lives at the end of the constructor. Closing.