How to improve RulerNode usage

[jonathanolson]

Desired general appearance is a ruler from 0 to 200cm where every multiple of 20 cm is labeled (excluding 0 and 200cm).

It looks like MLL is giving empty-string tick labels for 0, 200, and every 20*n+10 (multiple of 10 that isn't a multiple of 20):

    // create tick labels
    var tickLabel;
    var rulerTicks = [ '' ]; // zero tick is not labeled
    for ( var currentTick = TICK_INTERVAL; currentTick < ruler.length * 100; currentTick += TICK_INTERVAL ) {
      tickLabel = currentTick % (2 * TICK_INTERVAL) ? '' : currentTick.toString();
      rulerTicks.push( tickLabel );
    }
    rulerTicks.push( '' );

    // define ruler params in pixels
    var rulerWidth = modelViewTransform.modelToViewDeltaX( ruler.length );
    var tickWidth = rulerWidth / (rulerTicks.length - 1);

    RulerNode.call( this, rulerWidth, RULER_HEIGHT, tickWidth, rulerTicks, rulerUnitsString, {
      backgroundFill: 'rgb( 237, 225, 121 )',
      cursor: 'pointer',
      insetsWidth: 0,
      majorTickFont: FONT,
      majorTickHeight: 12,
      minorTickHeight: 6,
      unitsFont: FONT,
      unitsMajorTickIndex: rulerTicks.length - 3,
      minorTicksPerMajorTick: 4,
      tickMarksOnBottom: false
    } );

I was wondering if @pixelzoom could provide a brief recommendation for how this should be done, and if using empty tick labels is the correct approach (hopefully not?). If it is correct, I'd prefer to not add the Text nodes.

[pixelzoom]

The pendulum-lab ruler has no insets, so the min and max tick labels are not needed. But otherwise, RulerNode assumes that there's a label for every major tick:

assert && assert( Math.floor( rulerWidth / majorTickWidth ) + 1 === majorTickLabels.length ); // do we have enough major tick labels?

An unfortunate implementation, I agree - it was ported from the Java implementation (piccolophet.RulerNode). A better API would specify a mapping between tick values and labels, ala HSlider.

Options:

(1) Rewrite RulerNode, a lot of work, and 11 require statements in sims.

(2) Modify RulerNode so that empty strings (and null values?) are ignored in @param majorTickLabels. If you take the approach, you'll need to verify that options.unitsMajorTickIndex corresponds to a non-empty (and non-null) tick label.

[jonathanolson]

I followed the initial path of (2) for refactoring, but I can't guarantee unitsMajorTickIndex is actually a valid (non-negative integer) index. The else-if clause on line 135 (checking if majorTickIndex > options.unitsMajorTickIndex ) will only be triggered if unitsMajorTickIndex is negative OR a non-integer value (majorTickIndex starts at 0 and increases by 1, and since equality is checked first, we can guarantee the else-if won't fire if it's a non-negative integer).

Either it's (a) dead code and can be removed, or (b) fractional/negative indices are supported, I just made the change to prevent adding the empty labels as children.

I'll move any cleanup/optimization work to a scenery-phet issue (like not creating the empty labels we toss, so that we can actually pass in null).

[jonathanolson]

Closing here, see https://github.com/phetsims/scenery-phet/issues/213 for further RulerNode improvement discussion.

[pixelzoom]

Reopening.

In https://github.com/phetsims/pendulum-lab/issues/116#issuecomment-169102438, @jonathanolson said:

Either it's (a) dead code and can be removed, or (b) fractional/negative indices are supported, I just made the change to prevent adding the empty labels as children.

Neither is the case. The else if block at line 135 is hit by fluid-pressure-and-flow, which creates a RulerNode with unitsMajorTickIndex: 1 and has integer tick marks.

[jonathanolson]

My bad, looking into it.

[pixelzoom]

Not something you necessarily have to resolve for this issue. When I said in https://github.com/phetsims/pendulum-lab/issues/116#issuecomment-166996324:

you'll need to verify that `options.unitsMajorTickIndex corresponds to a non-empty (and non-null) tick label.

I was assuming that you wouldn't create or layout the Text node associated with the empty tick. But you're still doing that (line 106), then not adding it at line 117. That's fine if it simplifies the code.

[pixelzoom]

... and I don't completely understand the else if expression at line 135. It was added by @jbphet in @dd76f610320b87bff51e909f4722ea0b9ab53eda and is associated with https://github.com/phetsims/scenery-phet/issues/8. You might want to discuss with him.

[jonathanolson]

Creating the Text node isn't great, but it only slows down the creation of the ruler, not its display. It does simplify the code and changes for now.

Will plan to handle the cleanup and refactoring in https://github.com/phetsims/scenery-phet/issues/213, since it's related to how we set the maxWidth of the units label to fit within ticks.