-
Notifications
You must be signed in to change notification settings - Fork 454
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
Improve Gradients #54
Conversation
@slightfoot @dnfield here is our WiP work on the feature:
Thanks |
@dnfield We found and fixed the second issue. The first one remains open but it might not be a huge issue as we don't think that that use would be common. Let us know if the There are some golden images broken in the tests, but we took a look on the new ones and we didn't see any noticeable/visible difference. So we committed the new ones. We also fixed a deprecation warning that was causing Travis to fail. The only thing that is missing is to fix the golden_widget pngs inside the test directory, as we didn't find a way to generate those files. |
@sroddy It's been awhile since I looked at the code, but isn't that the However saying that, I do remember talking to @dnfield about the getting the viewBox down in the hierarchy and I think @dnfield mentioned about changing the way the engine parses the xml anyway. So their might be another issue ticket or PR with that code to give you a basis for that change. @dnfield would be able to advise better there. |
@slightfoot from what I've seen, the |
lib/src/svg/xml_parsers.dart
Outdated
|| _isPercentage(rawR) | ||
|| _isPercentage(rawFx) | ||
|| _isPercentage(rawFy)) { | ||
// TODO: Support userSpaceOnUse with percentage values |
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.
I'm pretty sure we should be able to just use window.physicalSize
from dart:ui
.
I'd like to avoid the NoopShader
- it seems dangerous and avoidable in this case.
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.
I guess window.physicalSize
would be the whole window. Here we need the size of the SVG viewport, which is taken from the width
and height
properties of the root svg
element of the asset.
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.
What do you suggest instead of the NoopShader
? We can return a fully transparent LinearGradient
, or maybe just be able to implement this last missing mode, if you can help with this last issue :)
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.
Ahhh, that's the confusion.
In that case, we could add another parameter to the gradient parsers, something like parseLinearGradient(XmlElement el, Rect viewBox)
. the bounds
parameter passed into parseSvgElement
and parseSvgGroup
is actually the viewBox
. That could be passed through to parseDefs
, which could pass it to parseGradients
, which could pass it to parse[Linear|Radial]Gradient
.
Does that make sense to you?
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.
there are actually 2 viewBoxes
:
- the one of the SVG root element
- the one of the object that is being drawn
When dealing with gradients, user can describe them either by 1) (userSpaceOnUse
) or by 2)(objectBoundingBox
). It looks like (but I might be wrong) that the bounds
parameter refers to the inner object (e.g. the Rect or the Path node) w.r.t the root viewBox that is described as a Rect with origin 0,0 and size of the SVG root node.
Assuming that this is the case, the value that we are missing in order to be able to support the last case would be the SVG root node bounds, which we should either inject all the way down in every parse
method or maybe make available inside every XmlElement
as the root
node.
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.
I think I understand you - what I'm saying is that you can pass in that root viewBox bounds like so:
- Parse the viewBox from the SVG root
- Pass that viewBox as the bounds parameter to
parseSvgElement
- Pass that viewBox as a new parameter to
parseDefs
- Pass from item 3 to
parseGradient
- Pass from item 4 to
parseLinearGradient
andparseRadialGradient
1 and 2 are done. 3 to 5 could be added in this patch.
The inner functions returned in parseLinearGradient
and parseRadialGradient
should not be modified - they're going to continue to need the bounds they get today. However, the outter method can take the viewBox in and use that to figure out the userSpaceOnUse
percentages if necessary.
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.
ok, this makes sense! I'm going to try to patch the code and ping you back if I find some issues :)
Sorry I've been so slow on this - I'm finally starting to catch up. I like what you're doing here, just have a couple suggestions/questions. Let me know if you'll be able to work on those or you'd like me to try to take it over - and thanks for the work so far! |
…_svg into feature/gradients
d7701e9
to
a6c1ce4
Compare
@dnfield I've implemented the missing mode as per your suggestions. |
This looks great! The only other thing I want to do is a quick validation on the golden diffs. I've definitely seen it happen where a one or two byte change happens consistently without breaking the visual output, but I've also seen breaks that I initially thought were like that and actually were significantly altering the output. |
Thanks for the contribution! The differences seem minor even with a close look. Some of them may be version/skia related? |
This PR incorporates the work that has been done here #29 and adds further support for the
gradientUnits
property on both Radial and Linear gradients.