Skip to content

Selection re-ordering for scatter traces with ids #1709

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 5 commits into from
May 23, 2017
Merged

Selection re-ordering for scatter traces with ids #1709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cbcec26
add failing test cases showcasing issue #1706
etpinard May 19, 2017
83234b5
[breaking] add `join.order()` calls during marker and text node updates
etpinard May 19, 2017
c8c3197
rm Drawing.pointStyle and Drawing.translatePoints on marker enter sel
etpinard May 23, 2017
5fa1e97
add test case for nodes add/remove on redraw:false animations
etpinard May 23, 2017
9c35675
do not remove nodes in Drawing.translatePoints
etpinard May 23, 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
5 changes: 2 additions & 3 deletions src/components/drawing/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ drawing.setRect = function(s, x, y, w, h) {
s.call(drawing.setPosition, x, y).call(drawing.setSize, w, h);
};

/** Translate / remove node
/** Translate node
*
* @param {object} d : calcdata point item
* @param {sel} sel : d3 selction of node to translate
Expand All @@ -56,7 +56,7 @@ drawing.setRect = function(s, x, y, w, h) {
*
* @return {boolean} :
* true if selection got translated
* false if selection got removed
* false if selection could not get translated
*/
drawing.translatePoint = function(d, sel, xa, ya) {
Copy link
Contributor Author

@etpinard etpinard May 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

To my surprise, Drawing.translatePoint is called only in scatter/plot.js and box/plot.js:

image

Note that this patch does not affect the box/plot.js logic where points with non-numeric positions are filtered out of the data bind.

// put xp and yp into d if pixel scaling is already done
Expand All @@ -71,7 +71,6 @@ drawing.translatePoint = function(d, sel, xa, ya) {
sel.attr('transform', 'translate(' + x + ',' + y + ')');
}
} else {
sel.remove();
return false;
}

Expand Down
15 changes: 11 additions & 4 deletions src/traces/scatter/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,17 +419,20 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
var enter = join.enter().append('path')
.classed('point', true);

enter.call(Drawing.pointStyle, trace)
Copy link
Contributor Author

@etpinard etpinard May 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@n-riesco thanks very much 🎉

Your solution appears to work:
peek 2017-05-23 11-43

My only concern now is how this interacts with animations. I think the reason why translatePoints and pointStyle are called on enter selection and merge selection has to do with this block below where entering nodes have their opacity transitioned from 0 to 1. I suspect this commit will break this behavior (although the tests are passing) has the entering aren't positioned and styled properly before opacity transition cc @rreusser . It would be nice to write a test case here 😏

I'm thinking instead, we could keep those enter.call(), make Drawing.translatePoints not (and maybe never) remove nodes and make the merge selection .each block below (which comes after the .order()) handle the cases where nodes need to be removed.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the point you make about animations is important.
And the solution you propose (i.e. remove nodes only after we're done with the enter selection) is more robust.
👍 from me.

Copy link
Contributor

@n-riesco n-riesco May 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@etpinard after thinking a little bit about it. I'm still think that translatePoints and pointStyle should be applied to the merge selection to keep in sync DOM and trace.

I haven't tested it, but .order should take care of my concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still think that translatePoints and pointStyle should be applied to the merge

That's always been the case here and here.

.call(Drawing.translatePoints, xa, ya, trace);

if(hasTransition) {
enter.style('opacity', 0).transition()
enter
.call(Drawing.pointStyle, trace)
.call(Drawing.translatePoints, xa, ya, trace)
.style('opacity', 0)
.transition()
.style('opacity', 1);
}

var markerScale = showMarkers && Drawing.tryColorscale(trace.marker, '');
var lineScale = showMarkers && Drawing.tryColorscale(trace.marker, 'line');

join.order();
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need join.order() twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one for marker nodes, one for text nodes

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah right - didn't notice join got reassigned between 👍


join.each(function(d) {
var el = d3.select(this);
var sel = transition(el);
Expand All @@ -441,6 +444,8 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
if(trace.customdata) {
el.classed('plotly-customdata', d.data !== null && d.data !== undefined);
}
} else {
sel.remove();
}
});

Expand All @@ -460,6 +465,8 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
// it gets converted to mathjax
join.enter().append('g').classed('textpoint', true).append('text');

join.order();

join.each(function(d) {
var g = d3.select(this);
var sel = transition(g.select('text'));
Expand Down
138 changes: 138 additions & 0 deletions test/jasmine/tests/scatter_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,144 @@ describe('end-to-end scatter tests', function() {
.catch(fail)
.then(done);
});

function _assertNodes(ptStyle, txContent) {
var pts = d3.selectAll('.point');
var txs = d3.selectAll('.textpoint');

expect(pts.size()).toEqual(ptStyle.length);
expect(txs.size()).toEqual(txContent.length);

pts.each(function(_, i) {
expect(d3.select(this).style('fill')).toEqual(ptStyle[i], 'pt ' + i);
});

txs.each(function(_, i) {
expect(d3.select(this).select('text').text()).toEqual(txContent[i], 'tx ' + i);
});
}

it('should reorder point and text nodes even when linked to ids (shuffle case)', function(done) {
Plotly.plot(gd, [{
x: [150, 350, 650],
y: [100, 300, 600],
text: ['apple', 'banana', 'clementine'],
ids: ['A', 'B', 'C'],
mode: 'markers+text',
marker: {
color: ['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)']
},
transforms: [{
type: 'sort',
enabled: false,
target: [0, 1, 0]
}]
}])
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)'],
['apple', 'banana', 'clementine']
);

return Plotly.restyle(gd, 'transforms[0].enabled', true);
})
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 0, 255)', 'rgb(0, 255, 0)'],
['apple', 'clementine', 'banana']
);

return Plotly.restyle(gd, 'transforms[0].enabled', false);
})
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)'],
['apple', 'banana', 'clementine']
);
})
.catch(fail)
.then(done);
});

it('should reorder point and text nodes even when linked to ids (add/remove case)', function(done) {
Plotly.plot(gd, [{
x: [150, 350, null, 600],
y: [100, 300, null, 700],
text: ['apple', 'banana', null, 'clementine'],
ids: ['A', 'B', null, 'C'],
mode: 'markers+text',
marker: {
color: ['rgb(255, 0, 0)', 'rgb(0, 255, 0)', null, 'rgb(0, 0, 255)']
},
transforms: [{
type: 'filter',
enabled: false,
target: [1, 0, 0, 1],
operation: '=',
value: 1
}]
}])
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)'],
['apple', 'banana', 'clementine']
);

return Plotly.restyle(gd, 'transforms[0].enabled', true);
})
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 0, 255)'],
['apple', 'clementine']
);

return Plotly.restyle(gd, 'transforms[0].enabled', false);
})
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)'],
['apple', 'banana', 'clementine']
);
})
.catch(fail)
.then(done);
});

it('should smoothly add/remove nodes tags with *ids* during animations', function(done) {
Plotly.plot(gd, {
data: [{
mode: 'markers+text',
y: [1, 2, 1],
text: ['apple', 'banana', 'clementine'],
ids: ['A', 'B', 'C'],
marker: { color: ['red', 'green', 'blue'] }
}],
frames: [{
data: [{
y: [2, 1, 2],
text: ['apple', 'banana', 'dragon fruit'],
ids: ['A', 'C', 'D'],
marker: { color: ['red', 'blue', 'yellow'] }
}]
}]
})
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 128, 0)', 'rgb(0, 0, 255)'],
['apple', 'banana', 'clementine']
);

return Plotly.animate(gd, null, {frame: {redraw: false}});
})
.then(function() {
_assertNodes(
['rgb(255, 0, 0)', 'rgb(0, 0, 255)', 'rgb(255, 255, 0)'],
['apple', 'banana', 'dragon fruit']
);
})
.catch(fail)
.then(done);
});
});

describe('scatter hoverPoints', function() {
Expand Down