search

phetsims / forces-and-motion-basics

"Forces and Motion: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/forces-and-motion-basics
GNU General Public License v3.0
7 stars 10 forks source link

optionalTandem indexes appearing in tandem names. #239

Closed phet-steele closed 6 years ago

phet-steele commented 7 years ago

Here: http://www.colorado.edu/physics/phet/dev/html/forces-and-motion-basics/2.3.0-phetiorc.3/wrappers/instance-proxies/instance-proxies.html?sim=forces-and-motion-basics&relativeSimPath

There are many many TEmitter proxies that have the name optionalTandem### within them. This is no bueno, and I'm not sure why it is happening. It would be better to have the names of the objects the Emitter is for, otherwise it's impossible to know for sure. For example a good Emitter where I know exactly what it applies to: forcesAndMotionBasics.navigationBar.homeButton.startedCallbacksForFiredEmitter: TEmitter

A bad Emitter that looks quite arbitrary: forcesAndMotionBasics.optionalTandem564.startedCallbacksForFiredEmitter: TEmitter

No idea if I should assign @zepumph or not, but I did it! Can't stop me now! Seen on macOS 10.12.6 Chrome. For phetsims/QA#42.

samreid commented 7 years ago

Supplying a tandem for the button model resolves the problem, but why was the optional and not supplied tandem being added in the first place? First of all, it was marked as "supplied", and I'm not sure why. This would be good for @zepumph to investigate further, and let me know if we should join forces.

zepumph commented 7 years ago

I see. In the above commit the direct problem was fixed, but there is a greater problem here.

First of all, it was marked as "supplied", and I'm not sure why

It wasn't marked as supplied, but rather it's child was. This is because supplied and required aren't being passed to children in the createTandem() call.

options = _.extend( { static: this.static, enabled: this.enabled }, options );

It should also be cherry picking supplied, and potentially required. I'm not sure.

zepumph commented 7 years ago

Then a problem like the above tandem "forcesAndMotionBasics.optionalTandem564.startedCallbacksForFiredEmitter" would never show up, since it is a child of the optional and not supplied 564. I think that is the best way to go. What do you think @samreid?

samreid commented 7 years ago

I'm not really sure. If a parent is supplied does that mean the child is also supplied? If the parent is required, does that mean the child is required? This seems like it warrants discussion.

jessegreenberg commented 7 years ago

@samreid @zepumph do you see this as an "aesthetic" issue or one that needs to be fixed before a version of FAMB can be deployed?

zepumph commented 7 years ago

@jessegreenberg you are good to go on this one! I implemented the general solution by passing the proper options to children in createTandem(). I'm going to leave this open for @samreid to review, but I think this will certainly work for FAMB.

samreid commented 6 years ago

I think @zepumph implemented this in the correct way. I'm not 100% sure the behavior will be correct for all corner cases, but hopefully it will come up on Bayes or during RC testing if not.