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

Flux meter alerts occur too often #391

Closed
Nancy-Salpepi opened this issue Feb 16, 2024 · 7 comments
Closed

Flux meter alerts occur too often #391

Nancy-Salpepi opened this issue Feb 16, 2024 · 7 comments
Assignees
Labels
type:bug Something isn't working

Comments

@Nancy-Salpepi
Copy link

Test device
MacBook Air M1 chip

Operating System
14.3.1

Browser
Safari 17.3.1

Problem description
For phetsims/qa#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

FluxMeterAlerts.mp4
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 Nancy-Salpepi added the type:bug Something isn't working label Feb 16, 2024
@Nancy-Salpepi
Copy link
Author

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

@Nancy-Salpepi Nancy-Salpepi changed the title VO reads flux meter too often Flux meter alerts occur too often Feb 16, 2024
@arouinfar
Copy link
Contributor

arouinfar commented Feb 16, 2024

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.

FluxMeterAlerts.mp4

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
Copy link
Author

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

@jbphet
Copy link
Contributor

jbphet commented Feb 20, 2024

The description behavior for the flux meter was reviewed and approved back in #369 (comment), 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
Copy link
Contributor

jbphet commented Feb 21, 2024

@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?

@jbphet jbphet assigned arouinfar and unassigned jbphet Feb 21, 2024
@arouinfar
Copy link
Contributor

arouinfar commented Feb 21, 2024

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

@arouinfar arouinfar assigned jbphet and unassigned arouinfar Feb 21, 2024
@arouinfar
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants