Handling of SkPath* return parameters is inconsistent
Project Member Reported by email@example.com, Jan 3 2012
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)
Aug 20 2014,
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?
Aug 20 2014,
Dec 7 2015,
Feb 25 2016,
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.
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;
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