Pressure sometimes doesn't change when the lid is compressing #354
Comments
|
May have something to due with platform (somehow) since I have not been able to recreate on my device. Win 10 Chrome. @phet-steele what were you on? |
|
When trying to drag the lid in master, I immediately see this error. |
|
I've added some debug code on master to try to help track this down. @phet-steele - Can you please reproduce this on your system using master and capture the output on the console? It will be long, so I'd recommend putting it in a "details" section. Also, is there any change the system you're testing with is using a faster-than-normal frame rate for the browser? If so, that would explain the problem. |
|
Note to self: It's a little lame that |
Almost guaranteed! Output======= TimeSpanDataQueue.js:78 totalContainedTime = 10.843999999999978 |
@phet-steele - Color me skeptical on the 300 FPS bit. That's 5x the nominal rate of 60 FPS, and the data you collected does not seem to indicate that rate. The profiler may be running into the same problem that the sim is, which is that it was assuming a nominal frame rate of 60 FPS, and if it gets something different, things go south. The data collected by @phet-steele in the previous comment seems to indicate that his system is running at a frame rate of around 75 FPS. @phet-steele - Can you check the refresh rate of your display and see if that is the value and report back with whatever you find? That's part 1. Part 2 is to test with the current version of master and see if the problem has gone away. If it has, please also test that the behavior of the pressure gauge is the same or similar enough to the published version (you may have to test the previous version on a different machine if this problem prevents it from running it). And finally, check that the performance hasn't degraded significantly with this change. You'll have to use a built version to make an apples-to-apples comparison to the published sim. Feel free to contact me on Slack if you have any questions on this. I'll write up an explanation of the problem and solution once we verify that this works (or come up with something else if it doesn't). |
|
Can you keep me informed of whether a patch for this will go into the maintenance releases? |
|
@jonathanolson - It is very likely that a patch will go in for states-of-matter and states-of-matter-basics, but it dependent upon what we hear back from @phet-steele and when. If there is something specific that you'd like me to do differently versus the usual process for this, please let me know. |
Release branches are still locked down to due the maintenance process, so let me know when it's ready to move to the branch. |
My refresh rate is 300 hz and this site also says I am getting 300 fps 😁 https://www.testufo.com/refreshrate
Sure seems like the problem has gone away, and I don't see performance issues on the mac and win 10 I have either. |
Okay. I'd just never heard of a refresh rate that high, but looking around online I do see monitors that spec that rate, so I stand corrected. Based on the data that @phet-steele supplied above, we're not seeing animation frame requests that fast, so I'm curious about how high refresh rates and the standard browsers'
Cool, good to hear. I'll move forward with a maintenance release to fix the issue. In summary, the problem was that the class that I had created to track and average the pressure data over time used pre-allocated memory to try to maximize its performance. This pre-allocation step assumed a standard rate for |
|
@jonathanolson - I'm ready to do a maintenance release for this issue. How should we proceed? |
|
For the record, @kathy-phet told me that she and @KatieWoe discussed whether or not to add a step to the QA process to test faster frame rates and decided against doing so. @kathy-phet recommended that, for now, we add an item to the code review checklist to look for any code that assumes a certain frame rate. I added such an item in phetsims/phet-info@1f1c0af#diff-f89f7f6ce4981e08a13b7a7a07456365dc54b1d206f3ae36af5b82a4a5f72bb3. |
My guess is phetsims/molecule-shapes/issues/180 another one of this class of issue. |
|
@jonathanolson - This is ready to go into an RC on the current release branch. I think it would be best if I did it myself, since I can verify it more easily and know what to look for. Is it okay for me to pull this onto the current release branch for states-of-matter and states-of-matter-basics? Also, should I do the RC, or is there an upcoming one that you would make this a part of? |
|
@jbphet depends on the priority, phetsims/qa#656 is the current maintenance QA task (so it could be integrated and re-RC'ed, and I wouldn't deploy it until it's tested). If you're ready to patch and RC both of them now, go for it (I'll check with you before starting any RC deployments). |
|
This is being tested by QA at the moment in an RC, see phetsims/qa#656. If QA testing passes, we can close it. I'll unassign @jonathanolson, since it's pretty much up to me to follow up. |
|
Assigning @KatieWoe as an assignee so that she or another member of the QA team can update based on the testing of the current RC. |
|
@phet-steele said in phetsims/qa#656 (comment)
|
|
Excellent. Closing. |
This is in the published sim! (1.2.1) But also, the version of this sim we are asked to test in phetsims/qa/issues/649 (https://phet-dev.colorado.edu/html/states-of-matter/1.2.2-rc.2/phet/states-of-matter_en_phet.html) no longer "produces" any change in pressure when collapsing the lid. It happens in both phet and phet-io brands. At least the Studio, Mirror Inputs, and State wrappers are buggy, but I don't see why not all of them would be buggy. There are times where this bug seems inconsistent, and if the bug doesn't occur it may help to refresh the web page and try again.
Seen on Win 10 Chrome.
The text was updated successfully, but these errors were encountered: