Bug 99343

Summary: [CSS Exclusions] Polygon edges should span colinear vertices
Product: WebKit Reporter: Hans Muller <giles_joplin>
Component: CSSAssignee: Hans Muller <giles_joplin>
Status: RESOLVED FIXED    
Severity: Normal CC: donggwan.kim, eric, krit, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 98548    
Bug Blocks:    
Attachments:
Description Flags
Patch
krit: review-, krit: commit-queue-
Patch none

Hans Muller
Reported 2012-10-15 11:32:24 PDT
At the moment, ExclusionPolygonEdges always (just) connect adjacent vertices. When a sequence of 3 or more colinear vertices appears, the polygon edge should be defined in terms of the first and last colinear vertices. This reduces the size of the overall ExclusionPolygon data structure, and will make looking up polygon edges - see m_edgeTree.allOverlaps() - more efficient. Note that this can be a little tricky when the colinear vertices wrap around the end of the polygon. For example this CSS polygon has 5 edges. polygon(10px 10px, 20px 10px, 30px 10px, 15px 30px, 0px 10px) Its ExclusionPolygon representation should be a simple triangle: polygon(0px 10px, 30px 10px, 15px 30px)
Attachments
Patch (12.19 KB, patch)
2012-11-01 21:25 PDT, Hans Muller 		krit: review-
krit: commit-queue-
DetailsFormatted DiffDiff
Patch (11.99 KB, patch)
2012-11-05 09:18 PST, Hans Muller 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Hans Muller
Comment 1 2012-11-01 21:25:27 PDT
Created attachment 171982 [details] Patch ExclusionPolygonEdges now span coincident and collinear vertices. Currently pairs of vertices are only considered coincident if their coordinates are exactly equal. Similarly, a vertex is only considered collinear with an edge if the area of the triangle defined by the three vertices is exactly zero. In the future it may be useful to relax the comparison with zero.
Dirk Schulze
Comment 2 2012-11-05 07:05:45 PST
Before I review this. Do you preserve the overall direction of the vertex?
Hans Muller
Comment 3 2012-11-05 07:53:27 PST
(In reply to comment #2) > Before I review this. Do you preserve the overall direction of the vertex? Yes.
Dirk Schulze
Comment 4 2012-11-05 08:23:48 PST
Comment on attachment 171982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171982&action=review Looks good, just some comments. > Source/WebCore/ChangeLog:24 > + (WebCore::ExclusionPolygon::ExclusionPolygon): Skip coincident,collinear vertices when building the list of edges. coincident,collinear space missing. > Source/WebCore/rendering/ExclusionPolygon.cpp:84 > + if (areCollinearPoints(vertexAt(vertexIndex1), vertexAt(vertexIndex2), vertexAt(vertexIndex3))) > + vertexIndex2 = vertexIndex3; > + else > + break; why not if (!(...)) break; vertexIndex2 = ... > Source/WebCore/rendering/ExclusionPolygon.cpp:103 > + if (!m_empty) { > + unsigned edgeIndex = 0; looks like you can return earlier here: if (m_empty) return; > Source/WebCore/rendering/ExclusionPolygon.cpp:129 > + if (!m_empty) { Ditto.
Hans Muller
Comment 5 2012-11-05 09:18:24 PST
Created attachment 172352 [details] Patch I've made the suggested changes.
Dirk Schulze
Comment 6 2012-11-05 09:44:28 PST
Comment on attachment 172352 [details] Patch r=me
WebKit Review Bot
Comment 7 2012-11-05 09:53:28 PST
Comment on attachment 172352 [details] Patch Clearing flags on attachment: 172352 Committed r133490: <http://trac.webkit.org/changeset/133490>
WebKit Review Bot
Comment 8 2012-11-05 09:53:32 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.