Ray: Convert to es6 class

[ianpurvis]

#11552

[Mugen87]

Please do not make a PR per class^^. Consider to bundle your changes into a single one.

[ianpurvis]

@Mugen87 Sorry about that- I want to make it easy to review, do you prefer like a monolithic PR with many reviewable commits?

[Mugen87]

Ideally you make a single commit/PR per directory. It least it was my plan to do it this way.

[Mugen87]

If we make a PR per class the upcoming release notes will look like a nightmare^^.

Besides, the review effort for most of the files should be small. Especially since all IIFE are already gone.

[ianpurvis]

Lol, my bad!!!

[Mugen87]

I wonder if there is a tool that can convert the files for us. It was not possible to do this earlier because of the IIFEs. But these are gone since a while now.

[Mugen87]

For more context: #14654

Here is the tool the OP used: https://github.com/Rich-Harris/convert-threejs-to-classes#follow-up-work

[ianpurvis]

I think that could be possible, the replacement pattern is so repetitive... I'll check it out.

Btw, do you want the existing PRs closed and combined into something new? Or just move forward?

[DefinitelyMaybe]

noted.

[Mugen87]

Btw, do you want the existing PRs closed and combined into something new? Or just move forward?

The existing PRs can be merged of course. But it would be great if you or @DefinitelyMaybe could check out the repo of @Rich-Harris. As you said, since the pattern is repetitive, it would be nice to automate this process.

[Mugen87]

Just for clarification: In the past, the critical part was the introduction of module scope variables in order to avoid closure scoped variables in IIFEs. This required actually a lot of testing (and some hotfixes^^). The resulting codebase should be easier to process for a tool in order to produce proper ES6 classes.

[DefinitelyMaybe]

if all of the IFEEs are gone then maybe. I remember that being a pain when I was trying to regex the entire library

[Mugen87]

Possibly interesting for ES6 conversions: https://github.com/lebab/lebab

[Mugen87]

It turns:

function CircleGeometry( radius, segments, thetaStart, thetaLength ) {

	Geometry.call( this );

	this.type = 'CircleGeometry';

	this.parameters = {
		radius: radius,
		segments: segments,
		thetaStart: thetaStart,
		thetaLength: thetaLength
	};

	this.fromBufferGeometry( new CircleBufferGeometry( radius, segments, thetaStart, thetaLength ) );
	this.mergeVertices();

}

CircleGeometry.prototype = Object.create( Geometry.prototype );
CircleGeometry.prototype.constructor = CircleGeometry;

into

class CircleGeometry extends Geometry {
    constructor(radius, segments, thetaStart, thetaLength) {

        super();

        this.type = 'CircleGeometry';

        this.parameters = {
            radius,
            segments,
            thetaStart,
            thetaLength
        };

        this.fromBufferGeometry( new CircleBufferGeometry( radius, segments, thetaStart, thetaLength ) );
        this.mergeVertices();

    }
}

[Mugen87]

Another example:

function LineBasicMaterial( parameters ) {

	Material.call( this );

	this.type = 'LineBasicMaterial';

	this.color = new Color( 0xffffff );

	this.linewidth = 1;
	this.linecap = 'round';
	this.linejoin = 'round';

	this.morphTargets = false;

	this.setValues( parameters );

}

LineBasicMaterial.prototype = Object.create( Material.prototype );
LineBasicMaterial.prototype.constructor = LineBasicMaterial;

LineBasicMaterial.prototype.isLineBasicMaterial = true;

LineBasicMaterial.prototype.copy = function ( source ) {

	Material.prototype.copy.call( this, source );

	this.color.copy( source.color );

	this.linewidth = source.linewidth;
	this.linecap = source.linecap;
	this.linejoin = source.linejoin;

	this.morphTargets = source.morphTargets;

	return this;

};

to

class LineBasicMaterial extends Material {
    constructor(parameters) {

        super();

        this.type = 'LineBasicMaterial';

        this.color = new Color( 0xffffff );

        this.linewidth = 1;
        this.linecap = 'round';
        this.linejoin = 'round';

        this.morphTargets = false;

        this.setValues( parameters );

    }

    copy(source) {

        super.copy(source);

        this.color.copy( source.color );

        this.linewidth = source.linewidth;
        this.linecap = source.linecap;
        this.linejoin = source.linejoin;

        this.morphTargets = source.morphTargets;

        return this;

    }
}

LineBasicMaterial.prototype.isLineBasicMaterial = true;

[ianpurvis]

Looks great!

[Mugen87]

You can try it out yourself with: https://lebab.unibtc.me/editor/

[Mugen87]

It seems it can't process this style:

function Bone() {

	Object3D.call( this );

	this.type = 'Bone';

}

Bone.prototype = Object.assign( Object.create( Object3D.prototype ), {

	constructor: Bone,

	isBone: true

} );

But maybe one could ask at the repository of lebab what to do in this case.

If the tool could get this to work, most of conversion could be nicely automated.

[ianpurvis]

@Mugen87 Thanks for the info. Do you guys foresee issues with classes extending a VIP like Object3D? (for better or worse I asked on the original issue thread #11552 (comment))

[mrdoob]

Please do not make a PR per class^^. Consider to bundle your changes into a single one.

Actually, a PR per class id easier to review than bundling all the changes in one. So I favour PR per class.

If we make a PR per class the upcoming release notes will look like a nightmare^^.

Yes it will... But it will only be a nightmare once 😇

[mrdoob]

A PR per folder is also okay, depending on the complexity. For example, renderers folder should definitely be per class. geometries folder could be per folder.

It's a case of one size don't fit all, so lets be flexible. Probably better to ask first before doing the PR?

[mrdoob]

I also wouldn't go the automation path, specially if it's a blocker because it doesn't support some of out patterns.

I know it is repetitive, but we only have to do this once.

[mrdoob]

Thanks!

[DefinitelyMaybe]

Yes it will... But it will only be a nightmare once 😇

tehe! to be fair it is likely to end other nightmares 👻

It's a case of one size don't fit all, so lets be flexible. Probably better to ask first before doing the PR?

roger. a little co-ordination perhaps.

[mrdoob]

@Mugen87 would doing the change log like this help?

* Source

* Global
 * Adopted ES6 Classes. #19976 #19977 #19978 #19979 #19980 (@DefinitelyMaybe , @linbingquan, @Mugen87)

[Mugen87]

Definitely. But frankly I would have done it like that anyway^^. It's the same approach we have used for the module migration last year. I just remember that it took a lot of time to process all exiting PRs.

[ianpurvis]

A PR per folder is also okay, depending on the complexity. For example, renderers folder should definitely be per class. geometries folder could be per folder.

This sounds good to me. One of the reasons that I went the per-file route and jumped around in the tree was that I kept getting blocked by #19982. Once we can resolve that issue, it will be a lot easier to PR a whole folder.

roger. a little co-ordination perhaps.

@DefinitelyMaybe Your dibs system is a great idea. I'll keep pushing it forward with you in #11552 👍

Thanks everybody- sorry to rock the boat