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

Calculation of number of sides based on $fs is slightly incorrect #5101

Open
jordanbrown0 opened this issue Apr 24, 2024 · 2 comments
Open

Comments

@jordanbrown0
Copy link
Contributor

Describe the bug
When $fs is used to calculate the number of sides for a circle, the expression used is (simplified):

ceil(r * 2 * pi / $fs)

But that calculates the number of sides to have an arc length of $fs, and $fs is defined to be the minimum size of a fragment - the chord length.

Mostly this doesn't matter, because the differences become visible only at small numbers of sides, and $fs and $fa are a "try to do something appropriate" mechanism rather than being intended to be precisely correct.

To Reproduce

r = 10;
$fs = 11;

n = ceil(r * 2 * PI / $fs);
side = 2*r*sin(360/n/2);
echo(n=n, side=side);

observe that this yields a hexagon with a side of 10, when it should be at least 11.

Expected behavior

side should be >= $fs.

The correct formula would be

floor(180/asin($fs/2/r))

which, for the parameters above, yields a pentagon with a side of 11.7557.

Additional context
Probably this doesn't matter. Compatibility says we shouldn't fix it: the (incorrect) formula for calculating the number of sides based on $fs has been documented, and models might have used that formula to calculate the number of sides so as to rotate to align a side or vertex with an axis. (They might also try to use it to adjust the radius so that it measures from the center to the side instead of the center to a vertex, but that would be wrong because adjusting the radius for that could yield a different number of sides.)

We might change the prose documentation to match the actual calculation, though it probably isn't worth the effort.

I just thought I should record the inaccuracy.

@jordanbrown0 jordanbrown0 changed the title Calculation of number of sides based on $fs is incorrect Calculation of number of sides based on $fs is slightly incorrect Apr 24, 2024
@rcolyer
Copy link
Member

rcolyer commented Apr 24, 2024

Well, it's neither the minimum chord length nor the minimum arc length when we factor in the more nuanced behavior of $fs in sphere, which ends up with edges well below $fs, as it is the like the equitorial arc length of a fragment for which equitorial fragments don't always even exist in a sphere. There has been a long stream of debate about how to describe these in a combination of clearly and correctly, which is why that section goes as far as including the code that calculates them (although weirdly including a C version of what is actually a C++ function in the code with a few extra cases of edge case handling). I don't see an issue with the current method of calculating them. But I'll note that the language about "minimum" is really trying to coarsely explain that fmin on the $fa and $fs computations. And there is definitely an opportunity to yet again try to explain that more clearly.

@jordanbrown0
Copy link
Contributor Author

I suspect (without looking at the implementation) that for sphere they control the equator and the lines of longitude.

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

No branches or pull requests

2 participants