Three.js: Purification of Math - radToDeg and DegToRad

[MasterJames]

This is just a notification of a patch within my PRs in progress branch
It was suggested from https://github.com/mrdoob/three.js/issues/7690

https://github.com/MasterJames/three.js/commit/ee5c1fef6886aa90b46145454b6e0b7b5ba71b3f

It simplifies the Math functions radToDeg and degToRad where radians and degrees are calculated against a returned function in a closure when the math was baked out program-atically.

Any advice on how to submit something like this is appreciated.
I hope this is an acceptably minimally obtrusive method.

[MasterJames]

In fact it's probably worth considering a degToRad2 and radToDeg2 too.
it would save a multiplication on somethings that probably have significant usage at times.
The usage of this would at least speed it up some if not nearly or literally "double it".
For a use case I'm thinking of the other FOV issue with a spot light angle animating ( needs recalculating spot lights.camera.fov )

    degToRad2: function ( degrees ) {

        // Math.PI / 360 = 0.008726646259971648

        return degrees * 0.008726646259971648;

    },

    radToDeg2: function ( radians ) {

        // 360 / Math.PI = 114.59155902616465

        return radians * 114.59155902616465;

    },

Because...
The size of a radian is determined by the requirement that there are 2 PI radians in a circle.

http://math.rice.edu/~pcmi/sphere/drg_txt.html

[RemusMar]

Honestly, these degToRad & radToDeg functions should be removed.
A call / return approach for a simple thing is a waste of resources for nothing.
If you need this "conversion", a singe line is needed:

radians = degrees * 0.00872665;
// or
degrees = radians * 114.59155903;

cheers

[makc]

indeed, it belongs to some separate math package somewhere in npm :) btw, what ever happened to all those tickets requesting to split three.js into clean modules?

[MasterJames]

I think the usage makes it hard to allow a breaking change (internal references too), so for compatibility they should stay maybe with a depreciating warning. I'd keep it and add the degToRad2 and radToDeg2 too.
So good to see these even stronger opinions though.

[mrdoob]

We could also do...

THREE.DEG2RAD = 0.008726646259971648;
THREE.RAD2DEG = 114.59155902616465;

[MasterJames]

Yes okay but would that be a potentially breaking change? Delete the functions? Easy migration fix for others.

Also what about the "half" version? I think 114 is double the previous radToDeg but again it would save a compute to have the 4 values?

[makc]

@MasterJames I bet they just copy-pasted the values from your xxx2 functions (without much thinking).

it would save a multiplication on somethings that probably have significant usage at times.

saving multiplications by introducing functions - that's the spirit )

[MasterJames]

The previous/current function was/is a closure wrapper. The constant idea would cause breakage by removing the functions but I'm all for it. Yes I just put the number that's why I brought it up.
Ya it's just to get the FOV you want 2 times the radians to degrees. 20 dividing by/with 360 is helpful normally it's by/with 180

[mrdoob]

Actually, if anything it should be...

THREE.Math.DEG2RAD = 0.008726646259971648;
THREE.Math.RAD2DEG = 114.59155902616465;

Which is ugly :disappointed:

[makc]

@mrdoob 0.017453292519943295 and 57.29577951308232 :pensive:

[mrdoob]

Sorry! 😅

THREE.Math.DEG2RAD = 0.017453292519943295;
THREE.Math.RAD2DEG = 57.29577951308232;

[RemusMar]

Ricardo,

These degToRad & radToDeg functions should be removed.
They waste resources for nothing and they also slow down the things.
There are 5 internal references to "degToRad" and 2 to "radToDeg".
They should be replaced with the constant values.

cheers

[mrdoob]

Are those constants precise enough?

[RemusMar]

Are those constants precise enough?

8 decimals are enough in 99% of the cases.
12-16 are MORE than enough.

[makc]

57.295779513082320876798154814105170332405472466564321549160243861202847148321552632440968995851110944

0.0174532925199432957692369076848861271344287188854172545609719144017100911460344944368224156963450948

Ok?

[makc]

actually if you just paste the number into console, you can see how many digits do you really need:

[MasterJames]

Ya the console precision is the same as baking it in a closure.
We should provide a depreciation warning in place of the old function if breakage here is not a concern.
I suppose some documentation updates are needed as well.

[RemusMar]

Ya the console precision is the same as baking it in a closure.

It's not about console.
In JavaScript the maximum number of decimals is 17
cheers

[MasterJames]

Okay good to know RemusMar, thanks for that.

Hmmm... ? Is it 17 digits of precision because the console returns an 18th digit? It's no doubt the extra zero in the tenths column, that doesn't count towards precision.
The precision should simply be rounded up from 5 to 6, so the console does get it wrong. (Rounding up ensures you cover the extent fully+).
In the case of the shadows you could round up somewhat sooner (multiplies faster too), but I don't think we should take it that far.

Math.PI / 180 = 0.017453292519943296 is best in my opinion. {rounded off is up}
180 / Math.PI = 57.29577951308232 the console drops the 17th one because it's a zero and the next digit won't force a round up either.

Originally I figured you all wanted it to be clear what the value was and how it was being computed was a big priority or you'd just use the values directly without even a global const lookup like I aspire to.

Also a maybe only perceived goal of avoiding breakage suggests keeping the leaner function named as it was (yes still an improvement over the closure version in use in the wild today).

Still noting that having the 360 versions along with the 180 pair makes sense anyway you decide to do it.

[RemusMar]

Still noting that having the 360 versions along with the 180 pair makes sense anyway you decide to do it.

I also think the current approach (rotations above 180 degrees) is not the best one.
Much more useful (and used in many 3D engines) is:

-180 ... 0 ... 180 (in degrees)
or
-3.14159265 ... 0 ... 3.14159265 (in radians)

[MasterJames]

Loved the comment RemusMar something to think about, but I'm not "all in" there yet.

The ironic comment to all this is, Maybe it's better to leave the rounding to the "device's" Math.PI library and bake it in a closure?

But I still could use the 360 pair too I thought? Ya I think so, wait "No! I'm just putting numbers now anyway." { ironic comment 2 } which supports constants if needed to change precision with mega ram 128bit system? which find and replace addresses fine by me in the unlikely hood.

[RemusMar]

To call a function only to multiply or divide something with a constant value is a bad idea from any point of view.

[makc]

@mrdoob

0.017453292519943296 is best in my opinion. {rounded up}

[MasterJames]

Ya okay it was just if you want to drop precision (round up). and in the case of the light shadow camera fov on my mind. { back peddle ... Um I was Gimblocked }

[RemusMar]

0.017453292519943296 is best in my opinion. {rounded up}

0.01745329252 is the best choice for rotations and radians.

[MasterJames]

For the SpotLight Shadow Camera's FOV based on the SpotLights 'angle', it can be 0.0174599 or 0.0174500 and we are fine with a penumbra of ~0.05 or greater?

[MasterJames]

Here this PR has 2 SpotLight Examples, it's where I'm coming from (but I think you have to implement an automatic angle solution somehow I suppose for that to work properly with angle animation in the second example).
https://github.com/MasterJames/three.js/pull/1/files
I thought another situation for rounding up might be "seam covering"?

[MasterJames]

All examples work with this.

`
degToRad: function ( degrees ) {

    // Math.PI / 180 = 0.017453292519943295

    return degrees * 0.0174534;

},

`

[makc]

wait, are you trying to save some bytes by not using full resolution numbers? RLY?

[MasterJames]

No it's just a friendly debate I appreciate the input.
Ironically the Guinit tests run slower, which I was not expecting.
2.5 vs 3.5 seconds.

http://127.0.0.1:8080/test/unit/unittests_three.html?hidepassed

says something for precision.

[MasterJames]

Anyway there are many reasons for "both" arguments when picking a number, I guess it depends on the situation.

[MasterJames]

Anyway It's about rounding off not down or up Pardon my terminology.
0.01745329251994329576923690768489
0.017453292519943296
Rounding off in this case is rounding up.
Its also just inherent to the nature of PI there's always a little bit more to it then you think.

[mrdoob]

I opted for a different approach 😉

[RemusMar]

I opted for a different approach

1) To call a function only to multiply or divide something with a constant value is a bad idea from any point of view.
2) In JavaScript the maximum number of decimals is 17
If you use more than 17, you might get even wrong results (!!!)
Example:

var v1 = 0.1;
var v2 = 0.2;
console.log( v1 + v2 );
// 0.30000000000000004

Of course, this is your library, so you're free to do anything you like.
cheers

[mrdoob]

1) To call a function only to multiply or divide something with a constant value is a bad idea from any point of view.

You don't have to call a function anymore, THREE.Math.DEG2RAD is there.

2) In JavaScript the maximum number of decimals is 17
If you use more than 17, you might get even wrong results (!!!)
Example:

var v1 = 0.1;
var v2 = 0.2;
console.log( v1 + v2 );
// 0.30000000000000004

Of course, this is your library, so you're free to do anything you like.

Not sure how this is related to the approach I went for.

[RemusMar]

You don't have to call a function anymore, THREE.Math.DEG2RAD is there.

1) why do you need "a child of a child of something" to define a constant value?

2) you still keep the (worthless) function:

radToDeg: function ( radians ) {
return radians * THREE.Math.RAD2DEG;
}

[mrdoob]

You forgot to respond point 2 in my previous comment.

[MasterJames]

From Chrome Console which I assumed is representational of the 64 bit numbers in JavaScript
Math.PI / 360 =
0.008726646259971648
0.00872664625997164788461845384244
Rounding off _(aka up)_ correctly!
Math.PI / 180 =
0.017453292519943295
0.01745329251994329576923690768489
Rounding off (aka up) failed.

What's up with this? I'll have to think more about it for a time. [Maybe I'll look around or post on SO about it.]
It seems that in the case of 0.00* the console is rounding Off the 17th precision (to 16) the console doesn't show it. It gets stranger comparing 180 vs 18.

Math.PI / 36 =
0.08726646259971647
0.08726646259971647884618453842443
{Rounding Off fails}

Math.PI / 18 =
0.17453292519943295
0.17453292519943295769236907684886
{Rounding Off fails}

{ Note: PI is safe from rounding "Off" issues at this precision }

Anyway I'm now thinking more seriously about the original closure method because of that rounding discrepancies one should probably let the system's Math.PI and division operator do the work per device, as the computer will prefer consistency even if the rounding off of numbers is odd and inconsistent (as demonstrated with dividing PI by 180 vs 18).
Putting it in the closure like before I figure saves a chaining lookup THREE->Math->D2RConversionValue 3 steps? I don't think the compiler bakes that in as it might change, since there's some argument still about the value it shouldn't be made to be a constant (so people can set/override the correctly rounded value if preferred) which as a constant the compiler might save the triple lookup every time by baking it.

I still believe it should round off _(aka up)_ to the value ending in 6.
Math.PI / 180 = 0.017453292519943295 but should round to 0.017453292519943296

@mrdoob 's second point is also interesting. It makes me think of a precision modulus but that would not be good in this function might be useful on the other end though.

var v3 = (V1 + V2);
console.log( v3 - (v3 % 0.001) );

All the more reason to do the Grade 4 (?) level fraction rounding to us and not the computer.

[RemusMar]

You forgot to respond point 2 in my previous comment.

If with 17 decimals JavaScript generate errors, why do you use DEG2RAD: Math.PI / 180 ?
And why do you need THREE.Math.DEG2RAD anyway?
Use the constant values (with 12-16 decimals) where they are referenced.
No more wasted resources for nothing, faster processing and more accurate results.
And be sure these constant values won't change for the entire life of JavaScript and WebGL ... LOL
cheers

[MasterJames]

My previous attempt gave NAN aka LOL.

Assuming you have this rounding function in your math lib (provided).

THREE.Math.DEG2RAD = Math.PI / 180;

function round_number(num, dec) return Math.round(num * Math.pow(10, dec)) / Math.pow(10, dec);

return round_number( ( radians * THREE.Math.DEG2RAD ), 16 );

Produces this...
round_number( ( 1 * THREE.Math.DEG2RAD ), 14 );
0.01745329251994
round_number( ( 1 * THREE.Math.DEG2RAD ), 15 );
0.017453292519943
round_number( ( 1 * THREE.Math.DEG2RAD ), 16 );
0.0174532925199433
round_number( ( 1 * THREE.Math.DEG2RAD ), 17 );
0.0174532925199433
round_number( ( 1 * THREE.Math.DEG2RAD ), 18 );
0.017453292519943295

Which still gives the value not of the correctly rounded 0.017453292519943296 because they are limited functions.