Summary: | [OpenVG] Implement support for paths | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jakob Petsovits <jpetsovits> | ||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | elongbug, joone, krit, mario.bensi, skyul | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 35445 | ||||||||||
Bug Blocks: | 33987 | ||||||||||
Attachments: |
|
Description
Jakob Petsovits
2010-01-29 18:37:12 PST
Created attachment 47755 [details]
Patch
Comment on attachment 47755 [details] Patch Some minor remarks. > +Path* PainterOpenVG::currentPath() > +{ > + return m_currentPath; > +} Shouldn't this be a const? > +bool Path::contains(const FloatPoint& point, WindRule rule) const > +{ > + notImplemented(); > + > + return boundingRect().contains(point); > +} Which one is correct? > +bool Path::strokeContains(StrokeStyleApplier* applier, const FloatPoint& point) const > +{ > + notImplemented(); > + > + return (const_cast<Path*>(this))->strokeBoundingRect().contains(point); > +} idem. > +FloatRect Path::strokeBoundingRect(StrokeStyleApplier* applier) > +{ > + notImplemented(); > + > + return boundingRect(); > +} idem. > +String Path::debugString() const > +{ > + String debugString = ""; > + > + /* > + m_path->makeCompatibleContextCurrent(); > + VGint numSegments = vgGetParameteri(m_path->vgPath(), VG_PATH_NUM_SEGMENTS); > + > + for (int i = 0; i < numSegments; i++) { > + // Hm, OpenVG provides no means to retrieve path segment information? > + // This is a bit unfortunate, we might need to store the segments in > + // memory if we want to implement this function properly. > + } > + */ > + > + return debugString; > +} Maybe just notImplemented() with the said comment? > +void Path::apply(void* info, PathApplierFunction function) const > +{ > + /* > + m_path->makeCompatibleContextCurrent(); > + VGint numSegments = vgGetParameteri(m_path->vgPath(), VG_PATH_NUM_SEGMENTS); > + > + for (int i = 0; i < numSegments; i++) { > + // Hm, OpenVG provides no means to retrieve path segment information? > + // This is *very* unfortunate, we might need to store the segments in > + // memory if we want to implement this function properly. > + // See http://www.khronos.org/message_boards/viewtopic.php?f=6&t=1887 > + // Bleh. > + } > + */ > +} idem. Created attachment 48904 [details]
Patch
Thanks Ariya, the updated the patch incorporates the above points. (The questions you brought up are now answered in comments.) I am aware that the OpenVG backend needs to switch from TransformationMatrix to AffineTransform, my plan was to tackle that after this patch has been landed. Hi~ Jakob Petsovits. I'm curious about how to build and test OpenVG graphics backend. Comment on attachment 48904 [details] Patch > +PlatformPathOpenVG::PlatformPathOpenVG(const PlatformPathOpenVG& other) > + : SharedResourceOpenVG() > + , m_currentPoint(other.m_currentPoint) > + , m_subpathStartPoint(other.m_subpathStartPoint) > +{ > + createPath(); > + // makeCompatibleContextCurrent(); // called by createPath(), not necessary > + vgAppendPath(m_vgPath, other.m_vgPath); > + ASSERT_VG_NO_ERROR(); > +} There shouldn't be two comments in one line. > + > +PlatformPathOpenVG& PlatformPathOpenVG::operator=(const PlatformPathOpenVG& other) > +{ > + if (&other != this) { > + clear(); > + // makeCompatibleContextCurrent(); // called by clear(), not necessary > + vgAppendPath(m_vgPath, other.m_vgPath); > + ASSERT_VG_NO_ERROR(); > + } > + return *this; > +} The same here. > +bool Path::contains(const FloatPoint& point, WindRule rule) const > +{ > + notImplemented(); > + > + // OpenVG has no path-contains function, so for now we approximate by > + // using the bounding rect of the path. > + return boundingRect().contains(point); > +} If I undersand Ariya correctly, ha wants you to either choose noImplemented() or the comment with the immedate code. Personaly, I would like to see the OpenVG port fixed, so that you can use AffineTransform here. I'm blameable for this breakage with the AffineTransform implementation, so I would like to help on fixing the issues. Comment on attachment 48904 [details] Patch > +PlatformPathOpenVG::PlatformPathOpenVG(const PlatformPathOpenVG& other) > + : SharedResourceOpenVG() > + , m_currentPoint(other.m_currentPoint) > + , m_subpathStartPoint(other.m_subpathStartPoint) > +{ > + createPath(); > + // makeCompatibleContextCurrent(); // called by createPath(), not necessary > + vgAppendPath(m_vgPath, other.m_vgPath); > + ASSERT_VG_NO_ERROR(); > +} Either delete the comments or, if you think it is necessary for the explanation, use two lines. > +void Path::transform(const TransformationMatrix& transform) > +{ > + PlatformPathOpenVG* dst = new PlatformPathOpenVG(); > + // dst->makeCompatibleContextCurrent(); // done by platform path constructor > + PainterOpenVG::transformPath(dst->vgPath(), m_path->vgPath(), transform); > + delete m_path; > + m_path = dst; > + > + m_path->m_currentPoint = transform.mapPoint(m_path->m_currentPoint); > + m_path->m_subpathStartPoint = transform.mapPoint(m_path->m_subpathStartPoint); > +} same here You should also make a explanation why Path::isEmpty is also empty for a moveTo, either with a link to the HTML 5 spec or technical reasons. The current implementation differs from other platforms. I realy would like to see AffineTransform instead of TransformationMatrix. r- mainly because of TransformationMatrix and Path:isEmpty. Created attachment 49632 [details]
Patch
Above patch uses AffineTransform (depending on bug 35445 which converts the already landed parts of the OpenVG backend), rewrites the comments so no double-commenting is done, and implements isEmpty() in the same way as other ports do. Hope that's good enough now :) Comment on attachment 49632 [details]
Patch
looks great r=me
Committed r55371: <http://trac.webkit.org/changeset/55371> |