Parameterization of a Torus with `minor > major`

I stumbled onto this while testing a new surface integral function:

A Torus can be constructed whose minor radii is greater than its major radii. Based on a quick reading of the Wikipedia page (IANA-geometer), it sounds like this is valid but sort of a special case.
For such a Torus, the current parameterization code will error. Line 55 essentially takes sqrt(t.major^2 - t.minor^2), which throws a DomainError since the argument is negative but also <:Real.

Lines 47 to 63 in d52cc5e

    
           function (t::Torus{T})(u, v) where {T} 
        
             if (u < 0 || u > 1) || (v < 0 || v > 1) 
        
               throw(DomainError((u, v), "t(u, v) is not defined for u, v outside [0, 1]².")) 
        
             end 
        
             c, n⃗ = t.center, t.normal 
        
             R, r = t.major, t.minor 
        
             Q = rotation_between(Vec{3,T}(0, 0, 1), n⃗) 
        
             kxy = R^2 - r^2 
        
             kz = √kxy * r 
        
             uₛ = T(π) * (2 * u - 1) 
        
             vₛ = T(π) * (2 * v - 1) 
        
             k = R - r * cos(vₛ) 
        
             x = kxy * cos(uₛ) / k 
        
             y = kxy * sin(uₛ) / k 
        
             z = kz * sin(vₛ) / k 
        
             c + Q * Vec{3,T}(x, y, z) 
        
           end

I don’t know enough about how the parameterization here is derived to know whether the absolute value of the interior term could be taken first, or if parameterizing Torus with this property is a harder problem. It’s also non-obvious to me whether this type of Torus was even intended to be supported.

@stla could you please take a look in this issue? Do I remember correctly that you added the Torus primitive last year?

Yes, that's me. In fact, I implemented a non-usual parameterization of the torus, valid only for major > minor. I thought it was interesting because it is a conformal parameterization, but actually this is useless.

What do you suggest in this case @stla? Should we adopt a different parametrization?

Yes, we should adopt the classical parameterization (certainly given on Wikipedia).

Appreciate if you can take a look into it. Otherwise, we can try to fix ourselves when we find time. Do you have a link to the wiki page and formula? That can speed things up. Em qua., 28 de fev. de 2024 11:53, stla ***@***.***> escreveu:

…

Yes, we should adopt the classical parameterization (certainly given on Wikipedia). — Reply to this email directly, view it on GitHub <#789 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZQW3I7FJGVSXUYSVXWS6DYV5AHNAVCNFSM6AAAAABD5FRK5SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRZGE2TMOBRGI> . You are receiving this because you commented.Message ID: ***@***.***>

Here. I could do the modifications but not so soon.

This seems like it should be easy enough to re-implement. I’ll make an attempt at a PR for this.

Fixed on master.

	function (t::Torus{T})(u, v) where {T}
	if (u < 0 \|\| u > 1) \|\| (v < 0 \|\| v > 1)
	throw(DomainError((u, v), "t(u, v) is not defined for u, v outside [0, 1]²."))
	end
	c, n⃗ = t.center, t.normal
	R, r = t.major, t.minor
	Q = rotation_between(Vec{3,T}(0, 0, 1), n⃗)
	kxy = R^2 - r^2
	kz = √kxy * r
	uₛ = T(π) * (2 * u - 1)
	vₛ = T(π) * (2 * v - 1)
	k = R - r * cos(vₛ)
	x = kxy * cos(uₛ) / k
	y = kxy * sin(uₛ) / k
	z = kz * sin(vₛ) / k
	c + Q * Vec{3,T}(x, y, z)
	end