search

phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
MIT License
8 stars 6 forks source link

Active wrapper: animation jumps when toggling Active between true and false #977

Closed Matthew-Moore240 closed 2 months ago

Matthew-Moore240 commented 3 months ago

Test Device

Windows 11 Desktop

Operating System

Windows 11 10.0.22635 build 22635

Browser

Firefox 127.0.0

Problem Description

During the active wrapper test, there seems to be some animation jumping when the sim is turned from inactive to active.

Steps to Reproduce

  1. Start the active wrapper
  2. Release one or more particles

  3. Turn the sim from active to inactive back to active

    Visuals

One particle https://github.com/phetsims/gas-properties/assets/48224214/8a1707b4-7ac4-4142-94db-11d61a981ebf

Lots of particles https://github.com/phetsims/gas-properties/assets/48224214/48d32ec3-6e26-449a-87e2-03dec8510f15

pixelzoom commented 3 months ago

@zepumph @samreid @KatieWoe This is the first I've heard of this wrapper being tested. Is this a wrapper that PhET is typically testing? (If not, why is it in the wrapper index?) Should I work on this issue?

samreid commented 3 months ago

Gas Properties is not doing anything wrong. When the active checkbox is checked, there is a large dt coming in from Joist. I saw 0.5 seconds in my testing, which corresponds to the default max set here:

https://github.com/phetsims/joist/blob/d9982c46593a41976a3a2e072e759fdf810f40b5/js/Screen.ts#L138-L139

Ideally we would not want toggling activeProperty true or false to trigger large dts. @zepumph what do you think about setting private lastStepTime = -1; back to -1 when the activeProperty is toggled so that when it resumes we just get a regular dt? Want to move this issue to joist?

UPDATE: How about also changing the default to null while we are here?

pixelzoom commented 3 months ago

Thanks @samreid. Thoughts on whether fixing this in joist is blocking? We're preparing to begin RC testing.

I'm also interested in hearing whether this wrapper is typically being tested by QA. If not, why it's included in the wrapper index. And what to do about the other PhET-iO sims (published and in QA) that undoubtedly have this problem.

samreid commented 3 months ago

Here is how that test is documented in the qa-book: https://github.com/phetsims/qa/blob/f1a0990aed77789ff7652c8439c2999f0c2c1da4/documentation/qa-book.md#test-17-active-wrapper-test

This affects projectile data lab's recent release. Not sure why the problem wasn't identified there.

Since this affects other simulations, I recommend this should not block the upcoming publication for gas properties. If we decide to fix it, we can fix it for everything. I don't see that a maintenance release for this would be worth the effort at the moment, but likewise it is difficult to see how a client could use the isActiveProperty in an animating sim while we have this bug. Let's chat with @zepumph

pixelzoom commented 3 months ago

I spoke with @Nancy-Salpepi during our standup. She confirmed that the Active wrapper is included in QA testing. And she said that the Active wrapper has behaved like this since she started with PhET.

So... I'm going to transfer this issues to joist, and ignore the issue for the purposes of Gas Properties 1.1 release.

zepumph commented 2 months ago

Thanks for the report. This is a very nice thing to fix, though I don't think it needs an MR. As @samreid noted, it will only ever provide a jump of that Screen.maxDT value (likely half a second). All usages of the maxDT option provide a value smaller than 1/2 a second, so this seems fine in the wild, and is likely only a blip in cases like LOL and phet-io activeProperty toggling.

Good to patch up on main though.

@samreid and I committed this together, and feel good to close.