-
Notifications
You must be signed in to change notification settings - Fork 158
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
Fixes fill animation path #56
base: master
Are you sure you want to change the base?
Conversation
Noticed when fill wasn't enabled, the line would draw from coordinates 0,0 within the sparkView. commit 8b58884 fixes this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fix! I hadn't thought about closing the path here before passing into the animator!
this.renderPath.reset(); | ||
this.renderPath.addPath(animationPath); | ||
this.renderPath.rLineTo(0, 0); | ||
this.renderPath.rLineTo(0, fillType == FillType.UP ? -getHeight() : fillY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit, but i think this can be simplified a bit to avoid needing to check for FillType.Up
:
// takes us straight up or down to the fill edge
this.renderPath.rLineTo(0, fillEdge);
// takes us straight over to the left edge
this.renderPath.lineTo(0, fillEdge);
// closes back to the path's start
this.renderPath.close();
you end up having to check the fill-edge for null, and only do those operations if non-null. but i think it reads a bit cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking back over this, it actually seemed to be an issue with the getFillEdge()
method, specifically:
case FillType.UP:
return (float) getPaddingTop();
which wasn't taking into account the view height. Moving -getHeight()
into this method and removing the ternary seemed to result in the same desired behavior. Though I'm not familiar with what else is using getFillEdge
but local testing showed promising results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I don't think we want the fill edge to be -getHeight()
for the FillType.UP
case. Conceptually, the fill edge should be the y value in drawing coordinates that we want to travel to to close the path. In the case of UP
, it should be 0
since the top edge of a view in Android is coordinate 0
. Then we have to take the padding into account, so we add paddingTop to that.
With that in mind, I think my initial code suggestion was wrong. We actually want something like:
// line up or down to the fill edge
sparkPath.lineTo(lastX, fillEdge);
// line straight left to far edge of the view
sparkPath.lineTo(getPaddingStart(), fillEdge);
// closes line back on the first point
sparkPath.close();
Which is from SparkView.populatePath()
. Unfortunately, we don't have lastX handy. Also the lastX may be different from one animation to another. So, I think we may want to move the path closing/filling to the animators themselves.
8b58884
to
dc75635
Compare
CI is failing since #64 got merged, classic case of the:
🙄 |
Closes #48
This PR fixes the "zigzag" animation when the fill is enabled. In the gif below, you'll notice the line stroke consumes the entire shape rather than just the top. Not sure if this is okay or a fillPath should be created that uses a separate
Paint
instance without stroke.