New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 7 users
Status: New
Owner:
Cc:
Area: ----
Priority: Low
Type: Idea



Sign in to add a comment
Handling of SkPath* return parameters is inconsistent
Project Member Reported by tomhudson@google.com, Jan 3 2012 Back to list
Some classes call SkPath::reset() or rewind() before writing into a SkPath parameter; others do not, appending instead of overwriting.

This is distributed throughout the interface (publicly: SkPathEffect, SkPaint, SkPath, SkPathMeasure, SkRegion, SkScalerContext, SkStroke; presumably others internally). Is it worth the effort to make it consistent?

(inspired by http://code.google.com/p/skia/issues/detail?id=356)

 
Project Member Comment 1 by hcm@google.com, Aug 20 2014
Cc: hcm@google.com
Labels: -Type-Review -Priority-Low Type-Idea Priority-Icebox
Seems we are not getting to this one- do our best to pick a best practice, share with the team, and implement consistently moving fwd?
Project Member Comment 2 by reed@google.com, Aug 20 2014
Labels: -Priority-Icebox Priority-Low
Owner: reed@google.com
Project Member Comment 3 by hcm@google.com, Dec 7 2015
Labels: Hotlist-Fixit
Project Member Comment 4 by caryclark@google.com, Feb 25 2016
Owner: caryclark@google.com
Proposal: change the documentation of all public interfaces that append to the path  that they do so.

Alternatively: Document that these functions expect empty paths as receptacles and  assert if the path is not empty.
Project Member Comment 5 by reed@google.com, Mar 2 2016
Some concrete examples:

    /**
     *  Returns true if the region is non-empty, and if so, appends the
     *  boundary(s) of the region to the specified path.
     *  If the region is empty, returns false, and path is left unmodified.
     */
    bool SkRegion::getBoundaryPath(SkPath* path) const;

    /**
     *  Applies any/all effects (patheffect, stroking) to src, returning the
     *  result in dst. The result is that drawing src with this paint will be
     *  the same as drawing dst with a default paint (at least from the
     *  geometric perspective).
     *
     *  @param src  input path
     *  @param dst  output path (may be the same as src)
     *  @param cullRect If not null, the dst path may be culled to this rect.
     *  @param resScale If > 1, increase precision, else if (0 < res < 1) reduce precision
     *              in favor of speed/size.
     *  @return     true if the path should be filled, or false if it should be
     *              drawn with a hairline (width == 0)
     */
    bool SkPaint::getFillPath(const SkPath& src, SkPath* dst, const SkRect* cullRect,
                     SkScalar resScale = 1) const;

    /**
     *  Given a src path (input) and a stroke-rec (input and output), apply
     *  this effect to the src path, returning the new path in dst, and return
     *  true. If this effect cannot be applied, return false and ignore dst
     *  and stroke-rec.
     *
     *  The stroke-rec specifies the initial request for stroking (if any).
     *  The effect can treat this as input only, or it can choose to change
     *  the rec as well. For example, the effect can decide to change the
     *  stroke's width or join, or the effect can change the rec from stroke
     *  to fill (or fill to stroke) in addition to returning a new (dst) path.
     *
     *  If this method returns true, the caller will apply (as needed) the
     *  resulting stroke-rec to dst and then draw.
     */
    virtual bool SkPathEffect::filterPath(SkPath* dst, const SkPath& src,
                            SkStrokeRec*, const SkRect* cullR) const = 0;

Project Member Comment 6 by reed@google.com, Mar 2 2016
Seems like we have some options:

- sometimes append may be fine, but perhaps not always...
    - filterPath seems (to me) like it should "create" its result, and not append it

Perhaps we could ask ourselves
A) when might the API be reorg'd to just return its path?
B) when is it useful to pass in the dst?

In the (A) case, perhaps we should assert that the dst is already empty, so it *could* be reformulated as a return value.

In the (B) case, lets see if its interesting/useful to pass in a non-empty path and append to it.

Of course, we could always require empty, and force the caller to manually append afterwards if they wished...
Sign in to add a comment