geom::Torus radii naming clarification

[gaborpapp]

I think that geom::Torus radii naming is a bit confusing and should be documented. Usually the distance between the axis and the center of the rotated circle is known as major radius (R), whereas the circle radius is called minor radius (r). While Cinder's major radius seems to be R + r, and minor radius is R.

[paulhoux]

Good point, Gabor. Changing the implementation would break existing code, but adding some documentation will do no harm. Do you think you could make a PR for it?

[gaborpapp]

Thanks Paul. I would clarify it something like this: major is the distance from the axis to the farthest point of the torus. minor is the distance between the axis and the center of the rotated circle.

Looking at the code of ratio() (https://github.com/cinder/Cinder/blob/master/include/cinder/GeomIo.h#L531) also uses the naming, but the code works differently.

The documentation says it Specifies the major and minor radius as a ratio (minor : major). Resulting torus will fit unit cube, but the code is mRadiusMajor = 1; mRadiusMinor = 1 - ratio; so radius( 1.0, 0.75 ) generates the same torus as aspect( 0.25 ), and not aspect( 0.75 ) as the comment suggests.

Here I think the code should be fixed to work according the documentation, although maybe it would be better to stick to the standard naming and break existing code to avoid further confusion. What do you think?

[richardeakin]

I think our policy has been that if the code will break at compile time, it's okay to make the change to improve an API. But we should avoid silent breaking changes, which I think this one would fall under. May I suggest adding a new method called radiusCentered( major, minor ) for the 'usual' version, and then improving the docs on the existing methods?

I will say that I recall fiddling with the Torus methods quite a bit to understand what the params are controlling major / minor axes, so improved docs would be great.