phetsims / density

"Density" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 6 forks source link

Number Control Visible Properties are not maintained in Studio #104

Closed Nancy-Salpepi closed 2 years ago

Nancy-Salpepi commented 3 years ago

Test device MacBook air (m1 chip)

Operating System 11.6

Browser chrome

Problem description In the Compare Screen, when the number control visible properties for mass, volume, or density are set to false in the Studio Wrapper, this state is not maintained once the sim is launched.

Steps to reproduce

  1. In the Compare Screen, select "same density" in the blocks panel
  2. Go to density.compareScreen.view.densityNumberControl.visibleProperty and uncheck checkbox to set property to false
  3. Press the "Preview Sim" button

Follow the same steps above but for volume.

For mass:

  1. Go to density.compareScreen.view.massNumberControl.visibleProperty and uncheck checkbox
  2. Press the "Preview Sim" button
  3. Select the "same volume" or "same density" option in the blocks panel
  4. Select the "same mass' option

Visuals densityvisibleproperty

jonathanolson commented 2 years ago

@arouinfar how should this be handled? Also the previewing of the sim isn't needed, just switch back and forth between different scenes/modes and it will make them visible again.

arouinfar commented 2 years ago

Really good catch @Nancy-Salpepi. I'm a bit embarrassed that this one slipped by me.

@jonathanolson there needs to be a way to hide the NumberControl without the model overriding its visibleProperty when switching between the different scenarios. Perhaps the approach used in https://github.com/phetsims/ph-scale/issues/102 (or one of the other issues that reference it) could serve as an example.

jonathanolson commented 2 years ago

@arouinfar I'm curious, the example has a neutral indicator node that says it's visible when it's actually hidden from view. Is that acceptable?

I could use the same method here.

arouinfar commented 2 years ago

I'm curious, the example has a neutral indicator node that says it's visible when it's actually hidden from view. Is that acceptable?

Yes, I think so @jonathanolson. It's saying that it's visible when applicable. There have been a few different approaches to handle situations like this, though. Let's ask the other devs during phet-io meeting.

jonathanolson commented 2 years ago

Sounds like I should discuss with @samreid and @zepumph on what approach to follow here (and we should document it as precedent).

samreid commented 2 years ago

Did we cover this in phet-io meeting today? If not, please reach out to me here or on slack.

samreid commented 2 years ago

@zepumph and @jonathanolson and I discussed this pattern and came up with a slightly different angle than in previous approaches. We use a derived property gate in the same way as before, but then we placed the new controlling property in place of the now-uninstrumented visibleProperty. This means the client will access the visible property in the same way, and it will work as expected regardless of when the scenes are changed.

jonathanolson commented 2 years ago

Commits above with a more specific solution. I believe this should be handled.

Nancy-Salpepi commented 2 years ago

Looks fixed in master @jonathanolson

Nancy-Salpepi commented 2 years ago

Looks good in the dev version as well.