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

Ellipse uses wrong extents calculation #193

Open
Taylor-Reid opened this issue Feb 13, 2023 · 1 comment
Open

Ellipse uses wrong extents calculation #193

Taylor-Reid opened this issue Feb 13, 2023 · 1 comment

Comments

@Taylor-Reid
Copy link

From the current code base:
<Value Condition="this.ContainsAngle(0.0)">Center + MajorAxis</Value> <Value Condition="this.ContainsAngle(90.0)">Center + new DxfPoint(0.0, MajorAxis.Length * MinorAxisRatio, 0.0)</Value <Value Condition="this.ContainsAngle(180.0)">Center - MajorAxis</Value> <Value Condition="this.ContainsAngle(270.0)">Center + new DxfPoint(0.0, -MajorAxis.Length * MinorAxisRatio, 0.0)</Value>
Ellipse seems to use the same 0,90,180,270 degree checks when building up the extents as the arc does, but Ellipse parameters are in radians, so it needs to check for radians.
Additionally, those hard code orthogonal points seem to make the assumption that the minor axis is always along the Y axis direction, which isn't necessarily the case.

@brettfo
Copy link
Member

brettfo commented Feb 13, 2023

Good catch, the calls to ContainsAngle() should be in radians and not degrees.

As for axis assumptions, I think the proper fix would be to cross the extrusion direction (also the normal) with the major axis and ensure the correct length and only then add that to the center point. This would be a much better solution, but still not quite correct, because the extents of a rotated ellipse aren't necessarily the axis endpoints; there could be part of the curve that still sticks out to the side, but that level of math is beyond my capabilities.

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

No branches or pull requests

2 participants