Friction - PhET-iO Design Review

[arouinfar]

@zepumph I took a pass through the tree in Studio. Overall, things look pretty good.

• Preferences button & dialog need to be instrumented, see PhET-iO instrumentation of Preferences and its dialog joist#744
• Alignment of contents of A11yButtonsHBox looks strange if one or more of the buttons is hidden, see Right-align contents of A11yButtonsHBox joist#747
• model.atoms seems like a candidate for phetioGroup. Should it be converted?
• This instrumentation is a bit older, so verify that we are using the latest patterns for listeners. Additionally, view.atomicView.grabDragInteraction seems like an odd thing to show up in the tree.
• view.atomicView.atomDragArea.visibleProperty seems like a relic from the age of over-instrumentation. It hides the blue rectangle in the atomic view, but its associated atoms are still visible. Can it be uninstrumented? If not, let's make it phetioReadOnly: true.
• Add an InputEnabledProperty to view.atomicView.atomDragArea and view.topBookNode (or perhaps its dragListener, not sure what the pattern should be).
• The client should be able to permanently hide the cueing arrows. This isn't currently possible because model.hintProperty is overridden by the ResetAllButton (as it should be). A typical strategy is to wrap the cueing arrows in a Node, and moleculePolarity.twoAtomsScreen.view.moleculeNode.hintArrowsNode may be a helpful example.
• Make these phetioReadOnly: true
  • model.contactProperty
  • model.evaporationEmitter
  • model.successfullyInteractedWithProperty
  • model.atoms.atom*.positionProperty (if converting to phetioGroup, the positions should still be read-only)
• Make these phetioHighFrequency: true
  • model.atoms.atom*.positionProperty
  • model.vibrationAmplitudeProperty
  • model.distanceBetweenBooksProperty
• Add phetioDocumentation to the following:
  • model.contactProperty
  • model.successfullyInteractedWithProperty
  • model.topBookPositionProperty (specify the coordinate frame) see What are the position units of this sim? #239
  • model.bottomOffsetProperty see What are the position units of this sim? #239

I have a few open design questions and should discuss with @kathy-phet

• model.atomRowsToEvaporateProperty allows clients to set the number of rows in the chemistry book that will evaporate away. Is this a realistic customization? If so, there needs to be a control in Studio so instructional designers can customize the value (not directly settable in the sim itself).
• There isn't much to customize in the view. A client can hide the atomic view, hide the thermometer, and change the name of the textbooks. Is there anything else we would like clients to be able to do?
• The data stream includes: books in contact, distance between books, book position, vibration amplitude, atom position, emitters for evaporating atoms, and temperature (as a percent). Is there anything else we should include?

Todos from #236 (comment):

• Uninstrument model.atomRowsToEvaporateProperty. If that's not possible, make it phetioReadOnly: true.
• If the above isn't uninstrumented, change its name to model.atomRowsToShearOffProperty
• Change model.evaporationEmitter to model.shearedOffEmitter and update the phetioDocumentation accordingly.

[zepumph]

model.atoms seems like a candidate for phetioGroup. Should it be converted?

In general, a PhetioGroup is for objects that are dynamic. I didn't use PhetioGroup because all atoms are created on startup. I would prefer to not add the overhead of PhetioGroup for purely statically created PhET-iO elements. I purposefully kept the tandem name different (omitting the underscore) so that there wasn't confusion about the dynamic-ness of it. Do you see a UX/studio improvement from having a PhetioGroup here?

[zepumph]

view.atomicView.atomDragArea.visibleProperty seems like a relic from the age of over-instrumentation. It hides the blue rectangle in the atomic view, but its associated atoms are still visible. Can it be uninstrumented? If not, let's make it phetioReadOnly: true.

• I got rid of the instrumented visibleProperty. I also did this for the BookNode (macro books), so they don't have visibleProperty either.

[zepumph]

• The client should be able to permanently hide the cueing arrows. This isn't currently possible because model.hintProperty is overridden by the ResetAllButton (as it should be). A typical strategy is to wrap the cueing arrows in a Node, and moleculePolarity.twoAtomsScreen.view.moleculeNode.hintArrowsNode may be a helpful example.

• I also made the hintProperty phetioReadOnly + documentation so that it was more obvious that you should use the view.

I am adding checkboxes to my thoughts above so that you can check them off when they are done or responded to below. I just don't want anything to fall off here.

[arouinfar]

model.atoms seems like a candidate for phetioGroup. Should it be converted?

In general, a PhetioGroup is for objects that are dynamic. I didn't use PhetioGroup because all atoms are created on startup. I would prefer to not add the overhead of PhetioGroup for purely statically created PhET-iO elements. I purposefully kept the tandem name different (omitting the underscore) so that there wasn't confusion about the dynamic-ness of it. Do you see a UX/studio improvement from having a PhetioGroup here?

Thanks for the explanation @zepumph. This sim was partially instrumented ages ago and predated PhetioGroup, so I thought I'd bring it up. I don't see any particular reason why it needs to be a PhetioGroup, so I'll check that item off in the comment above.

[arouinfar]

• @zepumph if view.atomicView.atomDragArea.inputEnabledProperty is false, model.hintProperty should also be false. The cueing arrows imply that the book is movable in the atomic view, but it's not.

[zepumph]

I created #239 to better understand the position units, phetioDocumentation for those Properties will need to wait on that. I'm getting pretty close to being done here.

[kathy-phet]

Thanks, @arouinfar. Here are some thoughts...

model.atomRowsToEvaporateProperty allows clients to set the number of rows in the chemistry book that will evaporate away. Is this a realistic customization? If so, there needs to be a control in Studio so instructional designers can customize the value (not directly settable in the sim itself).

This doesn't seem like something that would be a customization to me. What do you think?

There isn't much to customize in the view. A client can hide the atomic view, hide the thermometer, and change the name of the textbooks. Is there anything else we would like clients to be able to do?

Maybe hide the initial cuing arrows? (since we have made that possible on some sims)
I don't know why you would want to do it per se, but is their a "pickable false" mode for the book and microscopic book pieces? You might want to prevent interaction with one of them temporarily while you ask a questions, maybe? (It's a stretch I know, just throwing it out there).

The data stream includes: books in contact, distance between books, book position, vibration amplitude, atom position, emitters for evaporating atoms, and temperature (as a percent). Is there anything else we should include?

That seems like it covers everything I can think of.

[emily-phet]

A bit random, but my understanding is that the atoms that leave the book are "vaporizing" (solid to gas), not "evaporating" (liquid to gas). I hadn't realized properties in the code were labeled as evaporate, might be nice to update that language if others agree "vaporize" is the more correct terminology.

I've not been involved with this sim beyond interaction and auditory display features, so perhaps this has already been discussed and settled...just throwing it out there in case others feel the terminology change is more appropriate for user/client facing labels.

[kathy-phet]

Pinged Dubson on this, because I wasn't confident of the answer. His response "No obvious word. Not vaporizing. Not evaporating. Wear and tear. Or shearing off." Hmm....

[emily-phet]

I can see "shearing" - if it's not conceptualized as a change of state. I had been thinking of the atoms flying away as a gas, but if they kind of fly away and theoretically just fall to ground/table then "shear off" makes a lot of sense.
"shear" or "shearOff" could be used. Maybe even just "lose" - as in model.atomRowsToLoseProperty, since regardless of what happens to them, they are definitely "lost" from the book.

[kathy-phet]

I like ToShearOffProperty a bit better, but ToLose also works.

[arouinfar]

model.atomRowsToEvaporateProperty allows clients to set the number of rows in the chemistry book that will evaporate away. Is this a realistic customization? If so, there needs to be a control in Studio so instructional designers can customize the value (not directly settable in the sim itself).

This doesn't seem like something that would be a customization to me. What do you think?

I agree. There are also design concerns with reducing the number of rows that can be sheared off. The rows that cannot be sheared off are embedded in the blue/green rectangles in the atomic view. Changing the number of rows in the model will not impact the view and could be confusing. Let's uninstrument this property, or if that's not possible, make it phetioReadOnly: true.

Maybe hide the initial cuing arrows? (since we have made that possible on some sims)

I forgot to include that feature in the question directed to you, but it was a feature I requested from @zepumph.

The data stream includes...

That seems like it covers everything I can think of.

Me too, thanks for reviewing!

I like ToShearOffProperty a bit better, but ToLose also works.

@kathy-phet @emily-phet thanks for your input on the terminology. There are two places where "evaporate" shows up in the tree: model.evaporationEmitter and model.atomRowsToEvaporateProperty.

If the latter does need to remain instrumented, it would become model.atomRowsToShearOffProperty or model.atomRowsToLoseProperty. The associated emitter would then become something like model.shearedOffEmitter or model.atomsLostEmitter.

I think the "sheared off" language is a bit clearer, so that would be my preference.

@zepumph I'll add the todos from this comment to the original list above.

[zepumph]

#247 has been done, and also the checkboxes above about that specific terminology.

I am going to check off items that have their own issue devoted to them.

[zepumph]

This instrumentation is a bit older, so verify that we are using the latest patterns for listeners. Additionally, view.atomicView.grabDragInteraction seems like an odd thing to show up in the tree.

• This is the latest pattern, but I'm not really if we have designed the instrumentation for the grab drag interaction yet for PhET-iO. Right now it is just a pressListener because that gives info about the input coming from the view (for mouse/touch). Perhaps we should have a bit of a design meeting about that, or what kinds of stuff we could get from the grab/drag class.
  UPDATE: see PhET-iO instrumentation for GrabDragInteraction scenery-phet#720

[zepumph]

That is all that I see left for this general conglomerate issue.

[arouinfar]

Thanks @zepumph. Looks like everything in the main list has been handled (or is tracked in another issue). The only thing missing is #236 (comment).

[zepumph]

In design meeting today, we started to explore this complicated state space. we created three issues linked above. We will keep working.

[zepumph]

Alright. I think all is good for this issue. I created an issue for GrabDragInteraction iO design. @arouinfar, anything else here?

[arouinfar]

Looks good, thanks @zepumph. I think we can go ahead and close.