Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Systems Screen] designer review comments/questions for PhET-iO #307

Closed
6 of 7 tasks
arouinfar opened this issue Jan 8, 2020 · 5 comments
Closed
6 of 7 tasks

[Systems Screen] designer review comments/questions for PhET-iO #307

arouinfar opened this issue Jan 8, 2020 · 5 comments

Comments

@arouinfar
Copy link
Contributor

arouinfar commented Jan 8, 2020

Related to #216

@kathy-phet and I reviewed the Systems Screen in studio, and have some comments and questions for @chrisklus.

  • The Feed Me button should be instrumented, and ideally nested under model.energySources.biker and view.energySoruces.bikerNode. Its properties should probably all be read only because the button only appears when the biker's energy is depleted.

  • It seems that a more appropriate value for energyFormsAndChanges.systemsScreen.model.energyUsers.beakerHeater.thermometer.sensedElementNameProperty would be energyFormsAndChanges.systemsScreen.model.energyUsers.beakerHeater.waterBeaker since the thermometer is on the waterBeaker. Currently, the value is just the beakerHeater.

  • Since the waterBeaker and thermometer are not movable on the Systems screen, we should remove energyFormsAndChanges.systemsScreen.model.energyUsers.beakerHeater.thermometer.userControlledProperty and energyFormsAndChanges.systemsScreen.model.energyUsers.beakerHeater.waterBeaker.userControlledProperty

  • Is thermometer.positionProperty needed on this screen? Its value never changes.
    energyFormsAndChanges.systemsScreen.model.energyUsers.beakerHeater.thermometer.positionProperty

  • Everything related to beltNode should be read only.

  • The name FaucetAndWaterNode seems a bit out of place since it corresponds to the faucet. Can we simplify it to FaucetNode without too much hassle?

  • The litProportionProperty reaches a ceiling of ~0.7 with energy chunks off, but will go to 1 with energy chunks on. We would expect the energy in the light bulb to remain unchanged when toggling energy chunks. While we understand this difference is due to the differences in the underlying model with energy chunks on vs. off, this could be confusing to the instructional designers or muddy the data stream. We were wondering if there would be some way always supply the energy-chunks-off version of the number (perhaps create some other readout altogether).

  • The state is not being correctly saved ([Systems Screen] State is not being correctly saved #306)

@chrisklus
Copy link
Contributor

The Feed Me button should be instrumented, and ideally nested under model.energySources.biker and view.energySoruces.bikerNode.

The feed me button only appears under view.energySoruces.bikerNode because it's a UI component that's not directly correlated to a Property (model.energySources.biker.energyChunksRemainingProperty is most related). Is that sufficient?

Is thermometer.positionProperty needed on this screen? Its value never changes.
energyFormsAndChanges.systemsScreen.model.energyUsers.beakerHeater.thermometer.positionProperty

Technically no, just position would suffice for this particular thermometer. But positionProperty exists at the root of all model elements in this sim, and this is an unusual case of where the Property is not changed after startup. leftBurner and rightBurner are the other two examples I can think of. I would prefer to just leave them un-featured if that's reasonable enough - otherwise I could add an option to not instrument positionProperty for those cases.

The name FaucetAndWaterNode seems a bit out of place since it corresponds to the faucet. Can we simplify it to FaucetNode without too much hassle?

Yes, actually - FaucetAndWaterNode.js should be named as such because it manages the FaucetNode and FallingWaterCanvasNode. However, the instance of it is already named faucetNode, so I updated the Tandem to match. Instrumentation mistake on my part.

We were wondering if there would be some way always supply the energy-chunks-off version of the number (perhaps create some other readout altogether).

Sure thing - the maximum values of the lightbulb for energy chunks on and off are both clamped at 1. For now, I reduced the max value when chunks are on to 0.7, so the maximums approximately match for the faucet, sun, and tea kettle. Note that the biker can crank the bulb up to 1 with chunks off when she's at full power, so I could also clamp the value when chunks are off at 0.7, if desired. This would make this Property value look a bit nicer for the other sources, too, and be more consistent in the code. At the same time, I've always loved that the biker is the most powerful source on the screen :)

@chrisklus
Copy link
Contributor

@arouinfar and I met and discussed these today:

The feed me button only appears under view.energySoruces.bikerNode because it's a UI component that's not directly correlated to a Property (model.energySources.biker.energyChunksRemainingProperty is most related). Is that sufficient?

Yep, sounds good.

I would prefer to just leave them un-featured if that's reasonable enough - otherwise I could add an option to not instrument positionProperty for those cases.

@arouinfar is on board with just leaving them un-featured.

Lastly, @arouinfar and @kathy-phet will discuss the light bulb behavior together.

@arouinfar
Copy link
Contributor Author

Back to @chrisklus since there's some work left to be done on the litProportionProperty and angularVelocityProperty.

@chrisklus
Copy link
Contributor

Work done in the above recent commits:

  • added sunProportionProperty
  • moved clouds slider into clouds panel
  • instrumented the biker control panel and nested the biker's slider in it
  • omitted the flickering Studio value energyOutputRateProperty from the Solar panel

Anything else for this issue @arouinfar?

@chrisklus chrisklus assigned arouinfar and unassigned chrisklus Feb 13, 2020
@arouinfar
Copy link
Contributor Author

@chrisklus everything's looking good in studio, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants