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.

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

Jump to bottom

adding support for icon-rotate and icon-allow-overlap in mapbox #4575

Merged
merged 12 commits into from
Apr 24, 2020
Merged

adding support for icon-rotate and icon-allow-overlap in mapbox #4575

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e82c14c
adding support for icon-rotate and icon-allow-overlap in mapbox
zouhairm Feb 11, 2020
3a12803
fixing code style
zouhairm Feb 12, 2020
093f507
removing arrayOk from boolean attribute
zouhairm Feb 14, 2020
5285343
added jasmine tests for icon-rotate and icon-allow-overlap
zouhairm Feb 15, 2020
684df71
checking icon-rotate is in the layout
zouhairm Feb 15, 2020
2242b28
generating mapbox_angles.png from mapbox_angles.json
zouhairm Feb 15, 2020
6943d7a
PR suggestion
zouhairm Apr 23, 2020
f9eb48b
PR suggestion
zouhairm Apr 23, 2020
243680b
changing mapbox icon angle default to 0 + minor code edit
zouhairm Apr 23, 2020
f725425
Merge remote-tracking branch 'upstream/master' into dev/mbx_symbol
zouhairm Apr 24, 2020
1e1458b
cast to numbers in scattermapbox angle arrays
alexcjohnson Apr 24, 2020
cd91905
Update test/jasmine/tests/scattermapbox_test.js
alexcjohnson Apr 24, 2020
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
17 changes: 17 additions & 0 deletions src/traces/scattermapbox/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,23 @@ module.exports = overrideAll({
'are only available for *circle* symbols.'
].join(' ')
},
angle: {
zouhairm marked this conversation as resolved.
Show resolved Hide resolved
valType: 'number',
dflt: null,
zouhairm marked this conversation as resolved.
Show resolved Hide resolved
role: 'style',
arrayOk: true,
description: [
'Sets the marker angle.'
zouhairm marked this conversation as resolved.
Show resolved Hide resolved
].join(' ')
},
allowoverlap: {
valType: 'boolean',
dflt: false,
role: 'style',
description: [
'Flag to allow symbols to overlap'
zouhairm marked this conversation as resolved.
Show resolved Hide resolved
].join(' ')
},
opacity: markerAttrs.opacity,
size: markerAttrs.size,
sizeref: markerAttrs.sizeref,
Expand Down
30 changes: 27 additions & 3 deletions src/traces/scattermapbox/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,23 @@ module.exports = function convert(gd, calcTrace) {
'icon-size': trace.marker.size / 10
});

if('angle' in trace.marker) {
Lib.extendFlat(symbol.layout, {
// unfortunately cant use {angle} do to this issue:
// https://github.com/mapbox/mapbox-gl-js/issues/873
'icon-rotate': {
type: 'identity', property: 'angle'
},
'icon-rotation-alignment': 'map'
});
}

if('allowoverlap' in trace.marker) {
zouhairm marked this conversation as resolved.
Show resolved Hide resolved
Lib.extendFlat(symbol.layout,
{'icon-allow-overlap': trace.marker.allowoverlap});
}


Lib.extendFlat(symbol.paint, {
'icon-opacity': trace.opacity * trace.marker.opacity,

Expand Down Expand Up @@ -239,15 +256,21 @@ function makeSymbolGeoJSON(calcTrace, gd) {

var marker = trace.marker || {};
var symbol = marker.symbol;
var angle = marker.angle;

var fillSymbol = (symbol !== 'circle') ?
getFillFunc(symbol) :
blankFillFunc;

var fillAngle = (angle) ?
getFillFunc(angle) :
blankFillFunc;

var fillText = subTypes.hasText(trace) ?
getFillFunc(trace.text) :
blankFillFunc;


var features = [];

for(var i = 0; i < calcTrace.length; i++) {
Expand All @@ -266,7 +289,7 @@ function makeSymbolGeoJSON(calcTrace, gd) {
var meta = trace._meta || {};
text = Lib.texttemplateString(tt, labels, fullLayout._d3locale, pointValues, calcPt, meta);
} else {
text = fillText(calcPt.tx);
text = fillText(i);
}

if(text) {
Expand All @@ -280,7 +303,8 @@ function makeSymbolGeoJSON(calcTrace, gd) {
coordinates: calcPt.lonlat
},
properties: {
symbol: fillSymbol(calcPt.mx),
symbol: fillSymbol(i),
angle: fillAngle(i),
text: text
}
});
Expand All @@ -294,7 +318,7 @@ function makeSymbolGeoJSON(calcTrace, gd) {

function getFillFunc(attr) {
if(Lib.isArrayOrTypedArray(attr)) {
return function(v) { return v; };
return function(i) { return attr[i]; };
Copy link
Collaborator

Choose a reason for hiding this comment

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

This actually seems just fine for symbol and text, since those are both strings / string arrays. But what happens for angles if the user provides an array of numbers-as-strings? A single number as a string will get converted to an actual number by coerce, but for performance we don't reach into arrays to do this (until the calc step - that would be the one benefit of including angle in calc, if it was being used from there here, but I don't recommend that because scattermapbox shares calc with scattergeo so this would need to be a special case.)

It may be that this is just fine - assuming mapbox casts this angle to a number consistently, and treats non-numbers as 0. Can you confirm that this works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok so I tested this, and yes basically coerce seems to turn a string into an actual number which means it gets interpreted as an acceptable angle, but if provided as an array, then string values get passed on to mapbox which treats non-numbers as 0.

I'm not sure whether that's an acceptable behavior, and honestly I don't think I know enough about the code to figure out how to make it work the way it should without having to have a few back and forths and me diving deeper into other functions. So if you don't mind if this is not acceptable, it'd be great if you could suggest a fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think 1e1458b should do it 🙏

} else if(attr) {
return function() { return attr; };
} else {
Expand Down
3 changes: 3 additions & 0 deletions src/traces/scattermapbox/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
if(subTypes.hasMarkers(traceOut)) {
handleMarkerDefaults(traceIn, traceOut, defaultColor, layout, coerce, {noLine: true});

coerce('marker.allowoverlap');
coerce('marker.angle');

// array marker.size and marker.color are only supported with circles
var marker = traceOut.marker;
if(marker.symbol !== 'circle') {
Expand Down
Binary file modified test/image/baselines/mapbox_angles.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.
4 changes: 3 additions & 1 deletion test/image/mocks/mapbox_angles.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@
"monument",
"harbor",
"music"
]
],
"angle": [-45, 45, 0],
"allowoverlap":true
},
"subplot": "mapbox2"
}
Expand Down
24 changes: 24 additions & 0 deletions test/jasmine/tests/scattermapbox_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,30 @@ describe('scattermapbox convert', function() {
expect(symbolProps).toEqual(expected, 'geojson properties');
});


it('should allow symbols to be rotated and overlapped', function() {
var opts = _convert(Lib.extendFlat({}, base, {
mode: 'markers',
marker: {
symbol: ['monument', 'music', 'harbor'],
angle: [0, 90, 45],
allowoverlap: true
},
}));

var symbolAngle = opts.symbol.geojson.features.map(function(f) {
return f.properties.angle;
});

var expected = [0, 90, 45, undefined, undefined];
alexcjohnson marked this conversation as resolved.
Show resolved Hide resolved
expect(symbolAngle).toEqual(expected, 'geojson properties');


expect(opts.symbol.layout['icon-rotate'].property).toEqual('angle', 'symbol.layout.icon-rotate');
expect(opts.symbol.layout['icon-allow-overlap']).toEqual(true, 'symbol.layout.icon-allow-overlap');
});


it('should generate correct output for text + lines traces', function() {
var opts = _convert(Lib.extendFlat({}, base, {
mode: 'lines+text',
Expand Down