Spherical Coordinate - Code Review And Help Request

[Klaim]

This project is now hosted on GitHub: https://github.com/Klaim/ogre-spherical

Feel free to report problems and request or provide enhancements.

Initial post follows.

----------------------------------------------------------------------------------------------------------------------

I've been trying to understand in depths spherical coordinates but I'm failing in details I think.
Anyway searching both online and Ogre forum was useful, but I remarked that no type representing such coordinates was ever provided.

So I decided to make one for my own usage, but it seems it could be useful for a lot of other users.
I will certainly either make a PR to add this type to OgreMain, or make a wiki article for it, OR make a repository and put everything there and have a Wiki page link to it or something like that. I don't know what do you think might be the more useful?

Here is what I have so far:

Code: Select all

/** Spherical coordinates vector, used for spherical coordinate and transformations. */
	struct SphereVector
	{
		Ogre::Real   radius; ///< Rho or Radius is the distance from the center of the sphere.
		Ogre::Radian theta;	 ///< Theta is the angle around the x axis, values range from 0 to PI.
		Ogre::Radian phi;    ///< Phi is the angle around the x axis, values range from 0 to 2PI.
		
		friend SphereVector operator-( const SphereVector& value ) 
		{
			SphereVector result;
			result.radius = -value.radius;
			result.theta  = -value.theta;
			result.phi    = -value.phi;
			return result; 
		}

		friend SphereVector operator+( const SphereVector& left, const SphereVector& right )
		{ 
			SphereVector result;
			result.radius = left.radius + right.radius;
			result.theta  = left.theta + right.theta;
			result.phi    = left.phi + right.phi; 
			return result;
		}

		friend SphereVector operator-( const SphereVector& left, const SphereVector& right )
		{ 
			return left + (-right); 
		}
		
		SphereVector& operator+=( const SphereVector& other )
		{ 
			*this = *this + other;
			return *this;
		}

		SphereVector& operator-=( const SphereVector& other )
		{
			*this = *this - other;
			return *this;
		}

		/// Rotate the position around the center of the sphere.
		friend SphereVector operator*( const SphereVector& sv, const Ogre::Quaternion& rotation )
		{
			???
		}

		/// Rotate the position around the center of the sphere.
		friend SphereVector operator*( const Ogre::Quaternion& rotation, const SphereVector& sv ) { return sv * rotation; }

		/// Rotate the position around the center of the sphere.
		SphereVector& operator*=( const Ogre::Quaternion& rotation ) 
		{
			*this = *this * rotation;
			return *this;
		}

		/** @return a Cartesian vector coordinate from the spherical coordinate around the provided center position. */
		Ogre::Vector3 to_position_around( const Ogre::Vector3& sphere_center ) const
		{
			// NOTE: All online examples are based on +Z being up, but in Ogre/OGL +Y is up!
			Ogre::Vector3 result;
			result.z = radius * Ogre::Math::Sin( theta ) * Ogre::Math::Sin( phi );
			result.x = radius * Ogre::Math::Sin( theta ) * Ogre::Math::Cos( phi );
			result.y = radius * Ogre::Math::Cos( theta );
			return sphere_center + result;
		}

		/** @return a rotation from the provided axis to the spherical coordinate. */
		Ogre::Quaternion rotation_from( const Ogre::Vector3& center, const Ogre::Vector3& axis ) const 
		{
			const auto position_from_sphere_pos = to_position_around( center );
			return axis.getRotationTo( position_from_sphere_pos );
		}

	};

Questions and remarks:

1. ignore the naming, coding convention and use of auto for now, it's code used outside Ogre but I'll rewrite it to match Ogre's convention once it works well;
2. are the comments correct for the members? I'm having trouble with which theta or phi is around which axis in the case of y being the up vector. All the documentation online, Ogre forum included, describe the case for z being the up vector. This is very confusing for me but I think I might have got it right, so I need confirmation on this. (sorry I'm very slow at maths because I don't trust myself at all)
3. is to_position_around() implementation correct? From my tests it seems to be, but I think it's easy to get wrong.
4. same question rotation_from()
5. How should I implement rotation around the center using a quaternion? (see opertor*() ) I'm totally lost on this one but I need to use it systematically with this type.
6. Are there important missing operators or functions you want to see? I added operator*( Real) before but it made no sense so I removed it (and I wouldn't use it anyway). Also, I'm wondering if equality operators would make sense and if yes should I do like Ogre maths objects and use the natural == operator instead of Ogre::Math::RealEqual()?
7. Any comments are welcome.

Thanks for your attention and time.
I hope this will be useful, but it will certainly need upgrades.

[Klaim]

Quick Self review: I'll replace these functions

Code: Select all

/** @return a Cartesian vector coordinate from the spherical coordinate around the provided center position. */
      Ogre::Vector3 to_position_around( const Ogre::Vector3& sphere_center ) const
      {
         // NOTE: All online examples are based on +Z being up, but in Ogre/OGL +Y is up!
         Ogre::Vector3 result;
         result.z = radius * Ogre::Math::Sin( theta ) * Ogre::Math::Sin( phi );
         result.x = radius * Ogre::Math::Sin( theta ) * Ogre::Math::Cos( phi );
         result.y = radius * Ogre::Math::Cos( theta );
         return sphere_center + result;
      }

      /** @return a rotation from the provided axis to the spherical coordinate. */
      Ogre::Quaternion rotation_from( const Ogre::Vector3& center, const Ogre::Vector3& axis ) const 
      {
         const auto position_from_sphere_pos = to_position_around( center );
         return axis.getRotationTo( position_from_sphere_pos );
      }

By these

Code: Select all

/** @return a Cartesian vector coordinate from the spherical coordinate taking the origin as center of the sphere. */
		Ogre::Vector3 to_cartesian() const
		{
			// NOTE: All online examples are based on +Z being up, but in Ogre/OGL +Y is up!
			Ogre::Vector3 result;
			result.z = radius * Ogre::Math::Sin( theta ) * Ogre::Math::Sin( phi );
			result.x = radius * Ogre::Math::Sin( theta ) * Ogre::Math::Cos( phi );
			result.y = radius * Ogre::Math::Cos( theta );
			return result;
		}

		/** @return a rotation from the provided axis to the vector corresponding to the spherical coordinate around the origin. */
		Ogre::Quaternion rotation_from( const Ogre::Vector3& axis ) const 
		{
			const auto position_around_origin = to_cartesian();
			return axis.getRotationTo( position_around_origin );
		}

It feels both simpler and more modular, but I'll have to check in practice. At least in my user code it's seems clear.


I'll need a confirmation on this:

Code: Select all

SphereVector spherical_pos;
spherical_pos.radius = 1.f;
spherical_pos.theta = Ogre::Degree( 0.f );
spherical_pos.phi = Ogre::Degree( 0.f );

Am I correct that it should corresponds to Ogre::Vector3::UNIT_Z ?

[Kojack]

How should I implement rotation around the center using a quaternion? (see opertor*() ) I'm totally lost on this one but I need to use it systematically with this type.

You need to convert the spherical coords into vector form (relative, ignoring the centre), multiply the quaternion by the vector (this will rotate the vector but keep it the same length) then convert back to spherical.

The to_cartesian looks about right (although I haven't tested it). There's no single "standard" for spherical coordinates, so the meaning of theta and phi can be swapped around and the 0 degrees position can be in different places.
A from_cartesian would be handy too (including for the quaternion multiply above).
Something like:

Code: Select all

void from_cartesian (const Ogre::Vector3 &cartesian)
{
   radius = cartesian.length();
   phi = Math::Atan(cartesian.z / cartesian.x);
   theta = Math::ACos(cartesian.y / radius);
}

Am I correct that it should corresponds to Ogre::Vector3::UNIT_Z ?

That's a harder one. Since there's no real standard (just conventions in different fields), you can have it any way.
But ogre's default direction (for cameras, nodes, lookat, setdirection, etc)is NEGATIVE_UNIT_Z.
If you give your current formulas theta=0, phi=0,radius=1, you'll get out a vector aiming straight up. Theta is the angle from the vertical axis, so you need theta = 90 to aim level (x,z plane) and theta=180 to aim down. Phi is an angle from the x axis, so theta=90, phi=0, radius=1 will give you a UNIT_X vector3.

[Klaim]

Thanks Kojack, that's very helpful!
The initial version did have -Z as (radius=1, 0, 0) position, but I was wondering if it was because of Ogre using y for up and all the example code I've seen using z instead, so I tried to inverse.
Maybe I should have not, to least surprise people using this.

I'll do modifications and post a new version here.

[Klaim]

Here is what I have at the moment:

Code: Select all

	/** Spherical coordinates vector, used for spherical coordinates and transformations. 
		Some example values:
			( radius = 1.0, theta = 0.0deg , phi = 0.0deg  ) <=> Z unit vector in cartesian space
			( radius = 1.0, theta = 90.0deg, phi = 0.0deg  ) <=> Y unit vector in cartesian space
			( radius = 1.0, theta = 90.0deg , phi = 90.0deg ) <=> X unit vector in cartesian space
	*/
	struct SphereVector
	{
		Ogre::Real   radius; ///< Rho or Radius is the distance from the center of the sphere.
		Ogre::Radian theta;	 ///< Theta is the angle around the x axis (latitude angle counterclockwise), values range from 0 to PI.
		Ogre::Radian phi;    ///< Phi is the angle around the y axis (longitude angle counterclockwise), values range from 0 to 2PI.

		friend SphereVector operator-( const SphereVector& value ) 
		{
			SphereVector result;
			result.radius = -value.radius;
			result.theta  = -value.theta;
			result.phi    = -value.phi;
			return result; 
		}

		friend SphereVector operator+( const SphereVector& left, const SphereVector& right )
		{ 
			SphereVector result;
			result.radius = left.radius + right.radius;
			result.theta  = left.theta + right.theta;
			result.phi    = left.phi + right.phi; 
			return result;
		}

		friend SphereVector operator-( const SphereVector& left, const SphereVector& right )
		{ 
			return left + (-right); 
		}
		
		SphereVector& operator+=( const SphereVector& other )
		{ 
			*this = *this + other;
			return *this;
		}

		SphereVector& operator-=( const SphereVector& other )
		{
			*this = *this - other;
			return *this;
		}

		/// Rotation of the position around the relative center of the sphere.
		friend SphereVector operator*( const SphereVector& sv, const Ogre::Quaternion& rotation )
		{
			auto cartesian = sv.to_cartesian();
			cartesian = rotation * cartesian;
			return SphereVector::from_cartesian( cartesian );
		}

		/// Rotation of the position around the relative center of the sphere.
		friend SphereVector operator*( const Ogre::Quaternion& rotation, const SphereVector& sv ) { return sv * rotation; }

		/// Rotation of the position around the relative center of the sphere.
		SphereVector& operator*=( const Ogre::Quaternion& rotation ) 
		{
			*this = *this * rotation;
			return *this;
		}

		/** @return a relative Cartesian vector coordinate from this relative spherical coordinate. */
		Ogre::Vector3 to_cartesian() const
		{
			Ogre::Vector3 result;
			result.x = radius * Ogre::Math::Cos( phi ) * Ogre::Math::Sin( theta );
			result.y = radius * Ogre::Math::Sin( phi ) * Ogre::Math::Sin( theta );
			result.z = radius * Ogre::Math::Cos( theta );
			return result;
		}

		/** @return a relative spherical coordinate from a cartesian vector. */
		static SphereVector from_cartesian( const Ogre::Vector3& cartesian )
		{
			SphereVector result;
			result.radius = cartesian.length();
			result.phi    = Ogre::Math::ATan( cartesian.z / cartesian.x );
			result.theta  = Ogre::Math::ACos( cartesian.y / result.radius );
			return result;
		}

		/** @return a relative rotation from the provided axis to the axis corresponding to this relative spherical vector. */
		Ogre::Quaternion rotation_from( const Ogre::Vector3& axis ) const 
		{
			const auto relative_direction = to_cartesian();
			return axis.getRotationTo( relative_direction );
		}

	};
	

I setup code to check the spherical coordinate visually. Some points:

1. I used the formula using directly the forumula what is usually used (http://www.ogre3d.org/forums/viewtopic. ... 88#p445988), which means that in Ogre we get +Z BEING (radius=1, theta=0, phi=0). I'm not totally sure I should keep it that way because it is visually a bit perturbing to have poles (around which each latitude is) around the Z axis instead of the Y axis. So maybe I'll change it later but for now it's using the Z axis for poles. It means that, for example, incrementing phi will rotate around the Z axis.

2. I couldn't check if rotation_from() is correct but I think if Ogre's code is correct and to_cartesian() is correct then it should be correct.

3. the rotation operator ( operator*(Quaternion) don't seem to work at all. When I try to rotate a spherical position, I systematically get the visual object to disappear.
I suspect the from_cartesian() function to be incorrect. Kojack, your example should be when Y is the up, so I tried inversing z and y in this code but I still get the object disappearing.

[Kojack]

In to_cartesian, you need to swap the result.y and result.z.
At the moment, it has the z based entirely on the theta angle and x,y as the plane. But in from_cartesian it has the theta entirely based on the y (which is correct for a y up coordinate system).

[Klaim]

Thanks Kojack, I fixed it and checked visually that the cartesian point is always where needed, even using the rotation.
So the following version works:

Code: Select all

	/** Spherical coordinates vector, used for spherical coordinates and transformations. 
		Some example values:
			( radius = 1.0, theta = 0.0deg , phi = 0.0deg  ) <=> Y unit vector in cartesian space
			( radius = 1.0, theta = 90.0deg, phi = 0.0deg  ) <=> Z unit vector in cartesian space
			( radius = 1.0, theta = 90.0deg , phi = 90.0deg ) <=> X unit vector in cartesian space
	*/
	struct SphereVector
	{
		Ogre::Real   radius; ///< Rho or Radius is the distance from the center of the sphere.
		Ogre::Radian theta;	 ///< Theta is the angle around the x axis (latitude angle counterclockwise), values range from 0 to PI.
		Ogre::Radian phi;    ///< Phi is the angle around the y axis (longitude angle counterclockwise), values range from 0 to 2PI.
		

		friend SphereVector operator-( const SphereVector& value ) 
		{
			SphereVector result;
			result.radius = -value.radius;
			result.theta  = -value.theta;
			result.phi    = -value.phi;
			return result; 
		}

		friend SphereVector operator+( const SphereVector& left, const SphereVector& right )
		{ 
			SphereVector result;
			result.radius = left.radius + right.radius;
			result.theta  = left.theta + right.theta;
			result.phi    = left.phi + right.phi; 
			return result;
		}

		friend SphereVector operator-( const SphereVector& left, const SphereVector& right )
		{ 
			return left + (-right); 
		}
		
		SphereVector& operator+=( const SphereVector& other )
		{ 
			*this = *this + other;
			return *this;
		}

		SphereVector& operator-=( const SphereVector& other )
		{
			*this = *this - other;
			return *this;
		}

		/// Rotation of the position around the relative center of the sphere.
		friend SphereVector operator*( const SphereVector& sv, const Ogre::Quaternion& rotation )
		{
			auto cartesian = sv.to_cartesian();
			cartesian = rotation * cartesian;
			return SphereVector::from_cartesian( cartesian );
		}

		/// Rotation of the position around the relative center of the sphere.
		friend SphereVector operator*( const Ogre::Quaternion& rotation, const SphereVector& sv ) { return sv * rotation; }

		/// Rotation of the position around the relative center of the sphere.
		SphereVector& operator*=( const Ogre::Quaternion& rotation ) 
		{
			*this = *this * rotation;
			return *this;
		}

		/** @return a relative Cartesian vector coordinate from this relative spherical coordinate. */
		Ogre::Vector3 to_cartesian() const
		{
			Ogre::Vector3 result;
			result.x = radius * Ogre::Math::Cos( phi ) * Ogre::Math::Sin( theta );
			result.z = radius * Ogre::Math::Sin( phi ) * Ogre::Math::Sin( theta );
			result.y = radius * Ogre::Math::Cos( theta );
			return result;
		}

		/** @return a relative spherical coordinate from a cartesian vector. */
		static SphereVector from_cartesian( const Ogre::Vector3& cartesian )
		{
			SphereVector result;
			result.radius = cartesian.length();
			result.phi    = Ogre::Math::ATan( cartesian.z / cartesian.x );
			result.theta  = Ogre::Math::ACos( cartesian.y / result.radius );
			return result;
		}

		/** @return a relative rotation from the provided axis to the axis corresponding to this relative spherical vector. */
		Ogre::Quaternion rotation_from( const Ogre::Vector3& axis ) const 
		{
			const auto relative_direction = to_cartesian();
			return axis.getRotationTo( relative_direction );
		}

	};
	

Now I need to add some constructors and some useful constants.

[Klaim]

I've put the code on github and changed the initial post accordingly: https://github.com/Klaim/ogre-spherical

Any feedback is welcome!

I'm considering putting something on the wiki but I'm not sure if it wouldn't be redundant or where to put it in the wiki.

[Klaim]

Kojack, I have a very strange problem with from_cartesian() function: if the input is UNIT_Y I get the following spherical coordinates

Code: Select all

{radius=1.00000000 theta={mRad=0.000000000 } phi={mRad=-1.#IND0000 } }

(vs2013, 32bit, debug mode)

I'm not sure where the source of the problem is, but I'm suspecting a problem into Math::ACos(). Do you have any suggestion?

EDIT> There is a potential division per 0 on the pointed lines

Code: Select all

static SphereVector from_cartesian( const Ogre::Vector3& cartesian )
		{
			SphereVector result;
			result.radius = cartesian.length();
			result.phi = Ogre::Math::ATan( cartesian.z / cartesian.x ); // HERE
			result.theta = Ogre::Math::ACos( cartesian.y / result.radius ); // HERE
			return result;
		}

I fixed it this way and it seems to work find but I'm not totally convinced:

Code: Select all

static SphereVector from_cartesian( const Ogre::Vector3& cartesian )
		{
			using namespace Ogre;
			SphereVector result;
			result.radius = cartesian.length();
			result.phi = cartesian.x > Real(0) || cartesian.x < Real(0) ? Ogre::Math::ATan( cartesian.z / cartesian.x ) : Radian(0);
			result.theta = result.radius > Real(0) || result.radius < Real(0) ? Ogre::Math::ACos( cartesian.y / result.radius ) : Radian(0);
			return result;
		}

[Klaim]

I realized that this should be the correct computation:

Code: Select all

/** @return a relative spherical coordinate from a cartesian vector. */
		static SphereVector from_cartesian( const Ogre::Vector3& cartesian )
		{
			using namespace Ogre;
			SphereVector result;
			result.radius = cartesian.length();
			result.phi    = cartesian.x   > Real(0) || cartesian.x   < Real(0) ? Math::ATan( cartesian.z / cartesian.x )   : Math::ATan( 0 );
			result.theta  = result.radius > Real(0) || result.radius < Real(0) ? Math::ACos( cartesian.y / result.radius ) : Math::ACos( 0 );
			return result;
		}

[Klaim]

I am having trouble with another problem in from_cartesian().
Basically SphereVector::UNIT_X == SphereVector::UNIT_Z, which is obviously wrong. These values are built using from_cartesian(Vector::UNIT_X) and from_cartesian(Vector::UNIT_Z).

I'm comparing the code to the algorithm described by wikipedia and it seems to be the same, but I'm wondering if the checks for division by zero is wrong.

Any idea?

[Klaim]

This seems to work but I'm not confident that it's mathematically correct:

Code: Select all

static SphereVector from_cartesian( const Ogre::Vector3& cartesian )
		{
			using namespace Ogre;
			SphereVector result;
			result.radius = cartesian.length();
			result.phi    = cartesian.x   > Real(0) || cartesian.x   < Real(0) ? Math::ATan( cartesian.z / cartesian.x )   : Radian( Math::HALF_PI );
			result.theta  = result.radius > Real(0) || result.radius < Real(0) ? Math::ACos( cartesian.y / result.radius ) : Radian( 0 );
			return result;
		}

[Klaim]

The previous example don't really work, modifying theta and phi makes visible that it's only half of a sphere that is represented.

I'm still looking at fixing this, any help is welcome.

[Klaim]

These two variants seems to almost work:

Code: Select all

static SphereVector from_cartesian( const Ogre::Vector3& cartesian )
		{
			using namespace Ogre;
			SphereVector result = SphereVector::ZERO;
			result.radius = cartesian.length();
			if( result.radius == 0 )
				return result;
			result.phi    = Math::ATan2( cartesian.z, cartesian.x );
			result.theta  = Math::ATan2( Math::Sqrt( Math::Sqr( cartesian.x ) + Math::Sqr( cartesian.z ) ), cartesian.y );
			return result;
		}

Code: Select all

static SphereVector from_cartesian( const Ogre::Vector3& cartesian )
		{
			using namespace Ogre;
			SphereVector result = SphereVector::ZERO;
			result.radius = cartesian.length();
			if( result.radius == 0 )
				return result;
			result.phi    = Math::ATan2( cartesian.z, cartesian.x );
			result.theta  = Math::ACos( cartesian.y/ result.radius );
			return result;
		}

There a some issues, like my visual cursor avoiding positions around NEGATIVE_Z. So it's close but not correct (except if the problem comes from somewhere else.

[Klaim]

In case someone can help:

I'm having trouble getting the code right. The more tests I'm adding, the more trouble I find. Clearly I need some specialists to check that code.
The following code is not yet in the repository, that's what I have on my own project's code base.

The current code: https://gist.github.com/Klaim/8633895 with constants defined there: https://gist.github.com/Klaim/8634224
The current tests: https://gist.github.com/Klaim/8633917
The current test report (only for SphereVector): https://gist.github.com/Klaim/8633937

You can see in the error report that there are 4 test failings (starting at line 163).
Basically, rotating the sphere vector using a quaternion is failing.
Unfortunately, that also makes several other operations failing in my use code.
I'm not sure what exactly is the source of the failure but maybe it's the nature of the quaternion generated from vector3.getRotationTo() which is making the tests wrong.

Anyway, I'm stuck with this code so if you spot any problem with the code or the test, feel free to add comments or post here.
Damn I feel so dumb.

EDIT: I also posted there just in case http://codereview.stackexchange.com/que ... maths-code