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

Rapidly changing the source separation wrecks performance #316

Closed
samreid opened this issue Jan 15, 2019 · 18 comments
Closed

Rapidly changing the source separation wrecks performance #316

samreid opened this issue Jan 15, 2019 · 18 comments

Comments

@samreid
Copy link
Member

samreid commented Jan 15, 2019

Even before increasing the lattice size, it is possible to make the simulation unusably slow by:

  1. Interference
  2. Turn sources on
  3. Drag separation slider back and forth many times

Discovered during #315

@samreid samreid self-assigned this Jan 15, 2019
@samreid
Copy link
Member Author

samreid commented Jan 15, 2019

You can simulate the same problem by clicking the wave source on and off many many times. There is a bug in TemporalMask.prune.

@samreid
Copy link
Member Author

samreid commented Jan 15, 2019

The aforementioned fix corrects a memory leak and allows TemporalMask entries to be pruned when they have left the window. However, rapidly dragging the source separation slider or pressing the on/off button can still swamp the TemporalMask with deltas. More than 20 or so kills performance on my Mac Chrome. The performance is restored in 15 seconds or so when the wave has moved out of the visible region.

@arouinfar and @ariel-phet how do you want to address this problem? Here are some possibilities:

  • Clear the waves when the source separation changes (may also eliminate spurious artifacts, but makes it less user friendly when looking at a changing interference pattern in the wave area or on the screen since the user has to wait for re-propagation).
  • Only keep the last 5 or so deltas (when on/off or source separation changes). If you rapidly turn the wave on and off, the more distant waves will disappear.
  • Limit how frequently the user can turn the wave on and off.
  • Only apply the masking algorithm every Nth frame.
  • Come up with a different algorithm for masking.
  • Some combination of the preceding ideas
  • Your idea here

This issue blocks next round of dev testing, please let me know your thoughts or reach out to me on Slack/Zoom to discuss further.

UPDATE: I added some bullet points.

@samreid
Copy link
Member Author

samreid commented Jan 15, 2019

Also, @arouinfar I wonder if this is what you observed when the performance dropped on the Chromebook you tested.

samreid added a commit that referenced this issue Jan 15, 2019
@ariel-phet
Copy link

ariel-phet commented Jan 15, 2019

@samreid @arouinfar

This is just not a pedagogically productive action at all (rapid changing of separation). It seems perhaps keeping only the last 5 or so deltas is a good solution. Basically, I am fine with a solution that allows performance to remain, but if things look a bit odd with you moving the separation slider back and forth rapidly, I am fine with that, since it is not a productive interaction (and it should be a rare interaction).

@ariel-phet ariel-phet removed their assignment Jan 15, 2019
@arouinfar
Copy link
Contributor

@samreid I'm fairly certain this what I was seeing yesterday on the Chromebook. Perhaps I wasn't always changing separation rapidly or often enough to see it happen consistently.

I agree with @ariel-phet. Rapidly scrubbing the separation slider serves no pedagogical purpose, and would be a rare interaction. Keeping the last 5ish deltas also sounds reasonable to me. I do wonder what would happen in the lattice, though. Would part of the lattice clear itself when the 6th delta occurs?

@arouinfar arouinfar removed their assignment Jan 15, 2019
@samreid
Copy link
Member Author

samreid commented Jan 15, 2019

Yes, the temporal mask would "forget" the parts furthest from the oscillator. You can see it in practice on phettest: http://phettest.colorado.edu/wave-interference/wave-interference_en.html?ea&brand=phet

Try the following tests:

Test 1

  1. On the Waves screen
  2. select the Speaker scene
  3. Click the green speaker button 6 times before the wave reaches the right edge

Test 2

  1. On the Interference screen
  2. Select the Speaker scene
  3. Turn on the sources
  4. Drag the separation slider (so that the value changes at least 5 times).

Please let me know your thoughts.

@samreid samreid assigned ariel-phet and arouinfar and unassigned samreid Jan 15, 2019
@samreid
Copy link
Member Author

samreid commented Jan 15, 2019

Also, the proposed "keep only 5 deltas" solution helps performance, but isn't entirely consistent--it clears the temporal mask without clearing the lattice so you can end up with situations where the light screen/chart is lit up without a wave there:

image

@samreid
Copy link
Member Author

samreid commented Jan 15, 2019

@arouinfar and I decided to try 10 deltas. Also to address the artifacts:

(1) When the separation slider knob starts to move, clear the wave area and intensity samples and keep them clear until the slider knob is released at the new frequency.
(2) When the tweaker button is pressed, clear the wave area and intensity samples and keep them clear until the tweaker button is released.

@arouinfar and I agreed that @ariel-phet and probably also @kathy-phet would need to sign off on this before I begin investigation. Please see #316 (comment) for instructions how to replicate some of the odd behavior.

@samreid samreid assigned kathy-phet and unassigned arouinfar Jan 15, 2019
samreid added a commit that referenced this issue Jan 15, 2019
@ariel-phet
Copy link

@samreid I think given the nature of this issue (rare use case), dealing with the artifacts can be deferred until after 1.0 is published.

@ariel-phet ariel-phet assigned samreid and unassigned ariel-phet and kathy-phet Jan 15, 2019
@arouinfar
Copy link
Contributor

@ariel-phet while rapidly changing the separation and/or on/off state is a rare use case, the artifacts created by clearing the wave after 5 (or 10) deltas are not at all rare. Each increment the slider thumb snaps to (100 nm for light, or 1/35 of the slider range) would correspond to a single delta. A single adjustment of the separation from a high to low value would correspond to many deltas, which creates a ton of noise in the lattice. The noise will eventually die down, but only after several seconds of sitting around and waiting.

You might want to take a look at master for yourself, but here's a gif. (Dimensions are small because of GitHub limit on file size.)
noise

@arouinfar arouinfar assigned ariel-phet and unassigned samreid Jan 15, 2019
@samreid
Copy link
Member Author

samreid commented Jan 16, 2019

The above commits "mute" the sources while the separation is being changed. This was easy thanks to the pre-existing support in NumberControl, thanks @pixelzoom!

Setting the amplitude to 0 in each frame created odd artifacts at the wave sources, and would have required more complex logic to suppress deltas from the temporal mask. Instead I tried the simpler solution of just skipping setting point source values altogether. There is still an artifact when the wave value immediately stops oscillating, but this solves the problem when continuously changing the source separation, and also prevents those values from entering the list of deltas.

@arouinfar can you please test in master?

@samreid samreid removed their assignment Jan 16, 2019
samreid added a commit that referenced this issue Jan 16, 2019
samreid added a commit that referenced this issue Jan 16, 2019
@samreid
Copy link
Member Author

samreid commented Jan 16, 2019

A few fixes for water drops in the preceding commits.

@arouinfar
Copy link
Contributor

arouinfar commented Jan 17, 2019

@samreid I think things are generally looking a bit better, though I do find the artifacts in the lattice to be pretty distracting.

I'm not so sure the sim is behaving as proposed in #316 (comment). Here, the only adjustment I made was a smooth slide from high to low separation. The lattice unexpectedly cleared, and the lattice ended up pretty noisy and took a bit of time to settle.

noise4

I'm now leaning towards the first option proposed by @samreid in #316 (comment):

Clear the waves when the source separation changes (may also eliminate spurious artifacts, but makes it less user friendly when looking at a changing interference pattern in the wave area or on the screen since the user has to wait for re-propagation).

I think this would be somewhat similar to how we handle frequency changes for light, or how we clear the right side of the lattice when the barrier is moved.

@samreid
Copy link
Member Author

samreid commented Jan 17, 2019

I pushed a commit that suppresses waves while the separation is changing. @arouinfar can you please review on master? On second thought, I'll publish a dev version:

https://phet-dev.colorado.edu/html/wave-interference/1.0.0-dev.64/phet/wave-interference_en_phet.html

Noted: Sound particles "freeze" while dragging the source separation slider.

UPDATE: I'd like to fix the particles freezing, will commit shortly. Please hold off testing.

@samreid samreid assigned arouinfar and samreid and unassigned samreid and arouinfar Jan 17, 2019
@samreid
Copy link
Member Author

samreid commented Jan 17, 2019

This version clears the sound particles. Ready for testing
https://phet-dev.colorado.edu/html/wave-interference/1.0.0-dev.65/phet/wave-interference_en_phet.html

@samreid samreid assigned arouinfar and unassigned samreid Jan 17, 2019
@arouinfar
Copy link
Contributor

@samreid this is looking much cleaner to me. I like it!

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

No branches or pull requests

4 participants