math: move to es6 classes #19997

yomotsu · 2020-08-03T04:48:37Z

A part of #19986

This PR contains:

The rest of the math folder will be continued in separate PR.

The following are already completed in other PRs :

Ray ( Ray: Convert to es6 class #19980 )
Spherical ( Spherical: Converted to ES6 class. #19935 )

yomotsu · 2020-08-03T05:46:41Z

src/math/Vector3.js

 import { Quaternion } from './Quaternion.js';

-const _vector = new Vector3();
+let _vector;


Hoisting does not work anymore, because Vector3 is not a function.
Vec3 instance will be assigned at the end of this file.

yomotsu · 2020-08-03T13:00:18Z

I have converted the following classes in the math folder so far:

Vector2
Vector3
Vector4
Matrix3
Matrix4
Quaternion
Euler

@mrdoob @Mugen87
Would you mind reviewing them, please? 🙇‍♂️

If these are okay to merge, I will continue the rest of the math folder on other PR.

mrdoob · 2020-08-03T13:09:00Z

src/math/Vector3.js

-} );
+}

+_vector = new Vector3();


Would it work to do const _vector = new Vector3(); here?

That works! (I tested)
So, other internal use instances should be declared at the bottom of the file too?

Such as: const _quaternion = new Quaternion(); in

three.js/src/math/Vector3.js

Lines 1 to 7 in f549b79

import { MathUtils } from './MathUtils.js';

import { Quaternion } from './Quaternion.js';

let _vector;

const _quaternion = new Quaternion();

class Vector3 {

and

three.js/src/math/Matrix4.js

Lines 1 to 11 in f549b79

import { Vector3 } from './Vector3.js';

const _v1 = new Vector3();

let _m1;

const _zero = new Vector3( 0, 0, 0 );

const _one = new Vector3( 1, 1, 1 );

const _x = new Vector3();

const _y = new Vector3();

const _z = new Vector3();

class Matrix4 {

also maybe

three.js/src/math/Euler.js

Lines 4 to 9 in f549b79

import { MathUtils } from './MathUtils.js';

const _matrix = new Matrix4();

const _quaternion = new Quaternion();

class Euler {

So, other internal use instances should be declared at the bottom of the file too?

Yes, I think so!

Okay, done it with 8bcfccd

mrdoob · 2020-08-08T20:31:13Z

src/math/Euler.js


-			this._x = value;
-			this._onChangeCallback();
+	static get DefaultOrder() {


We should do a setter for this one too.

@mrdoob

Thank you for reviewing.
Sorry, I got it wrong, I thought DefaultOrder is read-only too.
I will fix it.

Before that, I have a question: How do we declare Public class fields?

The readable-writable static value can be declare like

class MyClass { static xxx = 1; constructor( x ) { this.x = x } }

However It seems Buble transpiler does not support the syntax, while Babel support that

Which option would be preferable in three.js (with Buble)?

class MyClass { constructor( x ) { this.x = x } } MyClass.xxx = 1;

or using Object.defineProperty

class MyClass { constructor( x ) { this.x = x } } Object.defineProperty( MyClass, 'xxx', { value: 1, configurable: true, writable: true } );

or

let _xxx = 1; class MyClass { static get xxx = () => { return _xxx; } static set xxx = ( value ) => { _xxx = value; } constructor( x ) { this.x = x } }

or other?
Thank you in advance🙇‍♂️

maybe @DefinitelyMaybe or @ianpurvis could give a suggestion?

class MyClass { constructor( x ) { this.x = x } } MyClass.xxx = 1;

I think this style is nice for read/write 👍

Thanks, I'll do that then!

For future reference:
There are only three things in the current r120 dev build where Buble is being used.
Curves, Geometries and Materials.
When Buble is no longer needed, i.e. after converting everything to es6 classes, we should have a look back into this

class MyClass { static xxx = 1; constructor( x ) { this.x = x } }

nvm. I've got Buble's purpose backwards. Buble is our 'convert classes back to support IE' tool and output it as build/three.js.

DefinitelyMaybe · 2020-08-09T04:19:48Z

static is a keyword for methods last time I checked. will have a look around.

ah. here we go

yomotsu · 2020-08-09T10:41:54Z

Fixed Euler.DefaultOrder to be readable-writable🙇‍♂️

mrdoob · 2020-08-11T11:40:20Z

Can you also revert RotationOrders?

Euler.RotationOrders = [ 'XYZ', 'YZX', 'ZXY', 'XZY', 'YXZ', 'ZYX' ];

mrdoob · 2020-08-11T11:52:17Z

Should we also do Euler.isEuler = true;?

yomotsu · 2020-08-11T12:13:07Z

I'll revert Euler.RotationOrders.

However, I personally think is*** properties can be getters, such as Euler.isEuler.

Users could change is*** however not recommended in the old document
Then it has been removed form the doc by Docs: Removed internal is* properties. #18217
is*** properties are read-only according to the d.ts

Sould we do Euler.isEuler = true;?

ianpurvis · 2020-08-12T23:28:24Z

I think we should give the read-only getter a try? That feels like the right spirit for type checking. I'm not a benchmark master, but I ran my cheap benchmark on some different type checking. TLDR, performance is all similar.

Vanilla instanceof seems fast these days. That has got to be the nicest semantically, might be a good direction for after all the ES6 class changes stabilize.

mrdoob · 2020-08-13T20:06:56Z

I think we should give the read-only getter a try?

I don't think this is the right time to give things a try.
I think we should focus on porting the code to es6 classes with the least side effects as possible.

Vanilla instanceof seems fast these days

We used to use instanceof. We changed it to is*** to allow tree-shaking.

mrdoob · 2020-08-13T20:09:30Z

However, I personally think is*** properties can be getters, such as Euler.isEuler.

It doesn't feel good to turn isEuler into a function. These variables get accessed a lot per frame.

ianpurvis · 2020-08-13T21:10:12Z

Fair enough, let's go with Euler.isEuler = true; 👍

yomotsu · 2020-08-14T02:12:45Z

I understand performance is paramount.
I changed is** to be a prototype value a53fcce

Also, I checked some in the example folder and working correctly.

mrdoob · 2020-08-14T09:48:06Z

Thanks!

yomotsu · 2020-08-14T10:54:08Z

Thank you so much for your kind advice and all help🙇‍♂️

yomotsu · 2020-08-14T11:05:58Z

FYI:
this.isVector3 is not allowed because of the test:

three.js/test/unit/src/core/Object3D.tests.js

Lines 243 to 247 in e9e5d0e

    
           assert.propEqual( obj.position, { 
        
           	x: 1, 
        
           	y: 1.23, 
        
           	z: - 4.56 
        
           } );

The properties of Vector3 can only contain x, y, z, and not isVector3.
But isVector3.prototype.isVector3 can pass the test.

I think we should declare is** as TheClass.prototype.is*** = true;

cc @DefinitelyMaybe @ianpurvis

DefinitelyMaybe · 2020-08-15T00:28:21Z

thank you! I was starting to look into why my unit tests were failing and was approaching a similar conclusion. I hadn't figured out how to work around it though. Great stuff!

cc @linbingquan

yomotsu added 3 commits August 3, 2020 13:19

Vector2

938a4cf

missing semi

881697f

Vector3, Vector4

9d14b87

yomotsu changed the title ~~Move to es6 classes/math~~ math: move to es6 classes Aug 3, 2020

solve hoisting in Vector3

979f99d

yomotsu commented Aug 3, 2020

View reviewed changes

Matrix3, Matrix4

b6e40eb

mrdoob added this to the r120 milestone Aug 3, 2020

DefinitelyMaybe mentioned this pull request Aug 3, 2020

Moving to ES6 Classes #19986

Closed

43 tasks

yomotsu added 3 commits August 3, 2020 20:47

Quaternion

22eca2d

Euler

5eef1d2

fix static value of Euler

f549b79

yomotsu marked this pull request as ready for review August 3, 2020 12:51

mrdoob reviewed Aug 3, 2020

View reviewed changes

move internal use instances to the end of the file

8bcfccd

mrdoob reviewed Aug 8, 2020

View reviewed changes

Euler.DefaultOrder should be writable

8000b03

revert Euler.RotationOrders to be writable

027a6b1

is*** should not be a getter function

a53fcce

mrdoob merged commit ff37eaa into mrdoob:dev Aug 14, 2020

mrdoob mentioned this pull request Aug 14, 2020

src/scenes: move to es6 classes #20007

Merged

yomotsu mentioned this pull request Aug 15, 2020

math area and space: move to es6 classes #20076

Merged

9 tasks

marcofugaro mentioned this pull request Aug 18, 2020

ES6 Classes: use Object.defineProperty() instead of accessing the prototype #20110

Merged

yomotsu mentioned this pull request Feb 23, 2021

add fitToSphere support for orthographic camera yomotsu/camera-controls#159

Merged

yomotsu deleted the move-to-es6-classes/math branch June 29, 2021 13:33

Mugen87 mentioned this pull request Jun 2, 2022

isVector3 is present when serializing Vector3 #24167

Closed

	import { MathUtils } from './MathUtils.js';
	import { Quaternion } from './Quaternion.js';

	let _vector;
	const _quaternion = new Quaternion();

	class Vector3 {

	import { Vector3 } from './Vector3.js';

	const _v1 = new Vector3();
	let _m1;
	const _zero = new Vector3( 0, 0, 0 );
	const _one = new Vector3( 1, 1, 1 );
	const _x = new Vector3();
	const _y = new Vector3();
	const _z = new Vector3();

	class Matrix4 {

	import { MathUtils } from './MathUtils.js';

	const _matrix = new Matrix4();
	const _quaternion = new Quaternion();

	class Euler {

Uh oh!

math: move to es6 classes #19997

math: move to es6 classes #19997

Uh oh!

Conversation

yomotsu commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yomotsu commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yomotsu Aug 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DefinitelyMaybe commented Aug 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yomotsu commented Aug 9, 2020

Uh oh!

mrdoob commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrdoob commented Aug 11, 2020

Uh oh!

yomotsu commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ianpurvis commented Aug 12, 2020

Uh oh!

mrdoob commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrdoob commented Aug 13, 2020

Uh oh!

ianpurvis commented Aug 13, 2020

Uh oh!

yomotsu commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrdoob commented Aug 14, 2020

Uh oh!

yomotsu commented Aug 14, 2020

Uh oh!

yomotsu commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DefinitelyMaybe commented Aug 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yomotsu commented Aug 3, 2020 •

edited

Loading

yomotsu commented Aug 3, 2020 •

edited

Loading

yomotsu Aug 9, 2020 •

edited

Loading

DefinitelyMaybe commented Aug 9, 2020 •

edited

Loading

mrdoob commented Aug 11, 2020 •

edited

Loading

yomotsu commented Aug 11, 2020 •

edited

Loading

mrdoob commented Aug 13, 2020 •

edited

Loading

yomotsu commented Aug 14, 2020 •

edited

Loading

yomotsu commented Aug 14, 2020 •

edited

Loading

DefinitelyMaybe commented Aug 15, 2020 •

edited

Loading