Core: Switch to this.is* = true. #24047

marcofugaro · 2022-05-11T19:23:21Z

Related issue: #24006, Reverts #21293

Description

As illustrated by @Rich-Harris in this tweet (although let's not use class properties yet, we tried in the past).

The reason why three.js is using *.prototype.is* = true is to share memory across all instances by using the prototype.

However in the real world, having the is* boolean in the prototype or the instance, makes no difference at all (a boolean is just 1 byte of memory after all).
Here is a stress example with 10000 instances of the same material.

The first two snapshots are of the page with the *.prototype.is* = true, the last two snapshots are with this.is* = true. Somehow the memory decreased with this.is* = true.

Here is the code of the example in case anyone else wants to test.

<!DOCTYPE html>
<html lang="en">
	<head>
		<title>three.js webgl - box selection</title>
		<meta charset="utf-8">
		<meta name="viewport" content="width=device-width, user-scalable=no, minimum-scale=1.0, maximum-scale=1.0">
		<link type="text/css" rel="stylesheet" href="main.css">
		<style>
			body {
				background-color: #f0f0f0;
				color: #000;
				touch-action: none;
			}

			a {
				color: #08e;
			}

			.selectBox {
				border: 1px solid #55aaff;
				background-color: rgba(75, 160, 255, 0.3);
				position: fixed;
			}
		</style>
	</head>
	<body>

		<div id="info">
			<a href="https://threejs.org" target="_blank" rel="noopener">three.js</a> webgl - box selection
		</div>

		<!-- Import maps polyfill -->
		<!-- Remove this when import maps will be widely supported -->
		<script async src="https://unpkg.com/es-module-shims@1.3.6/dist/es-module-shims.js"></script>

		<script type="importmap">
			{
				"imports": {
					"three": "../build/three.module.js"
				}
			}
		</script>

		<script type="module">

			import * as THREE from 'three';

			import Stats from './jsm/libs/stats.module.js';

			import { SelectionBox } from './jsm/interactive/SelectionBox.js';
			import { SelectionHelper } from './jsm/interactive/SelectionHelper.js';

			let container, stats;
			let camera, scene, renderer;

			init();
			animate();

			function init() {

				container = document.createElement( 'div' );
				document.body.appendChild( container );

				camera = new THREE.PerspectiveCamera( 70, window.innerWidth / window.innerHeight, 1, 5000 );
				camera.position.z = 1000;

				scene = new THREE.Scene();
				scene.background = new THREE.Color( 0xf0f0f0 );

				scene.add( new THREE.AmbientLight( 0x505050 ) );

				const light = new THREE.SpotLight( 0xffffff, 1.5 );
				light.position.set( 0, 500, 2000 );
				light.angle = Math.PI / 9;

				light.castShadow = true;
				light.shadow.camera.near = 1000;
				light.shadow.camera.far = 4000;
				light.shadow.mapSize.width = 1024;
				light.shadow.mapSize.height = 1024;

				scene.add( light );

				const geometry = new THREE.BoxGeometry( 20, 20, 20 );

				for ( let i = 0; i < 10000; i ++ ) {

					const object = new THREE.Mesh( geometry, new THREE.MeshLambertMaterial( { color: Math.random() * 0xffffff } ) );

					object.position.x = Math.random() * 1600 - 800;
					object.position.y = Math.random() * 900 - 450;
					object.position.z = Math.random() * 900 - 500;

					object.rotation.x = Math.random() * 2 * Math.PI;
					object.rotation.y = Math.random() * 2 * Math.PI;
					object.rotation.z = Math.random() * 2 * Math.PI;

					object.scale.x = Math.random() * 2 + 1;
					object.scale.y = Math.random() * 2 + 1;
					object.scale.z = Math.random() * 2 + 1;

					object.castShadow = true;
					object.receiveShadow = true;

					scene.add( object );

				}

				renderer = new THREE.WebGLRenderer( { antialias: true } );
				renderer.setPixelRatio( window.devicePixelRatio );
				renderer.setSize( window.innerWidth, window.innerHeight );

				renderer.shadowMap.enabled = true;
				renderer.shadowMap.type = THREE.PCFShadowMap;

				container.appendChild( renderer.domElement );

				stats = new Stats();
				container.appendChild( stats.dom );

				window.addEventListener( 'resize', onWindowResize );

			}

			function onWindowResize() {

				camera.aspect = window.innerWidth / window.innerHeight;
				camera.updateProjectionMatrix();

				renderer.setSize( window.innerWidth, window.innerHeight );

			}

			//

			function animate() {

				requestAnimationFrame( animate );

				render();
				stats.update();

			}

			function render() {

				renderer.render( scene, camera );

			}

		</script>

	</body>
</html>

The root issue is caused by bundlers having difficulty tree-shaking classes with .proprtype.* accessors. The current status is:

rollup: doesn't support it, plans to support it
esbuild: doesn't support it, has no intention of supporting it
webpack: doesn't support it, no expressed intention

Bundler	Before this PR	After this PR
rollup
esbuild
webpack

But most importantly this makes importing from 'three' same as importing from 'three/src/Three', so no more people rambling about this on twitter.

LeviPesin · 2022-05-11T19:46:27Z

#22437

~~That were private fields. I think public are much more supported?~~ No, they are about the same.

LeviPesin · 2022-05-11T19:52:43Z

Please also do the same for other classes that are using prototype, like KeyframeTracks.

marcofugaro · 2022-05-11T19:54:03Z

Please also do the same for other classes that are using prototype, like KeyframeTracks.

examples/jsm is coming in a future PR.

LeviPesin · 2022-05-11T19:56:10Z

The KeyframeTracks are in core.

marcofugaro · 2022-05-11T19:59:05Z

Oh ok, didn't notice.
I handled all *.prototype.is* = true in this PR, we can handle the remaining prototype accessors in another PR as well.

looeee · 2022-05-12T00:30:58Z

we can handle the remaining prototype accessors in another PR as well.

There's a few more uses of .prototype outside the animation system.

In WebGLUniforms, HemisphereLight, Interpolant, and Color.

Inside the animation system there's quite a lot, not just in keyframe tracks, so that should probably be handled in its own PR.

Mugen87 · 2022-05-14T08:52:11Z

I'm fine with this change. Eventually we should not access *.prototype anywhere in the code base.

I handled all .prototype.is = true in this PR, we can handle the remaining prototype accessors in another PR as well.

Yes. Let's focus this PR just on the is* flags.

mrdoob · 2022-05-20T07:48:05Z

Thanks!

LeviPesin · 2022-05-23T03:20:23Z

I think this PR missed WebGL1Renderer...

I think there should be one more PR to fix remaining prototype accesses. I think those are ~~WebGL1Renderer, BoxHelper,~~ GeometryUtils, CameraControls, FontLoader, VolumeSlice, Volume, LWO2Parser, LWO3Parser, VTKLoader, VRMLLoader, GLTFLoader, EXRLoader, InterleavedBuffer, BufferAttribute, and Source (these three are using Array.prototype.slice( TypedBuffer ) instead of just TypedBuffer.slice()), possibly editor-related classes (EditorControls and History), possibly tests, possibly libs, and the animation system.

Mugen87 · 2022-05-23T07:14:25Z

Eventually we should not access *.prototype anywhere in the code base.

It seems this statement goes too far. Let's remove prototype more selectively where it "makes sense". In certain use cases, it's okay to use prototype (as long as it does not conflict with tree-shaking).

LeviPesin · 2022-05-23T07:18:14Z

Very often it does not make sense to have it - e.g. in cases like Array.prototype.slice.apply( typedBuffer ) or let arr = []; Array.prototype.push.apply( arr, element ). I listed both tree-shaking and such cases (altough they are completely unrelated to each other), but not the cases where it does make sense.

marcofugaro · 2022-05-23T07:34:35Z

I think this PR missed WebGL1Renderer...

It was intetionally left out, the isWebGL1Renderer flag is used in the constructor of WebGLRenderer. You can't achieve this behaviour without accessing the prototype.

Mugen87 · 2022-05-23T07:39:37Z

Good to know! Eventually, WebGL1Renderer gets removed at some point so this issue should disappear automatically.

* Revert to this.is* = true * Put back local variables * Update tests * Add missing this.is* property * Update Color.js Co-authored-by: Michael Herzog <michael.herzog@human-interactive.org>

marcofugaro added 3 commits May 11, 2022 19:55

Revert to this.is* = true

f1dcc72

Put back local variables

27c4183

Update tests

85cfe32

marcofugaro mentioned this pull request May 11, 2022

Tree-shaking broken by Object.defineProperties( ... ) #24006

Closed

Add missing this.is* property

d7be078

Mugen87 approved these changes May 14, 2022

View reviewed changes

This was referenced May 14, 2022

HemisphereLight: Use super in copy(). #24060

Merged

Color: Do not use prototype for r/g/b properties. #24061

Merged

WebGLUniforms: Avoid usage of prototype. #24069

Merged

Mugen87 added 2 commits May 17, 2022 10:22

Merge branch 'dev' into remove-prototype-access

73a3795

Update Color.js

116262a

Mugen87 changed the title ~~Switch to this.is* = true~~ Core: Switch to this.is* = true. May 17, 2022

Mugen87 mentioned this pull request May 17, 2022

Interpolant: Remove beforeStart_() and afterEnd_() alias definitions. #24076

Merged

mrdoob added this to the r141 milestone May 20, 2022

mrdoob merged commit 6e897f9 into mrdoob:dev May 20, 2022

Mugen87 mentioned this pull request May 20, 2022

Examples: Switch to this.is* = true. #24092

Merged

RenaudRohlinger mentioned this pull request May 20, 2022

Core: Switch to Object.defineProperty( this, 'is*', { value: true } ); #24093

Closed

Mugen87 mentioned this pull request Jun 1, 2022

isVector3 is present when serializing Vector3 #24167

Closed

LeviPesin mentioned this pull request Jun 2, 2022

Cleanup #24181

Merged

vatro mentioned this pull request Jul 2, 2022

PropUtils type checking not working properly in SvelteKit dev mode with few routes vatro/svelthree#115

Closed

Mugen87 mentioned this pull request Apr 7, 2024

Vector3.prototype.isVector3 = true is being set inside the constructor of Vector3 class. #28079

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Switch to this.is* = true. #24047

Core: Switch to this.is* = true. #24047

marcofugaro commented May 11, 2022

LeviPesin commented May 11, 2022 •

edited

LeviPesin commented May 11, 2022

marcofugaro commented May 11, 2022 •

edited

LeviPesin commented May 11, 2022 •

edited

marcofugaro commented May 11, 2022

looeee commented May 12, 2022

Mugen87 commented May 14, 2022

mrdoob commented May 20, 2022

LeviPesin commented May 23, 2022 •

edited

Mugen87 commented May 23, 2022 •

edited

LeviPesin commented May 23, 2022 •

edited

marcofugaro commented May 23, 2022

Mugen87 commented May 23, 2022 •

edited

Core: Switch to this.is* = true. #24047

Core: Switch to this.is* = true. #24047

Conversation

marcofugaro commented May 11, 2022

LeviPesin commented May 11, 2022 • edited

LeviPesin commented May 11, 2022

marcofugaro commented May 11, 2022 • edited

LeviPesin commented May 11, 2022 • edited

marcofugaro commented May 11, 2022

looeee commented May 12, 2022

Mugen87 commented May 14, 2022

mrdoob commented May 20, 2022

LeviPesin commented May 23, 2022 • edited

Mugen87 commented May 23, 2022 • edited

LeviPesin commented May 23, 2022 • edited

marcofugaro commented May 23, 2022

Mugen87 commented May 23, 2022 • edited

LeviPesin commented May 11, 2022 •

edited

marcofugaro commented May 11, 2022 •

edited

LeviPesin commented May 11, 2022 •

edited

LeviPesin commented May 23, 2022 •

edited

Mugen87 commented May 23, 2022 •

edited

LeviPesin commented May 23, 2022 •

edited

Mugen87 commented May 23, 2022 •

edited