phetsims / greenhouse-effect

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

Flux meter alerts occur too often #391

Closed Nancy-Salpepi closed 7 months ago

Nancy-Salpepi commented 7 months ago

Test device MacBook Air M1 chip

Operating System 14.3.1

Browser Safari 17.3.1

Problem description For https://github.com/phetsims/qa/issues/1043, VO continually gives alerts for flux meter. This happens after checking the flux meter checkbox and also after changing a value, such as the IR absorbance.

Steps to reproduce Here is an example:

  1. Turn on VO
  2. On the Layer Model Screen, press Start Sunlight
  3. Tab to and increase the Absorbance Layers to 2
  4. Tab to and check the Flux Meter checkbox

Visuals

https://github.com/phetsims/greenhouse-effect/assets/87318828/a7fc9485-9662-4d6e-af87-86cbf752b369

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Greenhouse Effect‬ URL: https://phet-dev.colorado.edu/html/greenhouse-effect/1.3.0-dev.4/phet/greenhouse-effect_all_phet.html Version: 1.3.0-dev.4 2024-02-14 22:04:47 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.3.1 Safari/605.1.15 Language: en-US Window: 1321x712 Pixel Ratio: 2/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 (1.0) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 30 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}
Nancy-Salpepi commented 7 months ago

I can get it to happen in the a11yview as well....

  1. On either the Photons or Flux meter screen, press the Start Sunlight button
  2. Tab to and check Flux Meter
Screenshot 2024-02-16 at 1 28 40 PM
arouinfar commented 7 months ago

Thanks @Nancy-Salpepi. The behavior in the video looks correct to me.

Here's the initial alert: image

The incoming IR increased from "very low to low": image

The outgoing IR increased from "moderate" to "high". image

Similarly, the screenshot from the a11y view shows a change in the amount of outgoing sunlight and outgoing infrared.

However, before opening this issue @Nancy-Salpepi shared a video with me on Slack where the flux meter was repeating the exact same value after changing the IR absorbance.

https://github.com/phetsims/greenhouse-effect/assets/8419308/a22cd8f1-92db-4a16-8ca2-bf71130b9512

Here are screenshots of the relevant alerts, occurring at 0:11, 0:19, and 0:27 image image image

I was able to reproduce this behavior in the a11y view when changing the IR absorbance of the layers, similar to the video above. image

Nancy-Salpepi commented 7 months ago

haha....I was trying to find a simpler way to reproduce! I guess I should have stuck with my first video 😁.

jbphet commented 7 months ago

The description behavior for the flux meter was reviewed and approved back in https://github.com/phetsims/greenhouse-effect/issues/369#issuecomment-1865457965, but I understand that things can look different when doing a fully systems test.

The alerts with the same message occur because the code that decides whether to perform an alert (which currently resides in EnergyFluxAlerter) does so based on whether the amount of flux change exceeds a threshold, and not based on whether the text of the alert would be different. @arouinfar and I discussed this earlier today, and we decided that the code should be modified so that it does not repeat the same alert if it is not different from the previous one, even if the flux change exceeds the threshold. I'll endeavor to modify the code to behave this way.

jbphet commented 7 months ago

@arouinfar - I've added code to prevent redundant alerts from the flux meter. They can still occur often, but they shouldn't repeat themselves anymore. I tested it based on the scenario described above where the same alert occurred three times, and only saw that alert once.

Can you please check the behavior on main and let me know if this behavior is what we're after?

arouinfar commented 7 months ago

Thanks @jbphet, looks good! I'm unable to reproduce repeated alerts on main.

arouinfar commented 7 months ago

I verified this behaves in dev.6 (interview version), so I think we can go ahead and close.