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

Energy Balance is read out incorrectly if sim has not yet sarted #176

Closed
Tracked by #819
KatieWoe opened this issue Jun 14, 2022 · 12 comments
Closed
Tracked by #819

Energy Balance is read out incorrectly if sim has not yet sarted #176

KatieWoe opened this issue Jun 14, 2022 · 12 comments
Labels

Comments

@KatieWoe
Copy link
Contributor

Test device
Dell
Operating System
Win 11
Browser
Chrome
Problem description
For phetsims/qa#811
If you turn on the Energy Balance graph before you start the sunlight in the sim, an alert will play saying there is a net inflow of energy. This is true once you start the sunlight, but before then the graph is blank and there is no information available as no actions are happening yet.

Visuals
basealertwrong

Troubleshooting information:

!!!!! DO NOT EDIT !!!!!
Name: ‪Greenhouse Effect‬
URL: https://phet-dev.colorado.edu/html/greenhouse-effect/1.1.0-dev.9/phet/greenhouse-effect_en_phet.html?screens=1&postMessageOnLoad&postMessageOnError&supportsInteractiveDescription=true
Version: 1.1.0-dev.9 2022-06-08 20:37:50 UTC
Features missing: applicationcache, applicationcache, touch
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/102.0.0.0 Safari/537.36
Language: en-US
Window: 1280x649
Pixel Ratio: 1.5/1
WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium)
GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium)
Vendor: WebKit (WebKit WebGL)
Vertex: attribs: 16 varying: 30 uniform: 4096
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32)
Max viewport: 32767x32767
OES_texture_float: true
Dependencies JSON: {}

@KatieWoe KatieWoe added the type:bug Something isn't working label Jun 14, 2022
@KatieWoe
Copy link
Contributor Author

What's worse, is the scene description describes the opposite from the alerts:
opposite

@jbphet
Copy link
Contributor

jbphet commented Jun 22, 2022

I doubt there is a description that was designed for this situation, so @jessegreenberg will work on adding some handling for it, then run it by the design group at our next meeting.

@jessegreenberg
Copy link
Contributor

Currently the description is determined from inRadiativeBalanceProperty. Before the sunlight starts inRadiativeBalanceProperty.value is false because the energy coming from the sun is non-zero. This is true for the netInflowOfEnergyProperty.

Is this expected, or should this part of the model change? If this is expected we can handle this case in a special way for the description. But I think the descriptions would be correct if the inRadiativeBalanceProperty were true before sunlight starts.

@jbphet what do you think?

@jbphet
Copy link
Contributor

jbphet commented Jun 22, 2022

The design team discussed this in our 6/22/2022 meeting and decided that if sunlight is off the description should only include the part that says, "Energy Balance added to Observation Window" and omit the part that says, for example, "Outgoing energy is less than incoming energy at top of atmosphere, net energy inflow to Earth". It just feels a bit odd to have it say that the energy is in balance if there isn't any. It would be technically true, but not very useful.

Then, once the sunlight is started and the energy balance changes, there should be an alert that says this, basically saying "Outgoing energy is less than incoming energy at top of atmosphere, net energy inflow to Earth".

@jbphet
Copy link
Contributor

jbphet commented Jul 1, 2022

I have this mostly working as desired, but while testing it I ran into one odd and hard-to-reproduce case. Here is the sequence needed to reproduce. The timing matters, so read through it before doing it if you intend to try it.

  • Load the sim in the a11y view and go to the Waves screen
  • Start the sunlight
  • Hit the Reset All button and then very quickly re-enable the energy balance indicator
  • Continue cycling through the process of starting sunlight, reset, quickly re-enabling energy balance, and eventually you'll see a description of the energy balance come out before the sun is shining. It will look something like this:

image

There are other, similar scenarios that can be created, but this is one of the easiest to reproduce.

After looking through the code, I think what's going on here is that the GasConcentrationAlerter is stateful, but it's state isn't getting reset with the rest of the sim. It also updates its internal state at 4 second intervals whether the sun is shining or not. So, if you happen to reset the screen and then re-enable the energy balance indicator inside one of those 4 second windows, it will use the previous energy balance to decide whether to log an alert.

I think the best solution to this is probably to reset GasConcentrationAlerter when the model is reset.

@jbphet
Copy link
Contributor

jbphet commented Jul 1, 2022

@jessegreenberg - I think I have this working, but I'd like you to take a look at the changes before the next steps. My biggest question is about resetting the concentration describer. To fix the problem described in the previous comment, I ended up wiring through a way to reset the GasConcentrationDescriber. It seemed reasonable me, but I'm fairly new to description, so I want to make sure this approach is copacetic.

If this seems good, please assign to @arouinfar and @Matthew-Moore240 to review the behavior.

@jbphet jbphet assigned jessegreenberg and unassigned jbphet Jul 1, 2022
@jessegreenberg
Copy link
Contributor

Ah, that makes sense with this approach. Thanks for catching/adding.

Should these "previous" value variables be reset here as well or were they intentionally excluded?

previousTemperature
previousOutgoingEnergy

@jbphet
Copy link
Contributor

jbphet commented Jul 5, 2022

Thanks @jessegreenberg - I've added those to the reset method.

@jbphet
Copy link
Contributor

jbphet commented Jul 5, 2022

@arouinfar - Please check the behavior and make sure that it is what we want based on the design team discussion above. If so, this can be closed.

@jbphet jbphet assigned arouinfar and unassigned jbphet Jul 5, 2022
@arouinfar
Copy link
Contributor

Looks good, thanks @jbphet

@jbphet
Copy link
Contributor

jbphet commented Jul 8, 2022

Reopening, since I thought it would be good to have QA double check this during a dev test that is currently being set up.

@KatieWoe
Copy link
Contributor Author

This looks fixed in a11y view

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

No branches or pull requests

4 participants