Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for axis without arrow head(s) #38

Closed
pixelzoom opened this issue Jun 15, 2021 · 7 comments
Closed

Support for axis without arrow head(s) #38

pixelzoom opened this issue Jun 15, 2021 · 7 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jun 15, 2021

Axes are frequently need to be drawn without arrow heads in PhET sims. To accomplish that using bamboo's AxisNode, you have to clip to the ChartRectange to remove the parts of the axis (including the arrow head or heads) that extend past the edges.

So something like what I've done in base class WaveformChartNode in Fourier:

    // Parent for Nodes that must be clipped to the bounds of chartRectangle
    const clippedParent = new Node( {
      clipArea: chartRectangle.getShape(),
      children: [ xAxis, yAxis ]
    } );

    options.children = [
      .... // all the childen that are not clipped
      clippedParent
    ];

This is inconvenient, and I'd like to have a way of creating axes that do not have arrow heads. Some ways to accomplish that:

(1) Add an option to AxisNode, maybe something like:

// 0  = no arrow heads
// 1 = one arrow head, pointing in the positive direction
// 2 = two arrow heads
numberOfArrowHeads: 2

This would be a rewrite of AxisNode, since it currently extends ArrowNode and uses its options. Surprisingly, I only see AxisNode used in Fourier.

(2) Create a new type of axis Node that does not involve arrows. The extension option might still be nice to have, but default to 0. This involves coming up with a good name for the new class, and probably coming up with a new name for AxisNode -- maybe AxisLine and AxisArrowNode, since they are subclasses (specializations) of Line and ArrowNode? This is definitely the easier of the 2 approaches.

@samreid thoughts?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jun 15, 2021

In the above commits, I added AxisLine, and I'm using it throughout Fourier.

There is a lot of duplication between AxisLine and AxisNode, but also some important differences.

@samreid
Copy link
Member

samreid commented Jun 16, 2021

AxisLine sounds reasonable, but I'm wondering what you think about using the ArrowNode API for customizing it. For instance, setting an arrow head width/height to 0? Or combining that with doubleHead to determine the number of arrowheads?

@samreid samreid assigned pixelzoom and unassigned samreid Jun 16, 2021
@pixelzoom
Copy link
Contributor Author

That was option (2) in #38 (comment). It's more complicated and will involve options that can't be combined. I sort of double that ArrowNode supports headWidth:0 or headHeight:0, and I really don't want to touch ArrowNode.

So I'm inclined to rename AxisNode to AxisArrowNode, and call this done. @samreid thoughts?

@pixelzoom pixelzoom assigned samreid and unassigned pixelzoom Jun 22, 2021
@pixelzoom
Copy link
Contributor Author

I guess another approach would be AxisNode extends Node, and then have it create either ArrowNode or Line, based on options. An immediate problem is that ArrowNode and Line have very different APIs. So AxisNode API would probably need to look something like (yuck):

class AxisNode extends Node {

    options = merge( {
      value: 0, // by default the axis is at 0, but you can put it somewhere else
      extension: 20, // in view coordinates, how far the axis goes past the edge of the ChartRectangle

      useArrowNode: true, // true=use ArrowNode and arrowNodeOptions, false= use Line and lineOptions
      
      // ArrowNode options
      arrowNodeOptions: {
        doubleHead: true,
        headHeight: 10,
        headWidth: 10,
        tailWidth: 2
      },
      
      // Line options
      lineOptions: {
        lineWidth: 2
      }
    }, options );
} 

@samreid
Copy link
Member

samreid commented Jun 27, 2021

So I'm inclined to rename AxisNode to AxisArrowNode, and call this done. @samreid thoughts?

That seems the best solution to me. Still sound good to you?

@samreid samreid assigned pixelzoom and unassigned samreid Jun 27, 2021
pixelzoom added a commit that referenced this issue Jul 6, 2021
pixelzoom added a commit that referenced this issue Jul 6, 2021
@pixelzoom
Copy link
Contributor Author

In the above commits, I renamed AxisNode to AxisArrowNode, and I used AxisLine in one bamboo demo. I was surprised to find that AxisNode was not used outside of the bamboo demos.

Since this is a breaking API change, I announced on the Google Group at https://groups.google.com/g/developing-interactive-simulations-in-html5/c/1Zm4l3sye20/m/Iy34b9jRAAAJ.

@samreid have a look, feel free to close if it looks OK.

@samreid
Copy link
Member

samreid commented Jul 7, 2021

I reviewed the commits and tested the ChartCanvasNode demo. It seems reasonable and I don't have any recommendations, closing.

@samreid samreid closed this as completed Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants