Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1084943003:
Add GrAAConvexTessellator class (Closed)
|
Created:
5 years, 8 months ago by robertphillips Modified:
5 years, 7 months ago Reviewers:
bsalomon CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdd GrAAConvexTessellator class
This CL adds a GrAAConvexTessellator class. It does not connect it to the GrAAConvexPathRenderer.
Committed: https://skia.googlesource.com/skia/+/84b008873b5bdf35eba9185038fb3b5580a8b9a8
Patch Set 1 #
Total comments: 23
Patch Set 2 : update #Patch Set 3 : update #
Total comments: 6
Patch Set 4 : address code review comments #Patch Set 5 : Fix warnings as errors #Patch Set 6 : Fix more warnings as errors #
Messages
Total messages: 24 (11 generated)
robertphillips@google.com changed reviewers: + bsalomon@google.com
https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... File src/gpu/GrAAConvexTessellator.cpp (right): https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:35: static SkScalar perp_intersect(const SkPoint& p0, const SkPoint& n0, could use a tiny comment here https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:47: static SkScalar abs_dist_from_line(const SkPoint& p0, const SkPoint& p1, const SkPoint& test) { Wondering if we could use SkPoint::distanceToLineBetweenSqd here and skip the sqrt (with a sqd kClose value)? https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:208: while ((verb = iter.next(pts)) != SkPath::kDone_Verb) { Do we really need to iterate here? We know all the points in the path are a convex polygon. Can't we just get the point array out of the path, transform it in bulk, and then walk it again removing degeneracies? https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:230: if (maxCross < cross) { maxCross = SkTMax(maxCross, cross); minCross = SkTMin(minCross, cross); ? https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:281: SkPoint::Side side; It wouldn't surprise me much if the asserts here were hit. I think the convexity checker has some tolerance. Might wind up taking the larger abs value of min/maxCross to decide the side and assert that the other isn't "too big." https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:309: // Flip flop back and forth between fRings[0] & fRings[1] Is this different than int nextRing = lastRing == &fRings[0] ? 1 : 0; fRings[nextRing].setReserve(fInitialRing.numPts0()); fRings[nextRing].setSide(fInitialRing.side()); fRings[nextRing].rewind(); return &fRings[nextRing]; ? https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:346: for (int cur = 0; cur < numPts; ++cur) { Maybe an explanation somewhere that what we're doing here is adding three points to the outset polygon at each vertex of the original polygon, one for each edge normal and one along the bisector. Triangles are added... https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:351: if (lastOut > -1 && duplicate_pt(temp, this->point(lastOut))) { "With a very shallow angle between two edges, the outset points may be very close and should fuse"? As discussed, can probably remove the lastOut tracking and just compare norm-outsets vs bisector outsets within an iteration. https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:410: int next = (cur + 1) % lastRing.numPts0(); wonder if we can avoid int mod. https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:440: // 'dst' is the index into the vertex array each point in the current poly maps to/ maybe say "... in the last ring maps to/transforms into in the next ring."? https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... File src/gpu/GrAAConvexTessellator.h (right): https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.h:27: class GrRing { Seems like Ring should be a nested class of GrAACT and could be forward decl in the .h and defined in the .cpp? https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.h:203: int addPt(const SkPoint& pt, SkScalar depth, bool movable); comment about what movable means?
So ... there is a much more clever version of this here (https://codereview.chromium.org/1120023003/) which has a priority queue, only incrementally updates the points as it progresses, only recomputes the new bisectors as needed but is very numerically unstable. This version is brute force and inelegant but works. Note that of the 141 paths in the convex-lineonly-paths GM, 19 are shrunk to a single point, 115 need only 1 ring and the remaining 7 need 2 rings. https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... File src/gpu/GrAAConvexTessellator.cpp (right): https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:35: static SkScalar perp_intersect(const SkPoint& p0, const SkPoint& n0, On 2015/04/24 15:44:44, bsalomon wrote: > could use a tiny comment here Done. https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:47: static SkScalar abs_dist_from_line(const SkPoint& p0, const SkPoint& p1, const SkPoint& test) { Done. The new version uses the vector computed for the normals. https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:208: while ((verb = iter.next(pts)) != SkPath::kDone_Verb) { I've added a TODO. I think we would need a new entry point on path. https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:230: if (maxCross < cross) { On 2015/04/24 15:44:44, bsalomon wrote: > maxCross = SkTMax(maxCross, cross); > minCross = SkTMin(minCross, cross); > ? Done. https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:281: SkPoint::Side side; I think the colinear cleanup pass should remove the slightly small concavities so I would like to keep this hard for now. https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:309: // Flip flop back and forth between fRings[0] & fRings[1] On 2015/04/24 15:44:44, bsalomon wrote: > Is this different than > > int nextRing = lastRing == &fRings[0] ? 1 : 0; > fRings[nextRing].setReserve(fInitialRing.numPts0()); > fRings[nextRing].setSide(fInitialRing.side()); > fRings[nextRing].rewind(); > return &fRings[nextRing]; > > ? Done. https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:346: for (int cur = 0; cur < numPts; ++cur) { On 2015/04/24 15:44:44, bsalomon wrote: > Maybe an explanation somewhere that what we're doing here is adding three points > to the outset polygon at each vertex of the original polygon, one for each edge > normal and one along the bisector. Triangles are added... Done. https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:351: if (lastOut > -1 && duplicate_pt(temp, this->point(lastOut))) { On 2015/04/24 15:44:44, bsalomon wrote: > "With a very shallow angle between two edges, the outset points may be very > close and should fuse"? > > As discussed, can probably remove the lastOut tracking and just compare > norm-outsets vs bisector outsets within an iteration. Done-ish. I think it is still useful to track the last perpendicular point in order to create the trailing edge triangles but I have removed the comparison of the last perpendicular outset pt of the last point with the first perpendicular outset pt of the current point. https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.cpp:440: // 'dst' is the index into the vertex array each point in the current poly maps to/ On 2015/04/24 15:44:44, bsalomon wrote: > maybe say "... in the last ring maps to/transforms into in the next ring."? Done. https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... File src/gpu/GrAAConvexTessellator.h (right): https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.h:27: class GrRing { On 2015/04/24 15:44:44, bsalomon wrote: > Seems like Ring should be a nested class of GrAACT and could be forward decl in > the .h and defined in the .cpp? Done. The GrAACT does need to know the size but I have nested it inside. https://codereview.chromium.org/1084943003/diff/1/src/gpu/GrAAConvexTessellat... src/gpu/GrAAConvexTessellator.h:203: int addPt(const SkPoint& pt, SkScalar depth, bool movable); On 2015/04/24 15:44:44, bsalomon wrote: > comment about what movable means? Done.
lgtm https://codereview.chromium.org/1084943003/diff/40001/src/gpu/GrAAConvexTesse... File src/gpu/GrAAConvexTessellator.cpp (right): https://codereview.chromium.org/1084943003/diff/40001/src/gpu/GrAAConvexTesse... src/gpu/GrAAConvexTessellator.cpp:635: const SkTDArray<SkVector>& bisectors) { align? https://codereview.chromium.org/1084943003/diff/40001/src/gpu/GrAAConvexTesse... File src/gpu/GrAAConvexTessellator.h (right): https://codereview.chromium.org/1084943003/diff/40001/src/gpu/GrAAConvexTesse... src/gpu/GrAAConvexTessellator.h:61: class GrCandidateVerts { CandidateVerts? https://codereview.chromium.org/1084943003/diff/40001/src/gpu/GrAAConvexTesse... src/gpu/GrAAConvexTessellator.h:121: class GrRing { Ring?
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1084943003/#ps60001 (title: "address code review comments")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1084943003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1084943003/#ps80001 (title: "Fix warnings as errors")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1084943003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1084943003/#ps100001 (title: "Fix more warnings as errors")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1084943003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robertphillips@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1084943003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/84b008873b5bdf35eba9185038fb3b5580a8b9a8
Message was sent while issue was closed.
https://codereview.chromium.org/1084943003/diff/40001/src/gpu/GrAAConvexTesse... File src/gpu/GrAAConvexTessellator.cpp (right): https://codereview.chromium.org/1084943003/diff/40001/src/gpu/GrAAConvexTesse... src/gpu/GrAAConvexTessellator.cpp:635: const SkTDArray<SkVector>& bisectors) { On 2015/05/05 17:13:42, bsalomon wrote: > align? Done. https://codereview.chromium.org/1084943003/diff/40001/src/gpu/GrAAConvexTesse... File src/gpu/GrAAConvexTessellator.h (right): https://codereview.chromium.org/1084943003/diff/40001/src/gpu/GrAAConvexTesse... src/gpu/GrAAConvexTessellator.h:61: class GrCandidateVerts { On 2015/05/05 17:13:42, bsalomon wrote: > CandidateVerts? Done. https://codereview.chromium.org/1084943003/diff/40001/src/gpu/GrAAConvexTesse... src/gpu/GrAAConvexTessellator.h:121: class GrRing { On 2015/05/05 17:13:42, bsalomon wrote: > Ring? Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
