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

Fix stacked area layering and deletion bugs #3005

Merged
merged 4 commits into from
Sep 14, 2018
Merged

Conversation

alexcjohnson
Copy link
Collaborator

Lumped fixes for #3002 and #3003 into one PR, since they touch the same code.

  • Issue with fill in stackgroups #3003 had two related components. First, which trace is "next" when we fill between two traces (and if there even is a next or should we fill to zero), stacked or otherwise, needs to take account of stackgroup. The "next" trace must be in the same stack group, or no stack group. Second, once you've done this, it doesn't make much sense to preserve the trace ordering as it is in gd.data exactly. Traces in the same stack group, or that fill to each other, need to be in consecutive order - or you can get weird effects like Issue with fill in stackgroups #3003 (comment). As mentioned there, I chose to push traces only backward to meet the first in their stack, but otherwise to preserve the existing order as much as possible. The new mock demonstrates this.
  • Error when deleting traces with stackgaps #3002 pointed out that we were actually redrawing points even if we were about to remove their trace entirely! So not only does this work now, but deleting scatter traces will be faster in general now.

and along the way speed up trace deletion in general, by not redrawing
all the points in the deleted trace immediately before removing them!
@@ -72,7 +72,7 @@ module.exports = function plot(gd, plotinfo, cdscatter, scatterLayer, transition
});
});
} else {
scatterLayer.selectAll('g.trace').each(function(d, i) {
join.each(function(d, i) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@etpinard it doesn't seem like animations can remove traces, is that correct? That's what it looks like, since I don't see a join.exit() associated with them (since isFullReplot = !transitionOpts)... yet the comment above implies that you can add traces:

// Must run the selection again since otherwise enters/updates get grouped together
// and these get executed out of order. Except we need them in order!

So... do I need to worry about animations deleting traces or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

do I need to worry about animations deleting traces or not?

Right, removing traces with 'frame.redraw': false (i.e. without a replot at the end of the animation), doesn't work currently:

peek 2018-09-13 16-17

https://codepen.io/etpinard/pen/ZMRReB

so I don't think you broke anything here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah Ricky wrote an issue about this #932

@alexcjohnson
Copy link
Collaborator Author

@etpinard 3rd time's a charm, but the intermittent error in test-jasmine2 (that came in with the context loss event PR?) has been rearing its ugly head...

@etpinard
Copy link
Contributor

  • First, which trace is "next" when we fill between two traces (and if there even is a next or should we fill to zero), stacked or otherwise, needs to take account of stackgroup. The "next" trace must be in the same stack group, or no stack group. Second, once you've done this, it doesn't make much sense to preserve the trace ordering as it is in gd.data exactly. Traces in the same stack group, or that fill to each other, need to be in consecutive order - or you can get weird effects like #3003 (comment). As mentioned there, I chose to push traces only backward to meet the first in their stack, but otherwise to preserve the existing order as much as possible. The new mock demonstrates this.

Nice solution! Would you mind adding a few words about this behavior in the fill attribute description?

@alexcjohnson
Copy link
Collaborator Author

Would you mind adding a few words about this behavior in the fill attribute description?

Good call, I added it to both fill and stackgroup -> fe0c988

@etpinard
Copy link
Contributor

nicely done 💃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants