Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SVGLoader: Improve round rect corners approximation #22132

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

nkrkv
Copy link
Contributor

@nkrkv nkrkv commented Jul 14, 2021

Hello!

The current approximation of SVG round rects with Bezier curves is too rough when corner radii are large enough and comparable to the dimensions. Here’s a simple Inkscape example where border-radius is almost ½ of the rect height:

round-rect.svg.zip

It renders in a browser like this:

image

And loads by THREE.js like this:

image

Note the curve “overruns”. I tried to follow the current algorithm on paper and it flaws in several ways:

  • The 2 * rx and 2 * ry offsets is a mistake. They should not be doubled. That’s the reason for overrun
  • The control points of Bezier curve are trivially placed in corners. There are better ways to arrange them to approximate an ellipse, i.e. to minimize the approximation error (referred in comment).

I’ve adjusted the implementation and the rect now looks like:

image


BTW, is there any reason why ShapePath does not provide methods like arc and ellipse? I see they can be trivially implemented as a proxy to Path like lineTo or bezierCurveTo. It would allow avoiding approximation so that the corners may be implemented as true ellipse arcs. I suspect though there’s a reason, some dependency which does not allow to do it easily. Could you clarify this point? If it just “no time to bother”, I could try to do it.

@mrdoob mrdoob added this to the r131 milestone Jul 16, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jul 16, 2021

BTW, is there any reason why ShapePath does not provide methods like arc and ellipse? I see they can be trivially implemented as a proxy to Path like lineTo or bezierCurveTo. It would allow avoiding approximation so that the corners may be implemented as true ellipse arcs. I suspect though there’s a reason, some dependency which does not allow to do it easily. Could you clarify this point? If it just “no time to bother”, I could try to do it.

There's no reason no. Feel free to give it a go! 🙏

@mrdoob mrdoob merged commit 5732f19 into mrdoob:dev Jul 16, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jul 16, 2021

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Jul 20, 2021

/fyi @yomboprime

@yomboprime
Copy link
Collaborator

BTW, is there any reason why ShapePath does not provide methods like arc and ellipse? I see they can be trivially implemented as a proxy to Path like lineTo or bezierCurveTo. It would allow avoiding approximation so that the corners may be implemented as true ellipse arcs. I suspect though there’s a reason, some dependency which does not allow to do it easily. Could you clarify this point? If it just “no time to bother”, I could try to do it.

There's no reason no. Feel free to give it a go! pray

Yes, I think that a simple approximation was implemented because it was at hand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants