Bug 34366 - [OpenVG] Implement support for paths
Summary: [OpenVG] Implement support for paths
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 35445
Blocks: 33987
  Show dependency treegraph
 
Reported: 2010-01-29 18:37 PST by Jakob Petsovits
Modified: 2010-03-01 09:42 PST (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jakob Petsovits 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!
Comment 1 Jakob Petsovits 2010-01-29 18:42:58 PST
Created attachment 47755 [details]
Patch
Comment 2 Ariya Hidayat 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.
Comment 3 Jakob Petsovits 2010-02-17 08:29:37 PST
Created attachment 48904 [details]
Patch
Comment 4 Jakob Petsovits 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.
Comment 5 Mun Gwan-gyeong 2010-02-18 00:55:04 PST
Hi~ Jakob Petsovits. I'm curious about how to build and test OpenVG graphics backend.
Comment 6 Dirk Schulze 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.
Comment 7 Dirk Schulze 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.
Comment 8 Jakob Petsovits 2010-02-26 13:38:45 PST
Created attachment 49632 [details]
Patch
Comment 9 Jakob Petsovits 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 :)
Comment 10 Dirk Schulze 2010-03-01 09:17:33 PST
Comment on attachment 49632 [details]
Patch

looks great r=me
Comment 11 Jakob Petsovits 2010-03-01 09:42:09 PST
Committed r55371: <http://trac.webkit.org/changeset/55371>