Skip to content

Add "arrowanchor" property to annotations #2164

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Dec 7, 2017
Merged

Add "arrowanchor" property to annotations #2164

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f598c92
add arrowanchor property
apalchys Nov 16, 2017
c4a0f1f
add 'startarrowhead', 'startarrowsize' and 'startstandoff' and renam…
apalchys Nov 21, 2017
19a06c6
update auto range based on startarrowsize and baseline tests
apalchys Dec 6, 2017
42138e7
fix tests
apalchys Dec 7, 2017
191d5d2
coerce arrowhead, arrowsize and startarrowsize only when it is needed
apalchys Dec 7, 2017
8e75b74
update baseline image for gl2d_annotations
apalchys Dec 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 46 additions & 3 deletions src/components/annotations/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,25 @@ module.exports = {
dflt: 1,
role: 'style',
editType: 'arraydraw',
description: 'Sets the annotation arrow head style.'
description: 'Sets the end annotation arrow head style.'
},
startarrowhead: {
valType: 'integer',
min: 0,
max: ARROWPATHS.length,
dflt: 1,
role: 'style',
editType: 'arraydraw',
description: 'Sets the start annotation arrow head style.'
},
arrowside: {
valType: 'flaglist',
flags: ['end', 'start'],
extras: ['none'],
dflt: 'end',
role: 'style',
editType: 'arraydraw',
description: 'Sets the annotation arrow head position.'
},
arrowsize: {
valType: 'number',
Expand All @@ -182,7 +200,18 @@ module.exports = {
role: 'style',
editType: 'calcIfAutorange',
description: [
'Sets the size of the annotation arrow head, relative to `arrowwidth`.',
'Sets the size of the end annotation arrow head, relative to `arrowwidth`.',
'A value of 1 (default) gives a head about 3x as wide as the line.'
].join(' ')
},
startarrowsize: {
valType: 'number',
min: 0.3,
dflt: 1,
role: 'style',
editType: 'calcIfAutorange',
description: [
'Sets the size of the start annotation arrow head, relative to `arrowwidth`.',
'A value of 1 (default) gives a head about 3x as wide as the line.'
].join(' ')
},
Expand All @@ -200,7 +229,21 @@ module.exports = {
role: 'style',
editType: 'calcIfAutorange',
description: [
'Sets a distance, in pixels, to move the arrowhead away from the',
'Sets a distance, in pixels, to move the end arrowhead away from the',
'position it is pointing at, for example to point at the edge of',
'a marker independent of zoom. Note that this shortens the arrow',
'from the `ax` / `ay` vector, in contrast to `xshift` / `yshift`',
'which moves everything by this amount.'
].join(' ')
},
startstandoff: {
valType: 'number',
min: 0,
dflt: 0,
role: 'style',
editType: 'calcIfAutorange',
description: [
'Sets a distance, in pixels, to move the start arrowhead away from the',
'position it is pointing at, for example to point at the edge of',
'a marker independent of zoom. Note that this shortens the arrow',
'from the `ax` / `ay` vector, in contrast to `xshift` / `yshift`',
Expand Down
11 changes: 9 additions & 2 deletions src/components/annotations/common_defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,18 @@ module.exports = function handleAnnotationCommonDefaults(annIn, annOut, fullLayo
if(h) coerce('valign');

if(showArrow) {
var arrowside = coerce('arrowside');
var arrowhead = coerce('arrowhead');
var arrowsize = coerce('arrowsize');

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

coerce('startarrowsize', arrowsize);

and

coerce('startstandoff');

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

Copy link
Contributor Author

@apalchys apalchys Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
coerce('startarrowsize', arrowsize);
coerce('arrowcolor', borderOpacity ? annOut.bordercolor : Color.defaultLine);
coerce('arrowhead');
coerce('arrowsize');
coerce('arrowwidth', ((borderOpacity && borderWidth) || 1) * 2);
coerce('standoff');
coerce('startstandoff');

}

Expand Down
5 changes: 3 additions & 2 deletions src/components/annotations/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,8 @@ function drawRaw(gd, options, index, subplotId, xa, ya) {
});

var strokewidth = options.arrowwidth,
arrowColor = options.arrowcolor;
arrowColor = options.arrowcolor,
arrowSide = options.arrowside;

var arrowGroup = annGroup.append('g')
.style({opacity: Color.opacity(arrowColor)})
Expand All @@ -520,7 +521,7 @@ function drawRaw(gd, options, index, subplotId, xa, ya) {
.style('stroke-width', strokewidth + 'px')
.call(Color.stroke, Color.rgb(arrowColor));

drawArrowHead(arrow, 'end', options);
drawArrowHead(arrow, arrowSide, options);

// the arrow dragger is a small square right at the head, then a line to the tail,
// all expanded by a stroke width of 6px plus the arrow line width
Expand Down
84 changes: 51 additions & 33 deletions src/components/annotations/draw_arrow_head.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,15 @@ var ARROWPATHS = require('./arrow_paths');
*
* @param {d3.selection} el3: a d3-selected line or path element
*
* @param {string} ends: 'start', 'end', or 'start+end' for which ends get arrowheads
* @param {string} ends: 'none', 'start', 'end', or 'start+end' for which ends get arrowheads
*
* @param {object} options: style information. Must have all the following:
* @param {number} options.arrowhead: head style - see ./arrow_paths
* @param {number} options.arrowsize: relative size of the head vs line width
* @param {number} options.standoff: distance in px to move the arrow point from its target
* @param {number} options.arrowhead: end head style - see ./arrow_paths
* @param {number} options.startarrowhead: start head style - see ./arrow_paths
* @param {number} options.arrowsize: relative size of the end head vs line width
* @param {number} options.startarrowsize: relative size of the start head vs line width
* @param {number} options.standoff: distance in px to move the end arrow point from its target
* @param {number} options.startstandoff: distance in px to move the start arrow point from its target
* @param {number} options.arrowwidth: width of the arrow line
* @param {string} options.arrowcolor: color of the arrow line, for the head to match
* Note that the opacity of this color is ignored, as it's assumed the container
Expand All @@ -35,10 +38,14 @@ var ARROWPATHS = require('./arrow_paths');
module.exports = function drawArrowHead(el3, ends, options) {
var el = el3.node();
var headStyle = ARROWPATHS[options.arrowhead || 0];
var startHeadStyle = ARROWPATHS[options.startarrowhead || 0];
var scale = (options.arrowwidth || 1) * options.arrowsize;
var startScale = (options.arrowwidth || 1) * options.startarrowsize;
var doStart = ends.indexOf('start') >= 0;
var doEnd = ends.indexOf('end') >= 0;
var doNone = ends.indexOf('none') >= 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var backOff = headStyle.backoff * scale + options.standoff;
var startBackOff = startHeadStyle.backoff * startScale + options.startstandoff;

var start, end, startRot, endRot;

Expand All @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    }
}

hideLine();
return;
}
}

if(backOff) {
if(backOff * backOff > dx * dx + dy * dy) {
hideLine();
Expand All @@ -59,17 +73,27 @@ module.exports = function drawArrowHead(el3, ends, options) {
var backOffX = backOff * Math.cos(startRot),
backOffY = backOff * Math.sin(startRot);

if(doStart) {
start.x -= backOffX;
start.y -= backOffY;
el3.attr({x1: start.x, y1: start.y});
}
if(doEnd) {
if(doEnd || doNone) {
end.x += backOffX;
end.y += backOffY;
el3.attr({x2: end.x, y2: end.y});
}
}

if(startBackOff) {
if(startBackOff * startBackOff > dx * dx + dy * dy) {
hideLine();
return;
}
var startBackOffX = startBackOff * Math.cos(startRot),
startbackOffY = startBackOff * Math.sin(startRot);

if(doStart || doNone) {
start.x -= startBackOffX;
start.y -= startbackOffY;
el3.attr({x1: start.x, y1: start.y});
}
}
}
else if(el.nodeName === 'path') {
var pathlen = el.getTotalLength(),
Expand All @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly, seems like only the last condition matters here?

hideLine();
return;
}

if(doStart) {
var start0 = el.getPointAtLength(0);
var dstart = el.getPointAtLength(0.1);

startRot = Math.atan2(start0.y - dstart.y, start0.x - dstart.x);
start = el.getPointAtLength(Math.min(backOff, pathlen));
var start0 = el.getPointAtLength(0);
var dstart = el.getPointAtLength(0.1);

if(backOff) dashArray = '0px,' + backOff + 'px,';
}
startRot = Math.atan2(start0.y - dstart.y, start0.x - dstart.x);
start = el.getPointAtLength(Math.min(startBackOff, pathlen));

if(doEnd) {
var end0 = el.getPointAtLength(pathlen);
var dend = el.getPointAtLength(pathlen - 0.1);
dashArray = '0px,' + startBackOff + 'px,';

endRot = Math.atan2(end0.y - dend.y, end0.x - dend.x);
end = el.getPointAtLength(Math.max(0, pathlen - backOff));
var end0 = el.getPointAtLength(pathlen);
var dend = el.getPointAtLength(pathlen - 0.1);

if(backOff) {
var shortening = dashArray ? 2 * backOff : backOff;
dashArray += (pathlen - shortening) + 'px,' + pathlen + 'px';
}
}
else if(dashArray) dashArray += pathlen + 'px';
endRot = Math.atan2(end0.y - dend.y, end0.x - dend.x);
end = el.getPointAtLength(Math.max(0, pathlen - backOff));

var shortening = dashArray ? startBackOff + backOff : backOff;
dashArray += (pathlen - shortening) + 'px,' + pathlen + 'px';

if(dashArray) el3.style('stroke-dasharray', dashArray);
el3.style('stroke-dasharray', dashArray);
}

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

function drawhead(p, rot) {
function drawhead(headStyle, p, rot, scale) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed names to arrowHeadStyle and arrowScale

if(!headStyle.path) return;
if(headStyle.noRotate) rot = 0;

Expand All @@ -132,6 +150,6 @@ module.exports = function drawArrowHead(el3, ends, options) {
});
}

if(doStart) drawhead(start, startRot);
if(doEnd) drawhead(end, endRot);
if(doStart) drawhead(startHeadStyle, start, startRot, startScale);
if(doEnd) drawhead(headStyle, end, endRot, scale);
};
4 changes: 4 additions & 0 deletions src/components/annotations3d/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,13 @@ module.exports = overrideAll({
showarrow: annAtts.showarrow,
arrowcolor: annAtts.arrowcolor,
arrowhead: annAtts.arrowhead,
startarrowhead: annAtts.startarrowhead,
arrowside: annAtts.arrowside,
arrowsize: annAtts.arrowsize,
startarrowsize: annAtts.startarrowsize,
arrowwidth: annAtts.arrowwidth,
standoff: annAtts.standoff,
startstandoff: annAtts.startstandoff,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it works fine

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

hovertext: annAtts.hovertext,
hoverlabel: annAtts.hoverlabel,
captureevents: annAtts.captureevents,
Expand Down
Binary file modified test/image/baselines/annotations.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 2 additions & 0 deletions test/image/mocks/annotations.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Copy link
Contributor

@etpinard etpinard Nov 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Good idea, I will add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
"text": "arrow ML<br>+standoff", "x": 2, "y": 2, "ax": 20, "ay": 20,
Expand Down
10 changes: 10 additions & 0 deletions test/jasmine/tests/annotations_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,16 @@ describe('Test annotations', function() {
expect(layoutOut.annotations[2]._xclick).toBe(2, 'log');
expect(layoutOut.annotations[2]._yclick).toBe('A', 'category');
});

it('should default to end for arrowside', function() {
var layoutIn = {
annotations: [{ showarrow: true, arrowhead: 2 }]
};

var out = _supply(layoutIn);

expect(out[0].arrowside).toEqual('end');
});
});
});

Expand Down