search

phetsims / mean-share-and-balance

"Mean: Share and Balance" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 1 forks source link

Balance Point Screen Initial PhET-iO Implementation #196

Closed marlitas closed 1 month ago

marlitas commented 3 months ago

This issue will track commits for initial PhET-iO work and implementation in the Balance Point Screen including studio and state wrapper testing/support.

marlitas commented 3 months ago

PhET-iO is looking pretty good for screen 4. I think this is ready for code review and the studio tree is ready for design review.

amanda-phet commented 3 months ago

I took a look at both the 1st and 4th screens. I think thing are looking good, but once all screens are ready, I'd like to do a review with @arouinfar since she understands studio tree design patterns a lot better than I do.

Here are my questions/comments:

  1. Is meanShareAndBalance.introScreen.model.pipes.pipe1.rotationProperty necessary? Since it's read-only, perhaps this doesn't matter, but I'm not sure why this information is useful in studio as long as we have meanShareAndBalance.introScreen.model.arePipesOpenProperty
  2. I think meanShareAndBalance.introScreen.view.controls.introOptionsCheckboxGroup can just be called meanShareAndBalance.introScreen.view.controls.checkboxGroup to be consistent with Balance Point screen.
  3. If I make meanShareAndBalance.balancePointScreen.model.sceneModel.maxKicksProperty = 8, I can't actually allow 8 kicks. If I make it 2, the number spinner still lets me increase the number (although no balls are kicked, it's just odd)
  4. I don't understand meanShareAndBalance.balancePointScreen.view.meanInfoDialog.isShowingProperty
  5. It is inconsistent to have meanShareAndBalance.balancePointScreen.view.notepadNode.visibleProperty show/hide the notepad and all contents, while meanShareAndBalance.introScreen.view.notepadNode.visibleProperty shows/hides the notepad only (contents visibility is separate). Not sure which one is the better design pattern, but one should be changed to match the other.
  6. I don't see a property anywhere that has the total distance kicked. However, we'll be adding a checkbox for that soon, so I image a property will come with it then. I am not sure I saw one for balls kicked, either, but activeKickIndexProperty is a proxy for that information.
marlitas commented 3 months ago
  1. Is meanShareAndBalance.introScreen.model.pipes.pipe1.rotationProperty necessary? Since it's read-only, perhaps this doesn't matter, but I'm not sure why this information is useful in studio as long as we have meanShareAndBalance.introScreen.model.arePipesOpenProperty

Un-instrumented above.

  1. I think meanShareAndBalance.introScreen.view.controls.introOptionsCheckboxGroup can just be called meanShareAndBalance.introScreen.view.controls.checkboxGroup to be consistent with Balance Point screen.

Done above.

  1. If I make meanShareAndBalance.balancePointScreen.model.sceneModel.maxKicksProperty = 8, I can't actually allow 8 kicks. If I make it 2, the number spinner still lets me increase the number (although no balls are kicked, it's just odd)

That is very weird. I will look at it.

  1. I don't understand meanShareAndBalance.balancePointScreen.view.meanInfoDialog.isShowingProperty

This is the property that tells us wether the mean info dialog is visible or not. I can change the tandem name so that it's consistent.

  1. It is inconsistent to have meanShareAndBalance.balancePointScreen.view.notepadNode.visibleProperty show/hide the notepad and all contents, while meanShareAndBalance.introScreen.view.notepadNode.visibleProperty shows/hides the notepad only (contents visibility is separate). Not sure which one is the better design pattern, but one should be changed to match the other.

That is strange. I'll adjust.

  1. I don't see a property anywhere that has the total distance kicked. However, we'll be adding a checkbox for that soon, so I image a property will come with it then. I am not sure I saw one for balls kicked, either, but activeKickIndexProperty is a proxy for that information.

I instrumented that DerivedProperty it is phetioReadonly and is excluded from state setting. So essentially this is just for clients to be able to read. It can be found under sceneModel in the BalancePoint screen.

marlitas commented 2 months ago
marlitas commented 2 months ago

It is inconsistent to have meanShareAndBalance.balancePointScreen.view.notepadNode.visibleProperty show/hide the notepad and all contents, while meanShareAndBalance.introScreen.view.notepadNode.visibleProperty shows/hides the notepad only (contents visibility is separate).

@jbphet and I talked through this one, and we started to question why clients need to toggle the visibility of the notepad in the first place? It feels like that's a major part of the sim. Can we uninstrument that visible Property?

amanda-phet commented 2 months ago

I agree- yes, let's un-instrument that!

marlitas commented 2 months ago

Done! Over to @jbphet for code review.

jbphet commented 1 month ago

Code looks good. Closing.