phetsims / fourier-making-waves

"Fourier: Making Waves" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 3 forks source link

Wave Game: hidden amplitude controls are not being zeroed out #215

Closed DevonQui closed 2 years ago

DevonQui commented 2 years ago

Test device MacBook Pro M1

Operating System 12.1

Browser Safari 15.2

Problem description this is for https://github.com/phetsims/fourier-making-waves I was testing adding and removing harmonics for the Wave Game levels and noticed that the wave sum visual isn't adjusted when you remove higher harmonics.

Steps to reproduce

  1. Open Wave Game section of simulation
  2. Choose level 1
  3. Increase amplitude controls to any amount greater than 1
  4. Adjust all of the amplitudes to be non-zero
  5. Notice that reducing the amplitude controls of the harmonics doesn't change the overall wave sum

Visuals https://drive.google.com/file/d/1d9zBOPmt8ZnnR3lYyWQb8_LIX9drrNsG/view?usp=sharing

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Fourier: Making Waves‬ URL: https://phet-dev.colorado.edu/html/fourier-making-waves/1.0.0-rc.1/phet/fourier-making-waves_all_phet.html Version: 1.0.0-rc.1 2021-09-28 15:44:23 UTC Features missing: touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/15.2 Safari/605.1.15 Language: en-US Window: 1512x865 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: {}

arouinfar commented 2 years ago

Nice find @DevonQui. This is definitely a bug.

Steps to reproduce

  1. Open any game level
  2. Set an irrelevant harmonic to a non-zero value (A2 in the example below). image
  3. Reduce the number of amplitude controls until the harmonic from (2) is removed. The now-hidden harmonic (A2) still has a non-zero value that contributes to the sum. It's also still plotted in the Harmonics graph. image

@pixelzoom we should be zeroing out the amplitude when the control is hidden.

pixelzoom commented 2 years ago

Fixed in master and 1.0 branches in the above commits.

@DevonQui or @arouinfar please verify in 1.1.0-dev.1 or master. Then assign back to me and I'll publish a maintenance release.

DevonQui commented 2 years ago

I was able to reproduce the issue on master and the PhET website but not https://phet-dev.colorado.edu/html/fourier-making-waves/1.1.0-dev.1/phet/fourier-making-waves_en_phet.html. Tested on MacBook Pro M1.

pixelzoom commented 2 years ago

@DevonQui 1.1.0-dev.1 was published from master, so I'm confused about how you could have reproduced this from master. Did you by any chance use phettest to test master? If so, I suspect that phettest had not yet pulled everything, and you didn't have master. Please try again, and make sure that "up-to-date" is shown for all repositories. Press the "pull" button for any that are not "up-to-date".

EDIT: I just tested Fourier using phettest. I had to press the "pull" button for a couple of repos. And I could no longer reproduce this problem.

pixelzoom commented 2 years ago

I tried to public 1.0.4-rc.1 for testing, so that we could move forward on this. But I'm now blocked by a build failure, see https://github.com/phetsims/perennial/issues/256#issuecomment-1004209892.

arouinfar commented 2 years ago

@pixelzoom I verified that the behavior is correct in master and 1.1.0-dev.1. Once https://github.com/phetsims/perennial/issues/256 is addressed, please proceed.

pixelzoom commented 2 years ago

phetsims/perennial#256 is still not addressed. I worked around it by blasting chipper's node_modules:

% cd chipper
% rm -rf node_modules
% npm install

I deployed 1.0.4-rc.2, then verified the fixes for #215 and #216:

% cd fourier-making-waves
% grunt rc --brands=phet --branch=1.0

Then I published 1.0.4, verified the fixes for #215 and #216, and spot-checked a few translated versions:

% cd fourier-making-waves
% grunt production --brands=phet --branch=1.0

Closing.