Restructure processing of clip, transform, and group #223
Conversation
rsheeter
force-pushed
the
ungroup
branch
5 times, most recently
from
00e1dd1
to
d2eabf3
Compare
rsheeter
force-pushed
the
ungroup
branch
4 times, most recently
from
1b3c035
to
b217ca2
Compare
rsheeter
force-pushed
the
ungroup
branch
2 times, most recently
from
578c26e
to
a32f943
Compare
There was a problem hiding this comment.
very nice! I only started reviewing this, but I can already tell it's improving things a lot. E.g. the examples in #110 both work with this patch
rsheeter
force-pushed
the
ungroup
branch
2 times, most recently
from
7523b03
to
69f069d
Compare
rsheeter
force-pushed
the
ungroup
branch
2 times, most recently
from
93e94ef
to
3c8a1bc
Compare
|
For now I have commented out objectBoundingBox tests. I do think we should support them. |
rsheeter
force-pushed
the
ungroup
branch
2 times, most recently
from
3cd1b17
to
1da7064
Compare
rsheeter
force-pushed
the
ungroup
branch
3 times, most recently
from
2601ae5
to
0f6fa0f
Compare
| paths = list(self._stroke(paths[0])) | ||
|
|
||
| if context.transform != Affine2D.identity(): | ||
| paths = [p.apply_transform(context.transform) for p in paths] |
There was a problem hiding this comment.
you're applying the transforms on the paths and then you are intersecting them with the similarly transformed clip-paths. It might be simpler (and will produce the same result) if instead you intersect the untransformed clip-paths with the untransformed paths, and then transform the result of the intersection.
| keep_group = len(el) > 1 and opacity > 0.0 and opacity < 1.0 | ||
|
|
||
| if keep_group: | ||
| el.attrib.clear() |
There was a problem hiding this comment.
clear() can be dangerous, we might have forgotten something, how about you just assert that the only attribute left on <g> is opacity?
does perhaps _inherit_attrib already do the all checking for us?
There was a problem hiding this comment.
I didn't actually clear them off but as opacity being the only acceptable group attribute in picosvg output I think clear is right?
rsheeter
force-pushed
the
ungroup
branch
2 times, most recently
from
6669e21
to
48c99ef
Compare
Convert "objectBoundingBox" to "userSpaceOnUse" before transforming gradients
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
|
Hm.. looks like dataclasses backport for Python 3.6 does not like when attributes are NamedTuples (like Affine2D) and one uses dataclasses.asdict() on them: ericvsmith/dataclasses#141 see CI failure here: https:/googlefonts/picosvg/pull/223/checks?check_run_id=2720877391 how about we kill python 3.6 support? :) |
|
(the reason I didn't catch this problem in #230 is because the base branch there was not |
in this case, calling asdict was not actually needed. By replacing that with a simple getattr() we fix error with python3.6. Hopefully the dataclasses backport will one day fix the bug #223 (comment)
+1 |
Try to correctly handle a somewhat wider range of scenarios. Causes groups to be retained when opacity cannot simply be pushed down. Generate transformed gradients somewhat more correctly.
Do not submit until nanoemoji is ready to handle groups.
Fixes #78. Fixes #200. Fixes #110.