Core: Vector, Matrix, Quaternion and Color instanceof

[pschroen]

Related issues: #24219 #24199 #24167 #24199 (comment)

Description

Defining .prototype.is* inside the constructor is a bit of a hack, really the .prototype.* properties should be defined outside the classes once like in the rest of the library for consistency. 🙄

this wins the most-weird-thing-in-the-library award imho 😁

Given that, I've gone ahead and trying the instanceof method here, in my opinion I think using instanceof with tree-shaking is fine, because really you do need all those classes if you have code referencing them. Especially for core classes that are needed anyways, and many classes are already importing the classes used with their respective .is* properties anyways.

The syntax for using instanceof is often simpler as well without the need for checking the value first or use of optional chaining.

The epic #9310 from @Rich-Harris I would argue is a stopgap solution, it is possible to write a tree-shakeable library with instanceof, it's more of a design problem with the renderer, thus the stopgap solution. For example in OGL the renderer there only needs a Vec3.

https://github.com/oframe/ogl/blob/master/src/core/Renderer.js

This is a much larger discussion for a different issue, but wanted to mention it here as it's related to the use of instanceof. Though for comparison OGL doesn't use any instanceof's either.

Note I didn't do .isTexture, I feel like I'm opening Pandora's box here, so let's see what everyone thinks of this change first.

Also if we were to completely remove the .is* properties it would be a breaking change, they are used in the examples, tests, docs and more than likely other people are using them.

It's up for debate on whether this actually has any performance improvement or not. 😊

Finally, if instanceof doesn't work-out, I would at-least prefer setting the non-enumerable property with Object.defineProperty/ies() inside the constructor instead, as is done with LOD and NodeCode already.

three.js/examples/jsm/nodes/core/NodeCode.js

Line 9 in 3a76bc7

Object.defineProperty( this, 'isNodeCode', { value: true } );

.isColor instanceof Color/THREE.Color
.isVector2 instanceof Vector2/THREE.Vector2
.isVector3 instanceof Vector3/THREE.Vector3
.isVector4 instanceof Vector4/THREE.Vector4
.isMatrix3 instanceof Matrix3/THREE.Matrix3
.isMatrix4 instanceof Matrix4/THREE.Matrix4
.isQuaternion instanceof Quaternion/THREE.Quaternion

[donmccurdy]

thanks @pschroen!

in my opinion I think using instanceof with tree-shaking is fine, because really you do need all those classes if you have code referencing them. Especially for core classes that are needed anyways, and many classes are already importing the classes ...

Could you expand on "you do need all those classes" a bit more here? For math classes I think I follow, you're probably not going to do anything practical with three.js without math classes. But things like THREE.SkinnedMesh, THREE.Points, or THREE.Lines are only sometimes used — the renderer doesn't need them, but it needs to detect what they are when it gets them as input. These are cases where .is* makes a difference. I'm not sure we'd make this change just for the math classes if it doesn't generalize to the rest of the codebase...

After this change, what should be tree-shaken that wasn't before?

[pschroen]

Hey @donmccurdy 👋,

True if there isn't any performance improvement or a big difference in tree-shaking then I personally do like the .is* properties for consistency, they're used everywhere including in the NodeMaterial System.

This is more of a follow-up PR to @mrdoob's #24199 (comment)

At the very least if we don't find instanceof worthwhile I'd still like to change the .prototype.is* inside the constructors from #24219 to the same Object.defineProperty() syntax used in the NodeMaterial System.

For tree-shaking I've tested this PR in both Rollup and webpack just now and there's no difference in either, which is expected with this PR, as the .is* property checks and instanceof checks are optimized the same between bundlers.

It is worthwhile noting though that adding instanceof to these math classes also didn't increase the bundle size either, they output the same chunks, which goes to show in this case at-least using instanceof with tree-shaking does work fine, in other words, it doesn't make it worse here. 😅

[marcofugaro]

Defining .prototype.is* inside the constructor is a bit of a hack, really the .prototype.* properties should be defined outside the classes once like in the rest of the library for consistency.

Sorry but I don't see the purpose of this PR, is it just to make the code a little bit cleaner?
If so, doesn't look like there are much advantages to this PR, considering that this PR is introducing a breaking change by deprecating isVector3 and similar.

For tree-shaking I've tested this PR in both Rollup and webpack just now and there's no difference in either, which is expected with this PR, as the .is* property checks and instanceof checks are optimized the same between bundlers.

This statement is wrong.
Using instanceof prevents tree-shaking both in rollup and webpack.
I don't know how you've been testing this, but I've made a repro supporting my statement.

https://github.com/marcofugaro/treeshake-test/blob/master/src/index-instanceof.js

You can test it yourself by running npm i and then npm run build-instanceof.

[marcofugaro]

this creates performance issues, see #21284

[pschroen]

Hey @marcofugaro, I was tree-shaking just these math classes with instanceof as a follow-up PR to @mrdoob's #24199 (comment)

Perhaps I should have created this as a Draft PR for discussion. Tree-shaking these math classes with instanceof makes no difference in the final bundle, and yes using instanceof anywhere else like in your example or WebGLRenderer will not work.

It was also to test performance, but if we already know it's worse for performance we can close this issue then.

[LeviPesin]

But can we not replace these .is* with their "object definition" versions, but instead just leave their definition as they are (and mark them deprecated) and just change their uses to instanceof?

[marcofugaro]

just change their uses to instanceof?

Why? Could you explain the purpose of this?

[pschroen]

Why? Could you explain the purpose of this?

Because that was the ask as a possible alternative approach to solving #24167, this is less about tree-shaking really, it only came-up in our tree-shaking conversation because of .prototype.is*. See #24199 (comment)

I will say one last thing here though about all the confusion of instanceof and tree-shaking, I'm not proposing we use instanceof in the rest of the library, and I'm fine with this hybrid approach if it improves performance, but it's also at the cost of a pretty large breaking change.

Just as in OGL, we would never use instanceof for type checks with optional modules, and why we've been using .is* all these years. My point with calling #9310 a stopgap solution is really this is a design problem that would require a pretty big rewrite of the WebGL* files, and would be the much larger discussion I was referring to.

To make the renderer fully tree-shakeable we'll need to essentially remove all the type checks and invert the design. In other words, like with making the shaders tree-shakeable, all renderer modules will need to be self-contained with the context passed to them, rather than one giant renderer, it's the opposite approach really, but that's a conversation for another day. 🫠