Closed marlitas closed 3 months ago
This has been applied above. Over to @amanda-phet and @jbphet for review.
Can you explain how I can check this? I know I've done it in previous sims but I don't remember how to do it now.
Yes, sorry about that! Below are the commands. Copy and paste them into the dev tools console. The commands need to be updated with the relevant screen and data point values.
await phetioClient.invokeAsync( 'meanShareAndBalance.levelOutScreen.model', 'getDataPoints', [] );
await phetioClient.invokeAsync( 'meanShareAndBalance.levelOutScreen.model', 'setDataPoints', [ [0.5, 1, 0.25, 0.4] ] );
await phetioClient.invokeAsync( 'meanShareAndBalance.distributeScreen.model', 'getDataPoints', [] );
await phetioClient.invokeAsync( 'meanShareAndBalance.distributeScreen.model', 'setDataPoints', [ [2, 4, 1, 8, 9, 3] ] );
The code looks fine, though I'm not too familiar with adding phet-io-specific methods using IO types, so I'm just reviewing in a very general way.
I have a couple of questions about the behavior:
<sim>.<screen>.model
for the first three screens but has an extra level for the 4th screen called sceneModel
. For example, it's meanShareAndBalance.fairShareScreen.model
for the "Fair Share" screen but meanShareAndBalance.balancePointScreen.model.sceneModel
for the "Balance Point" screen. It took me a while to figure this out, and I only got there because I know the code. Is this a problem? Or do we just want to handle it with documentation? If it would be better to be consistent, we may be able to add a 'pass through' method to the Balance Point model (but note that it would get rid of the method in SceneModel
).[[]]
causes and assertion to be thrown. Since we ignore extra values, would it be more consistent to just ignore this?The first bullet point is more a question for @amanda-phet I think, so I'll send that to her.
Since we ignore extra values, would it be more consistent to just ignore this?
Yes, I think that makes a ton of sense. I'll add that functionality in.
Last bit is for feedback from @amanda-phet.
Don't forget @jbphet's question from https://github.com/phetsims/mean-share-and-balance/issues/241#issuecomment-2155526699
The commands work great!
The phet-io ID is of the form
. .model for the first three screens but has an extra level for the 4th screen called sceneModel.
This seems not ideal at all, and I wouldn't have known that if @jbphet hadn't mentioned it. If we can make all screens identical that would be great. If this is a soccer-common issue, then I could just be sure to include this in examples.md.
@jbphet and I talked about a workaround that would simplify this for MSaB while not affecting the soccer common API. Essentially clients could access the methods both with sceneModel
and without. I'll go ahead and move forward with that fix.
This has been done. Over to @amanda-phet for review. It is the same implementation as SoccerSceneModel
so does not need a code review.
This is working great for me. Thanks!
From the phet-io design doc:
User Story: Client wants to easily get and set the data values in an array
This is available for Balance Point already (in console- check examples.md for Center and Variability)
Ignore values over 7
Behavior in soccer common we will use to be consistent:
If the number of values > number visible on the screen ⇒ add cups/plates/balls
If the number of values < number visible on the screen ⇒ remove cups/plates/balls