Add "arrowanchor" property to annotations #2164

apalchys · 2017-11-16T14:31:33Z

This PR adds possibility to draw annotations arrow head on both sides of a line.
Plotly.js has the following comment in draw-arrow-head.js

plotly.js/src/components/annotations/draw_arrow_head.js

Line 23 in 8b35a8e

    
            * @param {string} ends: 'start', 'end', or 'start+end' for which ends get arrowheads

but only end is used in annotations/draw.js

plotly.js/src/components/annotations/draw.js

Line 523 in 8b35a8e

drawArrowHead(arrow, 'end', options);

So I added a new annotations property arrowanchor with the following possible options:

start
end (default)
start+end

Screenshot:

Demo:
https://codepen.io/plotly-demo/pen/aVBZBq

alexcjohnson · 2017-11-16T22:50:25Z

Ha, nice. That functionality was halfway built years ago but never finished. A couple of comments:

The word anchor is used elsewhere in plotly.js for a very different purpose. What can we call this then? arrowends? showarrowheads? @etpinard thoughts?
What if someone wants different heads on each end? Say a circle on one end and an arrow on the other? One way to do this would be to have a new attribute maybe startarrowhead that applies to the start, and inherits from arrowhead if not supplied.
Similarly, should there be a standoff for the start? startstandoff?
Do we allow text AND a starting arrowhead?

apalchys · 2017-11-17T13:19:58Z

@alexcjohnson

One way to do this would be to have a new attribute maybe startarrowhead that applies to the start, and inherits from arrowhead if not supplied.

Similarly, should there be a standoff for the start? startstandoff?

Ok, no problem to add it. Let's agree about new name for arrowanchor (@etpinard) and I will update my PR.

Do we allow text AND a starting arrowhead?

Yes, we do. See my screenshot above - "start+end" example

etpinard · 2017-11-17T15:40:04Z

I'd vote for:

arrowside: (flaglist): 'end' (default) | 'start' | 'start+end'

// yes should add a 'startarrowhead' attribute
var arrowside = coerce('arrowside');
var arrowhead = coerce('arrowhead');

if(arrowside.indexOf('start') !== -1) {
    coerce('startarrowhead', arrowhead);
}

// as well as 'startstandoff'
// though I don't think this one needs to inherit its
// default from 'standoff'

Do we allow text AND a starting arrowhead?

That probably means we'll need add a few more attributes to complement xanchor and yanchor to allow users to position the text labels e.g. at the start of the arrow or at the mid-point between the tip and tail. That said, a PR that just adds arrowside, startarrowhead and startstandoff would be a solid first step.

alexcjohnson · 2017-11-17T16:05:09Z

as well as 'startstandoff'
though I don't think this one needs to inherit its
default from 'standoff'

Agreed - standoff is specific to the item you're pointing at, so a different item will in general want a different standoff, and inheriting this would more often hurt than help.

@apalchys check out some of the other 'flaglist' attributes for how arrowside should work. Do we want a 'none' setting (in extras) here? You can get that behavior with arrowhead but it might be convenient to be able to do it with arrowside as well?

etpinard · 2017-11-17T16:19:07Z

Do we want a 'none' setting (in extras) here? You can get that behavior with arrowhead but it might be convenient to be able to do it with arrowside as well?

I like the sounds of that 'none' flag. Perhaps we could even deprecated arrowhead: 0.

… 'arrowanchor' to 'arrowside'

apalchys · 2017-11-21T07:49:21Z

I made the following changes:

added startarrowhead
added startarrowsize
added startstandoff
renamed arrowanchor to arrowside
added none to the extras of arrowside (0 still works)
made possible to add standoff and startstandoff without arrowheads

Here is the updated demo: https://codepen.io/plotly-demo/pen/aVBZBq

etpinard · 2017-11-24T01:17:42Z

src/components/annotations/common_defaults.js

+        var arrowsize = coerce('arrowsize');
+
+        if(arrowside.indexOf('start') !== -1) {
+            coerce('startarrowhead', arrowhead);


coerce('startarrowsize', arrowsize);

and

coerce('startstandoff');

should be inside this block as well - as they have no effect w/o start arrow.

There was possibility to use standoff with arrowhead: 0 (no arrow) before my changes and I decided to keep this functionality for ‘startstandoff’ -> when we have arrowside: none, we still can set both properties of standoff and see the result.

Please just confirm that it is not expected behaviour and I will apply suggested changes.

Ha good point. I think you have it right here.

Right, so (start)standoff always needs to be coerced - but doesn't startarrowsize go in here?

Also if we have no 'end' we don't need arrowhead and arrowsize do we?

Seems like @apalchys posted and deleted a comment, so maybe he noticed what I'm about to say... (incidentally, if that's the case, let me encourage you to edit comments, using ~~strikethrough~~ if necessary, rather than deleting them, as it makes the conversation flow clearer)

(start)arrowhead and (start)arrowsize are not needed logically when we're not showing a head on that end, so that's a warning sign that providing them could lead to bugs. Indeed, I think there is a subtle one: if you choose an arrowhead that has a backoff (including the default arrowhead: 1! backoff is how much you need to move the end of the line backward so the arrowhead can point to the right place without the line jutting through it) but you don't show that arrowhead, you'll get an excessive standoff.

options.arrowhead || 0 ensures an un-coerced arrowhead is treated as no arrow, and we may need options.arrowsize || 0 (this value probably doesn't matter as long as it's numeric) to make the math well-defined.

sorry, I recognized that my answer is wrong right after I submit it. I will commit an update soon.

Please check my commit - 191d5d2
Is it what you expected?

Perfect, thanks! Looks like this tweaked one more test image (gl2d_annotations) but once that's fixed I think we're ready to go 🎉

etpinard · 2017-11-24T01:24:46Z

src/components/annotations3d/attributes.js

    arrowwidth: annAtts.arrowwidth,
    standoff: annAtts.standoff,
+    startstandoff: annAtts.startstandoff,


Does this really work in 3D or are these attributes there just to make the supply-default function not error out?

Yes, it works fine

Great. Would you mind adding an annotations item showing off the new attributes to gl3d_annotations.json?

etpinard · 2017-11-24T01:26:07Z

test/image/mocks/annotations.json

@@ -45,6 +45,8 @@
                "arrowcolor":"rgb(166, 28, 0)","borderpad":3,"textangle":50,"x":5,"y":1
            },
            {"text":"","showarrow":true,"borderwidth":1.2,"arrowhead":2,"axref":"x","ayref":"y","x":5,"y":3,"ax":4,"ay":5},
+            {"text":"","showarrow":true,"borderwidth":1.2,"arrowhead":3,"startarrowhead":6,"arrowsize":1.5,"startarrowsize":3,"standoff": 7,"startstandoff": 15,"arrowside":"start+end","axref":"x","ayref":"y","x":2,"y":4,"ax":3,"ay":4},
+            {"text":"","showarrow":true,"borderwidth":1.2,"startarrowhead":2,"startstandoff": 5,"arrowside":"start","axref":"x","ayref":"y","x":1,"y":5,"ax":2,"ay":4},
            {"text":"","showarrow":true,"borderwidth":1.2,"arrowhead":2,"axref":"x","ayref":"y","x":6,"y":2,"ax":3,"ay":3},


Nice job here! Maybe we could add an arrowside: 'none' item?

Thanks. Good idea, I will add it.

Since it depends on your answer here #2164 (comment), I will add it when you confirm expected behaviour.

etpinard · 2017-11-29T15:38:36Z

I'll let @alexcjohnson review your draw_arrow_head.js additions as he wrote most of the original code.

I just noticed that the new start* arrow attributes don't currently have an effect on the autorange calculations. It's not exactly obvious to me what the behavior should be. Maybe this line could become:

headSize = 3 * Math.max(ann.arrowsize * ann.arrowwidth, ann.startarrowsize * ann.startarrowwidth) || 0;

alexcjohnson · 2017-11-29T17:04:05Z

src/components/annotations/draw_arrow_head.js

@@ -51,6 +58,13 @@ module.exports = function drawArrowHead(el3, ends, options) {

        startRot = Math.atan2(dy, dx);
        endRot = startRot + Math.PI;
+        if(backOff && startBackOff) {
+            if(backOff * backOff + startBackOff * startBackOff > dx * dx + dy * dy) {


I don't think this condition is quite right... should be the square of the sum, not the sum of squares. Probably we should just compare lengths, not lengths squared, that was an optimization on my part but it's obviously not very intuitive or extensible!

But also I think we can simplify and only do this whole check once, instead of once for both, then again for backOff, and a third time for startBackOff. Looks like no matter what both values will be non-negative numbers, so we should be able to do something like:

if(backOff || startBackOff) { if(backOff + startBackOff > Math.sqrt(dx * dx + dy * dy) { hideline(); return; } }

alexcjohnson · 2017-11-29T17:10:28Z

src/components/annotations/draw_arrow_head.js

@@ -79,41 +103,35 @@ module.exports = function drawArrowHead(el3, ends, options) {
            // combine the two
            dashArray = '';

-        if(pathlen < backOff) {
+        if(pathlen < backOff || pathlen < startBackOff || pathlen < backOff + startBackOff) {


similarly, seems like only the last condition matters here?

alexcjohnson · 2017-11-29T17:18:35Z

src/components/annotations/draw_arrow_head.js

    var doStart = ends.indexOf('start') >= 0;
    var doEnd = ends.indexOf('end') >= 0;
+    var doNone = ends.indexOf('none') >= 0;


'none' is an extras value, so it can only be given alone - ie ends === 'none'

That said, I'm a little confused by the if(do[Start|End] || doNone) logic below - haven't we settled on being able to apply either standoff regardless of whether there's an arrowhead shown at that end (and if not, which mechanism was used to hide it)? ie that if statement would disappear and we always execute its block?

Removed these if statements, now we always execute end|start blocks

alexcjohnson · 2017-11-29T17:22:10Z

src/components/annotations/draw_arrow_head.js

    }

    function hideLine() { el3.style('stroke-dasharray', '0px,100px'); }

-    function drawhead(p, rot) {
+    function drawhead(headStyle, p, rot, scale) {


This will work, but it's a bit confusing to reuse the names from the outer scope - perhaps rename to something like thisHeadStyle and thisScale?

Changed names to arrowHeadStyle and arrowScale

alexcjohnson · 2017-11-29T20:33:23Z

I just noticed that the new start* arrow attributes don't currently have an effect on the autorange calculations

Good catch @etpinard - this one is a little tricky, we need to cover two cases:

If a(x|y)ref is data-referenced then we could do it by modifying the textbox padding to include both the textbox and the start arrowhead - will probably look something like

ppadplus: Math.max(ann._xpadplus, startHeadPlus),
ppadminus: Math.max(ann._xpadminus, startHeadMinus)

where startHead(Plus|Minus) I guess would be calculated as head[Plus|Minus] are, but using the start head size and width.

If a(x|y)ref is in pixels then we probably need a third item in the Math.max calculations, which I'm guessing will be something startHeadPlus + ann.a(x|y) and startHeadMinus - ann.a(x|y)

These will need a little bit of testing with the strange edge cases where the start arrowhead actually does poke out farther than anything else, like:

This is going to in general overestimate the padding needed for the pointy arrowheads (as opposed to the circle and square which are centered on the target point) but that's already how we do it for the end arrowhead, and doing better than that would be complicated enough that I would vote to ignore it until someone complains.

apalchys · 2017-12-06T14:53:39Z

@alexcjohnson @etpinard made changes based on your review.

alexcjohnson · 2017-12-06T16:15:21Z

test/image/mocks/annotations-autorange.json

+                "arrowside":"start+end","startarrowhead":6,"startarrowsize":1,"xanchor": "right","yanchor": "bottom",
+                "bordercolor": "#444", "borderwidth": 3, "height": 30,"textangle":0,
+                "yshift": 10
+            },


alexcjohnson · 2017-12-06T16:19:31Z

@apalchys nice job, I really like the image tests. Is the test failure we're seeing right now an intermittent issue, CI-only, or do you see it locally too? Otherwise I think my only (small) blocker is #2164 (comment)

alexcjohnson · 2017-12-06T16:46:57Z

Same test failure twice - do you see this locally @apalchys?

	Expected 2.010335917312661,7.24031007751938 to be close to 1.41,7.42 xaxis4
	    at assertRanges (/tmp/tests/annotations_test.js:603:0 <- /tmp/56b7c3186b48d6f36aba40253c5487ac.browserify:250698:41)
	    at /tmp/tests/annotations_test.js:641:0 <- /tmp/56b7c3186b48d6f36aba40253c5487ac.browserify:250736:13

apalchys · 2017-12-07T11:38:03Z

@alexcjohnson everything was fine for me locally but I made a small adjustment and they pass now.

alexcjohnson · 2017-12-07T15:11:01Z

💃

apalchys · 2017-12-07T15:14:24Z

Thanks for your support! 👍

add arrowanchor property

f598c92

etpinard added status: in progress feature labels Nov 20, 2017

add 'startarrowhead', 'startarrowsize' and 'startstandoff' and rename…

c4a0f1f

… 'arrowanchor' to 'arrowside'

etpinard reviewed Nov 24, 2017

View reviewed changes

etpinard added status: reviewable and removed status: in progress labels Nov 24, 2017

alexcjohnson reviewed Nov 29, 2017

View reviewed changes

update auto range based on startarrowsize and baseline tests

19a06c6

alexcjohnson reviewed Dec 6, 2017

View reviewed changes

fix tests

42138e7

apalchys added 2 commits December 7, 2017 16:59

coerce arrowhead, arrowsize and startarrowsize only when it is needed

191d5d2

update baseline image for gl2d_annotations

8e75b74

alexcjohnson merged commit 6b4b892 into plotly:master Dec 7, 2017

etpinard added this to the v1.32.0 milestone Dec 7, 2017

Add "arrowanchor" property to annotations #2164

Add "arrowanchor" property to annotations #2164

Conversation

apalchys commented Nov 16, 2017

alexcjohnson commented Nov 16, 2017

apalchys commented Nov 17, 2017

etpinard commented Nov 17, 2017 • edited Loading

alexcjohnson commented Nov 17, 2017

etpinard commented Nov 17, 2017

apalchys commented Nov 21, 2017

Choose a reason for hiding this comment

apalchys Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Nov 29, 2017

apalchys commented Dec 6, 2017

Choose a reason for hiding this comment

alexcjohnson commented Dec 6, 2017

alexcjohnson commented Dec 6, 2017

apalchys commented Dec 7, 2017

alexcjohnson commented Dec 7, 2017

apalchys commented Dec 7, 2017

etpinard commented Nov 17, 2017 •

edited

Loading

apalchys Nov 27, 2017 •

edited

Loading

etpinard Nov 24, 2017 •

edited

Loading

etpinard commented Nov 29, 2017 •

edited

Loading