Skip to content

Identify Three.js objects across module copies - #443

Closed
oscarlorentzon wants to merge 1 commit into
sparkjsdev:mainfrom
oscarlorentzon:fix-three-instance-checks
Closed

oscarlorentzon wants to merge 1 commit into
sparkjsdev:mainfrom
oscarlorentzon:fix-three-instance-checks

Conversation

@oscarlorentzon

Copy link
Copy Markdown
Collaborator

Three.js objects were identified with instanceof, which fails when an application loads a second copy of the three module. An application passing objects from the second three copy got no translation from PointerControls, no environment map from recurseSetEnvMap, a throw or a NaN literal from DynoConst.getLiteral, and a level of detail limited only by the splat budget, never by on-screen size. All 20 checks now read the boolean marker Three.js sets on every object it creates, such as isVector3. Three.js identifies its own objects by the marker, and its build never uses instanceof on a Three.js class. A single copy of three behaves as before.

Also raised in #250.

Three.js objects were identified by their class, which fails when an
application loads a second copy of the module, so the checks now read the
flag Three.js sets on every object it creates.
@mrxz

mrxz commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Three.js objects were identified with instanceof, which fails when an application loads a second copy of the three module.

Multiple instance of three being included in an application is generally not supposed to happen. It can easily lead to issues, especially if the instances are different versions, but even if they match it can still cause unexpected problems. Some objects have auto incrementing IDs, which can lead to duplicates as each instance has its own counter. The Nodes system even does comparisons based on constructors, which would fail when encountering ones from a different instance.

The changes themselves are fine. It is more idiomatic for Three to discriminate types through the isXyz properties, though with TypeScript it has the unfortunate problem that it doesn't automatically narrow the type that way.

In short, I don't think we should make these changes to "support" setups with multiple instance of Three. Even if it might resolves some issue, the setup remains broken and can cause problems down the line for the user. As for using guards vs instanceof, I personally think the latter is more readable and convenient, despite not being idiomatic Three.


One change that might still be interesting is for the DynoConst. Instead of checking for actual Vector3 instances, we can be more lenient and check for Vector3Like by using a guard function that checks if the required properties (x, y, z) are present.

@oscarlorentzon

Copy link
Copy Markdown
Collaborator Author

Agree that the multi-instance three case could lead to other errors and may not be important to cover.

The guard functions narrow the type, since each is declared value is T, so that is not a problem here.

Still, instanceof is simpler and needs no extra code, so let's keep it. #446 switches the two remaining isTexture checks the other way, so Three.js types are identified in one way across the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants