We need a FrequencySlider

[samreid]

At the Wave Interference design meeting, we concluded we need to be able to reverse the WavelengthSlider, so that the high frequencies are shown to the right instead of the left (that is, we are using it as a "Frequency" slider instead of a Wavelength slider.)

My initial thought for the API would be to add an option like:

increasingWavelength: true

or maybe something like:

blueOnTheLeft: true

However, I realized that if I swapped the min and max like so:

    var lightFrequencySlider = new WavelengthSlider( model.wavelengthProperty, {
      minWavelength: VisibleColor.MAX_WAVELENGTH,
      maxWavelength: VisibleColor.MIN_WAVELENGTH,

and disabled the corresponding assertion statements that checked that min < max, then it rendered properly (though the input listener was broken):

However, if we stick with that pattern, then min and max may not be the best prefixes (since max would be greater than min).

@pixelzoom can you please recommend an API for this before I proceed?

[samreid]

However, if we stick with that pattern, then min and max may not be the best prefixes (since max would be greater than min).

For instance, we could go with leftWavelength and rightWavelength.

[samreid]

It also crossed my mind that we might be able to flip the slider with a scenery node scale (-1,1), but is that a hack?

[samreid]

Indeed, putting scale: new Vector2( -1, 1 ) in the options seems to work properly (including input listeners), maybe we should go with that?

[samreid]

I renamed the issue to "We need a frequency slider" because if you only reverse the wavelengths, you do not have the right "spacing" between frequency points. That is, we will want the transform to be linear in frequency (not -1/Hz), which is different.

The WavelengthSlider currently has an api: minWavelength, maxWavelength. We could still use those values for a frequency slider if you compute minWavelength = speed of light / maxFrequency etc.

I'll hold off until I can discuss the API with @pixelzoom.

[pixelzoom]

It sounds to me like there are enough differences that we may want to create a new FrequencySlider, rather than trying to coerce WavelengthSlider into handling both wavelength and frequency. Then see what bits WavelengthSlider and FrequencySlider share, and factor out a base type.

[samreid]

I made working copy changes to create SpectrumSlider with subclasses WavelengthSlider and FrequencySlider. I tested Beer's Law Lab (which uses WavelengthSlider) is working OK.

I added a scenery-phet test for the FrequencySlider here: http://localhost/scenery-phet/scenery-phet_en.html?screens=2&component=FrequencySlider

I confirmed the WavelengthSlider in the scenery-phet demo still works OK.

• I couldn't figure out why the value was being clamped in WavelengthSlider before--I removed it so SpectrumSlider is more general, and can handle default values between 0-1.
• I need to rename SpectrumNode => WavelengthNode
• I need to rename AbstractSpectrumNode => SpectrumNode
• I followed the same pattern for strings as was used for the wavelength, even though it is an older pattern
• For default text display for frequency, I chose to use Tera Hz (THz)

Initial commit forthcoming.

[samreid]

After initial commit, I'll proceed with the renamings mentioned above.

[samreid]

I renamed SpectrumNode => WavelengthSpectrumNode and confirmed it looks identical in Molecules and Light. Committing.

[pixelzoom]

I won't have time to look at this until next week. But hopefully none of these changes make it difficult to add UV and IR to WavelengthSlider, as is required for #211 (needed by MOTHA).

[samreid]

But hopefully none of these changes make it difficult to add UV and IR to WavelengthSlider, as is required for #211 (needed by MOTHA).

I introduced a new type SpectrumSlider which takes a min value, max value and color mapping function. I expect it would make it easier to add UV and IR. For instance, in the visible range, you would return VisibleColor.wavelengthToColor(), and in the UV range you could return Color.gray. You could even use that function to add the vertical lines, if they are still desired. The trick will be adding the text in the track--we may wish to add getter for the track node to make that easy, or a add/removeTrackChild method on SpectrumSlider.

[samreid]

I finished creating FrequencySlider, kept the same interface for WavelengthSlider, and renamed SpectrumNode => WavelengthSpectrumNode. I recommend the changes be reviewed since (a) they are used in several sims (b) they will affect how we proceed with #211 and (c) I have some questions for the reviewer:

• I couldn't figure out why the value was being clamped in WavelengthSlider before--I removed it so SpectrumSlider is more general, and can handle default values between 0-1. If necessary, we could add an option for flooring, but I don't know if that's necessary/valuable.
• I followed the same pattern for strings as was used for the wavelength, even though it is an older pattern, is that OK or should we use the new template string pattern (and mismatch with the wavelength slider)?
• For default text display for frequency, I chose to use Tera Hz (THz), is that OK?

More notes for the reviewer:

• You can test WavelengthSlider, FrequencySlider and SpectrumSlider at http://localhost/scenery-phet/scenery-phet_en.html?screens=2

@ariel-phet can you please prioritize/schedule/delegate?

[pixelzoom]

@samreid said:

renamed SpectrumNode => WavelengthSpectrumNode

I see both SpectrumNode and WavelengthSpectrumNode in scenery-phet, both with commits on 4/13/2018. Please clarify.

[pixelzoom]

I see these assertions in FrequencySlider and WavelengthSlider:

    assert && assert( typeof options.minValue === 'undefined', 'minValue is supplied by FrequencySlider' );
    assert && assert( typeof options.maxValue === 'undefined', 'maxValue is supplied by FrequencySlider' );

Why not this?

    assert && assert( options.minValue === undefined, 'minValue is supplied by FrequencySlider' );
    assert && assert( options.maxValue === undefined, 'maxValue is supplied by FrequencySlider' );

[pixelzoom]

@samreid asked:

• I couldn't figure out why the value was being clamped in WavelengthSlider before--I removed it so SpectrumSlider is more general, and can handle default values between 0-1. If necessary, we could add an option for flooring, but I don't know if that's necessary/valuable.

I have no recollection. If you remove it, test thoroughly.

• I followed the same pattern for strings as was used for the wavelength, even though it is an older pattern, is that OK or should we use the new template string pattern (and mismatch with the wavelength slider)?

New code should use the new pattern. FrequencySlider is new code.

• For default text display for frequency, I chose to use Tera Hz (THz), is that OK?

No idea. Probably a question for designers.

[pixelzoom]

Playing with FrequencySlider in the scenery-phet demo... the arrow buttons don't work.

[pixelzoom]

Review completed, see comments above.

Tested the sims I'm responsible for that use WavelengthSlider and I don't see any bugs introduced.

[samreid]

I see both SpectrumNode and WavelengthSpectrumNode in scenery-phet, both with commits on 4/13/2018. Please clarify.

From the docs:

/**
 * SpectrumNode displays a spectrum from one value to another.  The displayed colors are computed by a
 * required valueToColor function.
 *
 * @author Chris Malley (PixelZoom, Inc.)
 * @author Sam Reid (PhET Interactive Simulations)
 */

/**
 * WavelengthSpectrumNode displays a rectangle of the visible spectrum.
 *
 * @author Chris Malley (PixelZoom, Inc.)
 * @author Sam Reid (PhET Interactive Simulations)
 */

WavelengthSpectrumNode extends SpectrumNode.

[samreid]

From #371 (comment)

The question is about whether to use x.y===undefined or typeof x.y==='undefined'. They seem equivalent, I'll change to the former.

[samreid]

Remaining work:

• use x.y===undefined
• Playing with FrequencySlider in the scenery-phet demo... the arrow buttons don't work.
• I have no recollection. If you remove it, test thoroughly.

  Fuzz test seems OK, and I haven't seen any problems during my testing (yet). Hopefully QA will find a problem if there is one.

• New code should use the new pattern. FrequencySlider is new code.

[samreid]

I addressed the issues raised during review, @pixelzoom anything else to do before closing?