WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
34366
[OpenVG] Implement support for paths
https://bugs.webkit.org/show_bug.cgi?id=34366
Summary
[OpenVG] Implement support for paths
Jakob Petsovits
Reported
2010-01-29 18:37:12 PST
Next patch in my OpenVG graphics backend series, adding a VGPath-based implementation of WebCore::Path (and everything that it comes with). See the commit message for more details. Review and enjoy!
Attachments
Patch
(32.75 KB, patch)
2010-01-29 18:42 PST
,
Jakob Petsovits
no flags
Details
Formatted Diff
Diff
Patch
(32.94 KB, patch)
2010-02-17 08:29 PST
,
Jakob Petsovits
no flags
Details
Formatted Diff
Diff
Patch
(32.70 KB, patch)
2010-02-26 13:38 PST
,
Jakob Petsovits
krit
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Jakob Petsovits
Comment 1
2010-01-29 18:42:58 PST
Created
attachment 47755
[details]
Patch
Ariya Hidayat
Comment 2
2010-02-08 22:46:11 PST
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.
Jakob Petsovits
Comment 3
2010-02-17 08:29:37 PST
Created
attachment 48904
[details]
Patch
Jakob Petsovits
Comment 4
2010-02-17 08:39:09 PST
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.
Mun Gwan-gyeong
Comment 5
2010-02-18 00:55:04 PST
Hi~ Jakob Petsovits. I'm curious about how to build and test OpenVG graphics backend.
Dirk Schulze
Comment 6
2010-02-22 09:10:10 PST
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.
Dirk Schulze
Comment 7
2010-02-25 12:52:06 PST
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.
Jakob Petsovits
Comment 8
2010-02-26 13:38:45 PST
Created
attachment 49632
[details]
Patch
Jakob Petsovits
Comment 9
2010-02-26 13:41:55 PST
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 :)
Dirk Schulze
Comment 10
2010-03-01 09:17:33 PST
Comment on
attachment 49632
[details]
Patch looks great r=me
Jakob Petsovits
Comment 11
2010-03-01 09:42:09 PST
Committed
r55371
: <
http://trac.webkit.org/changeset/55371
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug