Add TypeScript defintions

[yfunk]

Description

Fixes: #18

This pull request adds TypeScript definitions for everything that is exported from the package root.

They were written and used for a project I am working on, so they are already somewhat tested to behave as expected (though I did have to change and update a few things, so further testing may be necessary).

Since there are some things left to do, I'll mark this as a draft for now.

Notes

• Every public class member (without a _ prefix) was added to the corresponding definition
• No JSDoc comments were added in accordance with the discussion at https://github.com/gkjohnson/three-mesh-bvh/
• The @types/node dependency was added to fix an error in the global definitions that would show up when running tsc

Since this project doesn't use a formatter or strict ESLint rules for code style, I tried my best to match it to the rest of the project.

• Adding empty lines between all class members made the code really difficult to read, so I decided against that and grouped them into blocks (see below).
• I used regions to mark the different areas of the code, since VSCode allows you to fold those

Formatting and grouping of class members

class <name> {

        constructor

	<readonly fields>

	<configuration fields>

	<data fields>

	<callbacks>

	<set / get methods>

	<methods>

}

To-do

• Fix PhysicalPathTracingMaterial overriding property with different type
• Add proper type for BVH Worker (not exported from three-mesh-bvh)
• Find a way to represent additional material properties supported by WebGLPathTracer (docs)
• Investigate TypeScript examples to ensure the definitions work as expected

[yfunk]

ShaderMaterial already uses lights for something else (docs) and TypeScript doesn't allow overriding properties with a different type.

Property 'lights' in type 'PhysicalPathTracingMaterial' is not assignable to the same property in base type 'MaterialBase<PhysicalPathTracingMaterialDefines>'.
  Type 'LightsInfoUniformStruct' is not assignable to type 'boolean'.ts(2416)

With type you could just omit the property (type Fixed = Omit<ShaderMaterial, "lights">), but afaik there is no way to do this with classes and interface.

So lights either needs to be renamed or MaterialBase can't extend ShaderMaterial.

[gkjohnson]

With the recent refactor I'm thinking we may not need to export this material which would simplify things quite a bit, I think.

[yfunk]

Even if this material may not be exported in the future, I think the TypeScript error is raising a valid concern.

Due to subtyping, any class that extends ShaderMaterial is expected to be compatible with its interface. Overriding lights with a LightsInfoUniformStruct may break other code that expects it to be a boolean as defined on ShaderMaterial in unexpected ways.

How do you feel about renaming this field to lightsInfo?

[gkjohnson]

In practice this isn't an issue especially if the class is not made public but I understand what you're saying - I'm happy with a change to "lightsInfo".

[gkjohnson]

Thanks! This is great. The project has just undergone a pretty big refactor so I may be limiting the API that's actually exported soon to make things easier to maintain. In practice I think the fields and classes I have documented are the ones I would consider "public" and worth adding typescript definitions for. But I'm happy to hear your thoughts, as well.

Since this project doesn't use a formatter or strict ESLint rules for code style, I tried my best to match it to the rest of the project.

The project does use ESLint for code style. You should be able to use npm run lint -- --fix to fix the files.

Find a way to represent additional material properties supported by WebGLPathTracer (docs)

You should be able to extend existing classes similar to how this is done for Raycaster in three-mesh-bvh

Add TypeScript examples to ensure the definitions work as expected

I'm not a typescript expert so I don't know if I feel we need typescript examples in the project but it's good to validate.

[yfunk]

The project does use ESLint for code style. You should be able to use npm run lint -- --fix to fix the files.

I did use ESLint auto-fix, but what I meant was that the rules in the config are not very strict, so there is a lot of "wiggle room" for how you format the code. For example, you could add 10 empty lines between two class members or indent each of them differently without ESLint complaining or auto-fixing it.

You should be able to extend existing classes similar to how this is done for Raycaster in three-mesh-bvh

This would extend the type across the entire application, right? I am not sure how I feel about that, especially for large projects. Someone working on an entirely different part of the app could misinterpret the materials having a matte property as WebGLRenderer also supporting that. Do any other libraries except three-mesh-bvh do something like this?

Maybe these properties could be implemented as an extended material provided by three-gpu-pathracer instead, similar to PhysicalSpotLight or ShapedAreaLight? Alternatively, we could add JSDoc comments to these properties that specify they only work with three-gpu-pathtracer.

I'm not a typescript expert so I don't know if I feel we need typescript examples in the project but it's good to validate.

As far as I know, copying the examples and changing the extension to .ts should get you 95% of the way there. Maybe TypeScript could even check the .js files directly, I'll investigate this further.

Having TypeScript examples that are checked during linting could help prevent the implementation and type definitions from getting out of sync in the future, so I think this is something worth considering.

I may be limiting the API that's actually exported soon to make things easier to maintain. In practice I think the fields and classes I have documented are the ones I would consider "public" and worth adding typescript definitions for. But I'm happy to hear your thoughts, as well.

Generally, I prefer when libraries export as much of the API as possible (even if it is undocumented), but I understand the burden of maintaining type definitions for everything.

With the modular way three-gpu-pathtracer is built, you can just take PathTracingRenderer as a framework and implement your own path tracing material or make a copy of PhysicalPathTracingMaterial to modify it (which I did in my project). I haven't tried the new WebGLPathTracer yet, but currently I think that everything necessary to roll your own version of it should stay exported. Of course, you could still use deep imports for this, but not having any type definitions for something you import is a huge pain in TypeScript projects (and writing definition files for deep imports even more so).

Overall, it comes down to whether you think it is viable to maintain the type definitions for these parts of the API, and how much you expect them to change in the future. But in my opinion, there is definitely value in having them.

[gkjohnson]

there is a lot of "wiggle room" for how you format the code

Gotcha - this must just be the case for typescript files, then.

This would extend the type across the entire application, right?

I'm not sure how this would be avoidable. The function takes a material class and we're extending it to expect a "matte" or "castShadow" member on it. There would be no way for a syntax detection system to know if user code was using a material with the intent of passing it into the path tracer or not.

Do any other libraries except three-mesh-bvh do something like this?

Again I'm not a typescript expert so I can't say. I add these declaration files because people ask for or contribute them and I have to fix them when people complain.

Alternatively, we could add JSDoc comments to these properties that specify they only work with three-gpu-pathtracer.

Where would these comments live? In the typescript file? In that case I'd be okay with it for these two fields.

Having TypeScript examples that are checked during linting could help prevent the implementation and type definitions from getting out of sync in the future, so I think this is something worth considering.

I understand but this means more to maintain and fix when something changes. I'd rather not add code for a solution for a problem we don't have, yet unless someone wants to take on responsibility for maintaining this part of the project.

Generally, I prefer when libraries export as much of the API as possible (even if it is undocumented), but I understand the burden of maintaining type definitions for everything.

Similarly this puts a lot of restrictions on the internals of the project and means having to consider changes to any part of the API because they're technically "public". I know it might be nice as a user but as a maintainer it's a nightmare. One of the benefits of encapsulating the path tracer in a more wholistic class is the internals can change more freely without worrying about other projects breaking. I don't think it's reasonable to try to maintain code to support non-specific use cases that may not exist.

I'd much rather only export what is intended to be used and documented and if someone wants something publicly we can talk about exporting it or understand the use case and how WebGLPathTracer class can be improved to support it more easily.

Again I appreciate the PR and type definitions - I think it's a good change - but lets start with the minimal additions and when we run into problems we can add a more well-considered solution for it.

[yfunk]

Additional Material properties

I'm not sure how this would be avoidable.

The only alternative I can think of (as mentioned) is exporting separate material classes that have those properties, similar to PhysicalSpotLight or ShapedAreaLight. That also would allow copy and clone to support them.

Where would these comments live? In the typescript file?

Yes, just like the deprecation comments I added for the old scene generator classes (1, 2). Editors like VSCode can pick those up and show them when you hover the property.

People often seem to use VSCode auto-complete instead of reading the documentation, so I wanted to make sure there was some sort of notice that these properties are not defined by Three.js itself. Unless you want to expose separate material classes, I think adding a comment is enough to avoid confusing users.

TypeScript examples

I understand but this means more to maintain and fix when something changes.

Absolutely, maintaining two sets of examples is not viable at all.

The most interesting solution would have been to check the existing JS examples using the TypeScript compiler (since it can do this with the checkJs option). I looked into this yesterday, but with the way the examples are written, it's not possible without further modifications, as there is some code where TypeScript can't infer the type from usage and needs explicit annotations.

With the exposed API being limited and the scope of the type definitions becoming narrower, this is no longer as relevant as before anyway.

Exposed API

I'd much rather only export what is intended to be used and documented and if someone wants something publicly we can talk about exporting it or understand the use case and how WebGLPathTracer class can be improved to support it more easily.

This makes sense, and I agree that exposing the internal API will probably only be relevant for a very small number of use cases that don't warrant the additional maintenance overhead.

Lets start with the minimal additions and when we run into problems we can add a more well-considered solution for it.

Sounds good, then I guess I'll wait for you to limit the exported API and adjust the type definitions accordingly.

[gkjohnson]

The only alternative I can think of (as mentioned) is exporting separate material classes that have those properties, similar to PhysicalSpotLight or ShapedAreaLight

If it's a separate material, if someone imports a glTF then for each object that we want to adjust these fields on - the material must be replaced, previous fields copied, and then members set. You have to strike a balance between technical idealism with practical ease-of-use for the end user and you can't always have both. The latest refactor has been focused on making the project easier to use so I'm hesitant to make changes that I know will be more cumbersome. This is my current feeling, at least.

These fields could be added to material.userData but I'll have to think about that a bit more.

People often seem to use VSCode auto-complete instead of reading the documentation, so I wanted to make sure there was some sort of notice that these properties are not defined by Three.js itself.

Okay great that sounds fine to me then.

Sounds good, then I guess I'll wait for you to limit the exported API and adjust the type definitions accordingly.

I've just merged #627 which adjusts the exports to a more limited scope. I've also marked some classes as "deprecated" which we shouldn't have types added for. Related to that I'll also be removing IESLoader in another PR in favor of using three.js' built in one, instead.

Thanks for bearing with me in figuring all this out!

[yfunk]

I just updated the type definitions to match the current exports, so this PR should be ready now. Feel free to take another look and request changes if necessary.

I also removed the declaration of StaticGeometryGenerator, which isn't exported but is technically still accessible via PathTracingSceneGenerator. I am not sure if there is a valid use case for changing the config fields on this class (useGroups, applyWorldTransforms etc.), let me know if you think it should be added back in.

Also, for future reference, or if anyone needs them for importing internals, here are the full type definitions.

[gkjohnson]

This is great, thank you! Once the next release is out let me know if you run into any issues with the new API and we can try to figure out how to accommodate those use cases.