Polish discontinuity points

[pixelzoom]

From 3/6/2023 design meeting to review interview results (@amanda-phet @catherinecarter @kathy-phet @veillette @pixelzoom)

Plotted discontinuities sometimes look fully or paritially filled with the same color as the curve. Start by filling them with the chart's background color. Possibly use a smaller lineWidth value.

[pixelzoom]

In CurveNode.ts, I made this change so that I could see what's going on with the discontinuity points:

      discontinuousPointsScatterPlotOptions: {
-       fill: null,
+       fill: 'yellow',
+       radius: 6
        lineWidth: 2
      },

Here's what I saw for the derivative on the Derivative screen:

This bit of horizontal line is what's making the smallet points appear to be partially filled. The problem seems to be in bamboo's ScatterPlot.ts. The points are drawn as part of a single path, and the circles are not being closed.

[pixelzoom]

This was indeed a bug in bamboo ScatterPlot, fixed in the above commit with @jonathanolson's assistance (thanks!)

[pixelzoom]

After fixing the problem in bamboo, I filled the discontinity points with the background color of the chart, and increased the radius of the points slightly (from 2 to 2.5). I did not change the lineWidth -- it's still 2.

Radius of 2 still looked a little crunchy to me:

Radius of 2.5 looks significantly better to me:

@amanda-phet please review, close if OK.

[amanda-phet]

Yay, thanks for finding that bug! The larger radius looks good to me.

[kathy-phet]

I like the larger radius too, thanks.

[amanda-phet]

Can you confirm that there is a fill? It still looks like you can "see through" them when I get layered discontinuities.

[pixelzoom]

This is symptomatic of how bamboo ScatterPlot renders points. It uses kite.Shape like this:

 public update(): void {
    const shape = new Shape();
    const length = this.dataSet.length;
    for ( let i = 0; i < length; i++ ) {

      // NaN or Infinite components draw nothing
      const dataPoint = this.dataSet[ i ];
      if ( dataPoint.isFinite() ) {
        const viewPoint = this.chartTransform.modelToViewPosition( dataPoint );
        shape.moveTo( viewPoint.x + this.radius, viewPoint.y ); // need to move to where circle actually starts
        shape.circle( viewPoint.x, viewPoint.y, this.radius );
      }
    }
    this.shape = shape.makeImmutable();
  }

Since it's a Shape, the lines are stroke, and then any interior regions are filled.

Here's an example where I've increased the point radius to 10, so we can what's going on clearly. Note that you can only "see through" the points where they overlap; you cannot see the axes through the points.

This seems like a general problem that @samreid may not have considered when he implemented ScatterPlot. Maybe @jonathanolson has some ideas. Assigning to them for input, high priority please.

[amanda-phet]

Thanks for investigating this! It's more than an aesthetic issue.. when the points overlap, it's possible for them to appear filled, especially on lower-resolution or smaller screens.

[samreid]

It's unclear what's being requested, how to reproduce the problem, what the problem is or how I can contribute. I think a zoom call would be a great, efficient way to proceed. I'm available on slack for scheduling. Or I would recommend empowering this subteam to make changes in bamboo as appropriate.

[pixelzoom]

It's unclear what's being requested, how to reproduce the problem, what the problem is or how I can contribute.

See https://github.com/phetsims/calculus-grapher/issues/256#issuecomment-1458912615. When ScatterPlot draws overlapping points, filling the points does not look correct. Rather than overlapping filled points, you see what's shown in https://github.com/phetsims/calculus-grapher/issues/256#issuecomment-1458912615. The code responsible for this is pasted into https://github.com/phetsims/calculus-grapher/issues/256#issuecomment-1458912615, and was written by @samreid. We're trying to figure out how to get the correct behavior when points overlap.

Or I would recommend empowering this subteam to make changes in bamboo as appropriate.

This subteam feels empowered to change bamboo - I've already done so once for this issue, see https://github.com/phetsims/bamboo/commit/15497c730f982db3886d8535426871fc713c8908. Now I'm at a loss for how to proceed. So I'm contacting @samreid (ScatterPlot author) and @jonathanolson (Shape author) for thoughts on how to solve this problem.

I'll also follow up with @samreid on Slack, to see if he still prefers to discuss synchronously on Zoom.

[pixelzoom]

@samreid and I discussed via Slack DM. Summary:

• Ask the design team if they're willing to live with this problem, or willing to use solid points for discontinuities.
• @samreid demonstrated that we could fix the behavior of ScatterPlot if we replaced Shape with a scenery.Circle instance for each point. But that's not likely scalable. So this is not going to be fixable in ScatterPlot as long as it uses Shape. Create a bamboo GitHub issue, note this limitation in a comment in ScatterPlot.ts (for the next developer who encounters it), and move on.
• Investigate writing CanvasScatterPlot. It's not clear whether we'll have the same problem there. I suspect that we might because @jonathanolson previously said (I think?) that Shape mimics Canvas behavior. It would be nice to have @jonathanolson's input before doing this.
• Noting that creating CanvasScatterPlot makes this problem worse: https://github.com/phetsims/bamboo/issues/46. Having 2 parallel sets of bamboo classes is a real problem. For example, I could not easily switch from LinePlot to CanvasLinePlot in calculus-grapher because the APIs are diffferent -- CanvasLinePlotOptions does not support lineWidth. It would be superior to have one set of bamboo classes, each with a renderer option, and have different internal implementations based on the value of renderer. This would ensure a consistent class and Options API.

[pixelzoom]

@amanda-phet This problem cannot be fixed in the existing bamboo implementation (ScatterPlot.ts). Unless we're willing to make compromises in Calculus Grapher, we'll need a new implementation to render discontinuty points (estimate: 3-6 hours)

The 2 possible compromises are:

(1) Live with the problem where points overlap. Estimate = 0 hours (2) Use solid points for discontinuties. Estimate = 10 minutes

Are either of these compromises acceptable? Or should I continue with working on a solution for "hollow" discontinuity points?

[jonathanolson]

Investigate writing CanvasScatterPlot. It's not clear whether we'll have the same problem there. I suspect that we might because @jonathanolson previously said (I think?) that Shape mimics Canvas behavior. It would be nice to have @jonathanolson's input before doing this.

Similar issue (you'd have to stroke/fill each circle independently in Canvas), the rendering should be pretty identical otherwise.

It might be possible to write bamboo to use sprites for points (if high performance is needed). WebGL would also be capable. Both of those options seem like a lot of work. For chart lines, WebGL would probably significantly increase performance without a huge increase in complexity.

[pixelzoom]

@jonathanolson said:

Similar issue (you'd have to stroke/fill each circle independently in Canvas), the rendering should be pretty identical otherwise.

I'm confused -- I can't tell if you're saying it's possible or not with Canvas. Couldn't I so something like this? That is, call context.beginPath for each point in the dataSet?

const radius = 3;
context.fillStyle = 'white';
context.strokeStyle = 'red';
for ( let i = 0; i < dataSet.length; i++ ) {
  const point = dataSet[ i ];
  context.beginPath();
  context.arc( point.x, point.y, radius, 0, 2 * Math.PI );
  context.fill();
  context.stroke();
}

[jonathanolson]

Couldn't I so something like this? That is, call context.beginPath for each point in the dataSet?

You could do it like that, yes. That will work. It's equivalent visually to creating a bunch of Circle nodes displaying in SVG (but that's essentially much more overhead).

So yes it's possible in Canvas, although it does have some overhead for having that many fill/stroke (if you are concerned about performance).

[pixelzoom]

@veillette says that his work on cusp points in #208 should result in fewer discontinuity points, and less overlapping of points. So I'm going to defer further work on this issue until #208 is done, so we can reevaluate.

[amanda-phet]

@amanda-phet This problem cannot be fixed in the existing bamboo implementation (ScatterPlot.ts). Unless we're willing to make compromises in Calculus Grapher, we'll need a new implementation to render discontinuty points (estimate: 3-6 hours)

The 2 possible compromises are:

(1) Live with the problem where points overlap. Estimate = 0 hours (2) Use solid points for discontinuties. Estimate = 10 minutes

Are either of these compromises acceptable? Or should I continue with working on a solution for "hollow" discontinuity points?

If option (3) is 3-6 hours of work to render these points, then option (1) is the only reasonable way to go, in my opinion. Option (2) isn't acceptable.

My recommendation is to go with option (1) and accept that sometimes they can overlap, but it shouldn't happen as frequently once #208 is worked out.

[pixelzoom]

@amanda-phet said:

... My recommendation is to go with option (1) and accept that sometimes they can overlap, but it shouldn't happen as frequently once https://github.com/phetsims/calculus-grapher/issues/208 is worked out.

On hold until https://github.com/phetsims/calculus-grapher/issues/208 is done, so we can evaluate how often points overlap.

[pixelzoom]

The deficiencies of ScatterPlot have been reported in https://github.com/phetsims/bamboo/issues/63.

[pixelzoom]

https://github.com/phetsims/calculus-grapher/issues/278 elimintate redundant work related to LinePlot, and #208 will be reducing the number of discontinuity points. So we probably have the option of drawing discontinuity points using scenery.Circle. In https://github.com/phetsims/calculus-grapher/issues/256#diff-2b946cddf945cc06778f4cad002992c6ce7015bd358bccca732505613b91a9a1, I accientally pushed DiscontinuousPointsPlot.ts, which is not used yet, though I confirmed that it works.

[pixelzoom]

In the above commit, I swapped changed CurveNode's discontinuousLinePlot from ScatterPlot to DiscontinuousPointsPlot. It was a trivial change, and easy to change back.

@veillette while you're working on cusps in #208, please be on the lookout for any incorrect behavior or perforamance issues. Then we can evaluate whether to keep DiscontinuousPointsPlot, or revert to ScatterPlot.

[veillette]

I did not notice anything suspicious with my work on #208 and #275, so it is working well. I reviewed DiscontinuousPointsPlot. Since we cannot guarantee a maximum number of scenery.Circle that are needed for the plots, I think your approach to recycle some of the Circles, and generate others as needed is the most appropriate.

[veillette]

Reassigning to @pixelzoom in case there is something else do to here.

[pixelzoom]

I believe we're done here! Closing.