phetsims / natural-selection

"Natural Selection" is an educational simulation in HTML5, by PhET Interactive Simulations
GNU General Public License v3.0
3 stars 7 forks source link

Population panel shows incorrect checkboxes with ?stringTest #269

Closed DevonQui closed 3 years ago

DevonQui commented 3 years ago

Test device MacBook Pro

Operating System 11.2.3

Browser Safari

Problem description This is for https://github.com/phetsims/QA/issues/641. I was doing the "rtl" stringTest and noticed that the allele column for the Lab section (6 alleles) was being displayed in the Intro section (normally 2 alleles) of the simulation. I'm not sure if it'll affect the published version, I just wanted to be sure though.

Steps to reproduce

  1. Add the string query "?stringTest = rtl" to the end of the URL of the sim
  2. Click onto the Intro section of the sim and notice the allele column matches that of the Lab section when they should two different things.

Visuals

Screen Shot 2021-04-19 at 1 39 14 PM Screen Shot 2021-04-20 at 10 22 17 AM

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Natural Selection‬ URL: https://phet-dev.colorado.edu/html/natural-selection/1.3.0-rc.1/phet/natural-selection_all_phet.html Version: 1.3.0-rc.1 2021-04-13 21:06:47 UTC Features missing: touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0.3 Safari/605.1.15 Language: en-us Window: 1440x792 Pixel Ratio: 2/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 (1.0) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 15 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 8192x8192 OES_texture_float: true Dependencies JSON: {}

pixelzoom commented 3 years ago

Good catch @DevonQui.

I see this problem with ANY value for stringTest. So I'm going to adjust the title of this issue accordingly.

More details:

pixelzoom commented 3 years ago

Example...

Here's what the Population control panel show look like in the Intro screen:

screenshot_950

Here's what it looks like with stringTest=X:

screenshot_952
pixelzoom commented 3 years ago

This problem does NOT occur in the published version, 1.1.1.

screenshot_954
pixelzoom commented 3 years ago

This problem DOES occur in the published brand=phet-io version, 1.2.0:

screenshot_955
pixelzoom commented 3 years ago

This problem was introduced sometime between 1.2.0-dev.3 (9/2/2020) and 1.2.0-dev.4 (9/15/2020). It's present in all 1.2 RC versions, starting with 1.2.0-rc.1 (9/17/2020).

Compare: https://phet-dev.colorado.edu/html/natural-selection/1.2.0-dev.3/phet/natural-selection_en_phet.html?stringTest=X https://phet-dev.colorado.edu/html/natural-selection/1.2.0-dev.4/phet/natural-selection_en_phet.html?stringTest=X

Since it was introduced in a dev version, the regression was originally introduced in master, not patched into an RC branch. And since it's in master, it's therefore also in the 1.3 branch.

pixelzoom commented 3 years ago

I suspect the problem was introduced in https://github.com/phetsims/natural-selection/commit/bd5d258f8bc07cc4aabcdecd87eca8b71d907977, and maybe https://github.com/phetsims/natural-selection/commit/6eff202ced8a9ae38842fdfca1e1bf6bb2d3dadb.

Here's my hypothesis on what's happening... Some string related to alleles is being changed by stringTest, then compared to some string that's NOT being changed by stringTest. And that comparison is resulting in alleles being made visibile when they should not be visible.

Note that this problem has 0% chance of occurring in real-world usage, it occurs only with stringTest when all strings have the same value. But I think it's still worth addressing.

pixelzoom commented 3 years ago

PopulationPanel setGeneVisible is where the visibility of the checkboxes is set.

  /**
   * Sets visibility of the UI components related to a specific gene.
   * @param {Gene} gene
   * @param {boolean} visible
   * @public
   */
  setGeneVisible( gene, visible ) {
    assert && assert( gene instanceof Gene, 'invalid gene' );
    assert && assert( typeof visible === 'boolean', 'invalid visible' );

    // Checkbox for the normal allele
    const normalCheckbox = _.find( this.checkboxes, checkbox => ( checkbox.alleleName === gene.normalAllele.name ) );
    assert && assert( normalCheckbox, `normalCheckbox not found for ${gene.normalAllele.name} allele` );
    normalCheckbox.visible = visible;

    // Checkbox for the mutant allele
    const mutantCheckbox = _.find( this.checkboxes, checkbox => ( checkbox.alleleName === gene.mutantAllele.name ) );
    assert && assert( normalCheckbox, `mutantCheckbox not found for ${gene.mutantAllele.name} allele` );
    mutantCheckbox.visible = visible;
  }

Adding this to the beginning of setGeneVisible:

    console.log( `-------------------------` );
    console.log( `gene.normalAllele.name=${gene.normalAllele.name}` );
    console.log( `gene.mutantAllele.name=${gene.mutantAllele.name}` );
    this.checkboxes.forEach( checkbox => console.log( `checkbox.alleleName=${checkbox.alleleName}` ) );

With stringTest=X, I see:

gene.normalAllele.name=X
gene.mutantAllele.name=X
7 checkbox.alleleName=X

So all checkboxes match the allele name because all strings are "X", therefore all checkboxes are visible. Though I'm not sure why the "Total" checkbox is not visible - probably because it doesn't have an associated allele.

pixelzoom commented 3 years ago

Fixed in the master and 1.3 branches in the above commits.

The fix for this was relatively straightforward, but time consuming. Instead of comparing allele names, we are now comparing Allele instances -- which is what is none in other panels.

Labeling for verification in the next RC. Steps to verify:

(1) Run the sim with no query parameters. Verify that the Population control panel looks correct for the Intro and Lab screens. Like this:

Intro screen: screenshot_221

Lab screen: screenshot_222

(2) Run the sim with stringTest=X or stringTest=rtl (or any value). Verify that the Population control panel looks correct for the Intro and Lab screens. Pay particular attention to the number of checkboxes, colors, and line style (solid vs dashed) for each checkbox. With stringTest=X:

Intro screen: screenshot_223

Lab screen: screenshot_224

KatieWoe commented 3 years ago

Looks good in rc.2 on Win 10 Chrome. For both PhET and PhET-iO brands