SpanNode doesn't listen for ChartTransform changes

[pixelzoom]

Shouldn't SpanNode listener to ChartTransform changedEmitter, and update itself, like other bamboo components?

[pixelzoom]

FYI, there's no SpanNode in Fourier.

[pixelzoom]

I just realized that the measurement tools in Fourier will have much in common with SpanNode. They function like a SpanNode, but have a different look and can be dragged. So maybe after I've implemented them, I'll take a stab at SpanNode.

[pixelzoom]

SpanNode's width is currently specified in view coordinates:

  /**
   * @param {Node} scaleIndicatorTextNode
   * @param {number} viewWidth
   * @param {Object} [options]
   */
  constructor( scaleIndicatorTextNode, width, options )

Like other bamboo components, the constructor should take model coordinates and a ChartTransform:

  /**
   * @param {ChartTransform} chartTransform
   * @param {number} modelWidth
   * @param {Node} scaleIndicatorTextNode
   * @param {Object} [options]
   */
  constructor( scaleIndicatorTextNode, width, options )

The problem that I've run into is that SpanNode is being used in griddle's SeismographNode, where it's using an entirely different transform:

    const viewSpanWidth = gridTransformProperty.get().modelToViewDeltaX( this.majorVerticalLineSpacing );
    const spanNode = new SpanNode( spanLabelNode, viewSpanWidth, {
      left: this.chartPanel.left,
      top: this.chartPanel.bottom + 2
    } );

@samreid suggestions on how to resolve this? Should SeismographNode also create a ChartTransform?

[samreid]

I don't think griddle should use ChartTransform--that would amount to doing the same thing in 2 different ways since it already has an independent transform. Perhaps we should move SpanNode back to griddle as "GriddleSpanNode" and leave it view-based, and create a new SpanNode in bamboo that uses ChartTransform?

[pixelzoom]

OK, I'm going to copy SpanNode.js back to griddle. Then I'll convert bamboo/SpanNode.js to the pattern used by bamboo components.

[samreid]

Maybe just restore the historical version that is already in the griddle history?

[pixelzoom]

Sorry, I don't have time to restore griddle history. There were only a few commits before it was moved to griddle. And it's deprecated.

I've converted SpanNode to the bamboo pattern in my local copy. But I've run into a new problem. What SpanNode really cares about is whether the unit delta has changed in ChartTransform. But ChartTransform is very coarse - you get notified if anything changes. So (for example) in CCKCChartNode where the Voltage chart scrolls, I'm seeing SpanNode update continuously as the chart scrolls.

I guess I can keep track of the model range, only update when that changes, and ignore other notification from ChartTransform. But that's still going to result in lots of calls to SpanNode.update in CCK:AC, none of which are needed.

@samreid thoughts?

[samreid]

SpanNode could keep track of its last value and guard like:

if (spanNodeWidth!===this.spanNodeWidth){...}

?

Or what would it look like to have a ChartTransform.horizontalDeltaChangedEmitter?

[pixelzoom]

In the above commits, I changed SpanNode to a model-based API, like other bamboo components. I also added a demo to DemoLinePlot.js.

There are lots of TODOs in the code (with this issue noted), most related to the fact that the implementation was (and still is) specific to the x axis -- there is no support for a SpanNode on the y axis. I don't have time to address these TODOs, so I'll leave it to the developer who needs support for y-axis spans.

To address the issue of the ChartTransform changing for a scrolling chart, I short-circuited the update method like this:

  update() {

    const viewWidth = this.chartTransform.modelToViewDelta( this.axisOrientation, this.delta );

    //TODO https://github.com/phetsims/bamboo/issues/19 ChartTransform notifies if anything changes, how to know when to update?
    // If the view width changes a 'noticeable amount', then update.
    if ( Math.abs( viewWidth - this.viewWidth ) > 0.25 ) {
...
    }
}

I'm not happy with this workaround. We want to update only when the length of the model range has changed. But in CCK:AC Voltage chart (for example), modelXRange is frequently changing by a very small amount, probably due to floating-point precision. Unfortunately I can't devote more time to working on this in the near future.

Over to @samreid to review. Make sure that this is "OK for now", and that the additional SpanNode notifications are not causing problems in CCK:AC.

[samreid]

The deltas are indeed caused by floating point errors:

35.29411764705878
35.29411764705884
35.29411764705878
35.29411764705884
35.29411764705878
35.29411764705884
35.29411764705878
35.29411764705884

The guard is working well in CCK for now. @jonathanolson how do we typically deal with problems like this? Do other scenery nodes have guards or other preventions against this kind of floating point fluctuation?

[jonathanolson]

Scenery Node's bounds do have a notification guard, currently set at:

const notificationThreshold = 1e-13;

in Node's validateBounds.

If the change in bounds is below the threshold, it won't notify. Does that work here?

[samreid]

Just a quick note for when I return to this issue:

35.29411764705884 - 35.29411764705878 = 5.684341886080802e-14

[samreid]

Since this guard is the same one we rely on in scenery, it seems acceptable to me. I think the cutoff of 0.25 should be reduced to 1E-13 or so, since the span node may not be in pixels (may be transformed). Normally I would have suggested Number.EPSILON but that is 2.220446049250313e-16 and we would like a number greater than 5.684341886080802e-14 at least for CCK's case.

@pixelzoom what do you think?

[pixelzoom]

Scenery's threshold (1e-13) seems like a fine default. Should ChartTransform have an option that allows you to change that default, in case it doesn't work for some specific transform?

[samreid]

Scenery doesn't have a facility for changing the threshold, perhaps this doesn't need one either. I changed to 1e-13 in the commit. Also, I noticed there are other TODOs in the code referring to this issue.

@pixelzoom can we open another issue for the other SpanNode TODOs, such as making it work vertically, and make this one ready for review?

[pixelzoom]

Since we're dealing with view coordinates at this point:

if ( Math.abs( viewWidth - this.viewWidth ) > notificationThreshold ) {

... it seems reasonable to use 1e-13.

Other TODOs have been moved to #21 and #22.

Closing.