exyte / Macaw

Powerful and easy-to-use vector graphics Swift library with SVG support
MIT License
6.02k stars 557 forks source link

Shape.bounds returns incorrect rectangle #623

Open mhdhejazi opened 5 years ago

mhdhejazi commented 5 years ago

Consider the following simple shape: <path d="M160 80.3848C160 187.051 0 187.051 0 80.3848C0 -26.2819 160 -26.2819 160 80.3848Z" fill="#C4C4C4"/> image

As you can see the size is 160x160, but when I call Shape.bounds it returns: x: 0.000000, y: -26.281900, w: 160.000000, h: 213.332900

If we check the curves of this path closely we can understand what's happening. The returned rectangle is bigger because it also includes the control points: image

I checked how the bounds are calculated and found out that the native method CGContext. boundingBoxOfPath is the culprit since it calculates the bounding box for all the points including the control points:

CGContext.boundingBoxOfPath

The bounding box is the smallest rectangle completely enclosing all points in a path, including control points for Bézier cubic and quadratic curves.

While CGPath.boundingBoxOfPath, despite having the exact same name, doesn't include the control points in the calculation:

CGPath.boundingBoxOfPath

The path bounding box is the smallest rectangle completely enclosing all points in the path but not including control points for Bézier and quadratic curves.

I tried using ctx.path?.boundingBoxOfPath instead of ctx.boundingBoxOfPath here: https://github.com/exyte/Macaw/blob/3acbaffa97f0297cde7eb4fce40cdc2e4ca0f56f/Source/model/scene/Shape.swift#L68 and it returned the correct result: x: 0.000000, y: 0.384775, w: 160.000000, h: 159.999675

Searching for boundingBoxOfPath in this repo I found out that Path.bounds() actually uses the correct method: https://github.com/exyte/Macaw/blob/3acbaffa97f0297cde7eb4fce40cdc2e4ca0f56f/Source/model/geom2d/Path.swift#L15-L17

So I think this method can be used as a workaround until this is fixed (instead of Shape.bounds, one can use Shape.form.bounds()).

ystrot commented 4 years ago

Hi @MhdHejazi,

Thanks for detailed description! I've pushed a fix.

ystrot commented 4 years ago

Actually it's not that easy. We have simple sample in tests:

<path d="M10,10C40,10,65,10,95,80S150,150,180,80"/>

And on this sample both CGPath.boundingBoxOfPath and CGContext. boundingBoxOfPath works incorrectly (blue is path bounds, red is context bounds):

Screenshot 2019-11-29 at 17 58 56

In this case we need to use context, because at least it returns a bounding box. It's not minimal, but path bounding box completely wrong. So I'll revert this change and we need to wait Apple will fix it.

mhdhejazi commented 4 years ago

I see. But it's confusing that Shape.bounds doesn't return the same value as Path.bounds(). The former returns the bigger rectangle in your example, while the later returns the smaller one.

ystrot commented 4 years ago

Agree, but it's the best we can do. In Path.bounds we don't have context, so we need to use path method.