Closed pixelzoom closed 3 years ago
RewardNode and its subclasses were converted to ES6 in the above commit.
@jonathanolson MakeATenRewardNode was a piece of cake to convert, and easy to test. So unassigning you, and thank you!
@jbphet BAARewardNode was really messed up, and required some big changes. It's also using deprecated tandem.createGroupTandem
, and I had no idea how to deal with that or test it. I also couldn't spend the time to play the EE game (query parameter showRewardNodeEveryLevel
was no help), so you'll have to have a look at that one too -- it's currently untested. Highly recommended to add a query parameter that shows the reward regardless of what the score is -- imo that should be required for all sims that have a reward.
@samreid 2 things:
(1) Please have a look at RewardNode line 114. This looks like an old way of checking if state is being set. Shouldn't this be checking phet.joist.sim.isSettingPhetioStateProperty.value
?
114 if ( Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine ) {
(2) This code was not up to PhET standards. Missing visibility, missing param annotations, missing field definitions in the constructor... Please review to make sure that my guesses were correct.
Assigning to @ariel-phet to prioritize, since I don't know if any sims with games/rewards are in the QA pipeline.
I just noticed that RewardNode wasn't following the convention for implementing dispose
, so I cleaned that up in the above commit. The stop
method is gone, absorbed into disposeRewardNode
. Since it was only used in dispose
, I assumed it was private. And there is no start
method or documentation about the intent of the stop
method, so it looked like a way of unhooking the stepEmitter listener.
@samreid please review.
@samreid mentioned at dev meeting today that this should block publication of anything that uses RewardNode.
In the review, I spotted a few places where arrow functions should be used, a site where we may be able to use this
instead of self
and an IIFE that may no longer be necessary. I'll make some commits.
I tested build an atom with ?reward and did not see any problems.
Review complete.
Back to @pixelzoom to review the changes, and to recommend next steps. Should we assign this to @jbphet to test in EE as a last step?
Back to @samreid. You didn't address or comment on item (1) in https://github.com/phetsims/vegas/issues/82#issuecomment-693967615.
Yes, leave assigned to @jbphet -- he has work to do as described in https://github.com/phetsims/vegas/issues/82#issuecomment-693967615.
Back to @samreid. You didn't address or comment on item (1) in #82 (comment).
Item (1) says:
(1) Please have a look at RewardNode line 114. This looks like an old way of checking if state is being set. Shouldn't this be checking
phet.joist.sim.isSettingPhetioStateProperty.value
?
114 if ( Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine ) {
The pattern was suboptimal and I changed it from:
this.initializationVerifier = null;
if ( Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine ) {
this.initializationVerifier = () => {
if ( !this.isInitialized ) {
this.initialize();
}
};
Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( this.initializationVerifier );
}
to
this.initializer = () => this.initialize();
Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( this.initializer );
We use Tandem.PHET_IO_ENABLED
rather than isSettingPhetioStateProperty
because we are deciding whether to add a listener, not determining if we are in the process of setting state at the moment.
Thank for clarifying the phet-io related changes.
From the 10/1/2020 developer meeting, I should:
createGroupTandem
is being used)I removed the instrumentation of the reward nodes and tested both, they looked fine. Screenshots below (the nodes were moving correctly). I think that's it, right @pixelzoom?
Yep. Looks like we're done here. Closing.
From https://github.com/phetsims/tasks/issues/1044, where I was converting RewardNode to ES6.
This needs its own issue because it turned out to be quite complicated, for several reasons:
Assigning to @jbphet because he's responsible for BAARewardNode and EERewardNode.
Assigning to @jonathanolson because he's responsible for MakeATenRewardNode.
Assigning to @samreid to look into the PhET-iO state code in the constructor, and because he's the author of RewardNode.
Mentioning @ariel-phet and @KatieWoe so that they are aware of this work.