node material

[miko3k]

Why

I like Typescript and need the nodes material. It seems to be the only reasonable way to manage shaders.

Issue #137

What

Typings for the Node material.

I also increased minimum Typescript version to 4.0 because of variadic tuple types.

Type Inference works neatly for the ShaderNode, see an example. It's a bit hacky, but I find it sweet. Can be changed if needed.

This PR is against master as the feature is already present in r143.

I am excited about getting some feedback to this.

A few Typescript types are defined by me in this PR:

• types defined in nodes/core/constants.d.ts
• Swizzable and other stuff from ShaderNode.d.ts
• other utility types should be rather noncontroversial
  • various "scopes", they usually end with Scope, other examples are MathNodeMethod, OperatorNodeOp
  • search for export type in the source code to find out :)

Checklist

• [x] Checked the target branch (current goes master, next goes dev)
• [x] Added myself to contributors table
• [ ] Ready to be merged

[joshuaellis]

Wow, thanks for doing so much work! I'm sure you appreciate how large this PR is and i'll need to investigate it thoroughly to get it over the line.

I think to start, i'd like to know why you've had to update the minimum version of TS to 4.0? 😄

As a side note, typically unused files in examples have to be listed in OTHER_FILES.txt however, this is considered "bad practice" by DT and we should avoid it, to do so adding tests is the solution, I can see you've added some already & it'd be great to flesh more so we don't have to add any to the file, e.g. looking at the CI, CameraNode is not used but i'm sure we could sort this out.

[miko3k]

Wow, thanks for doing so much work! I'm sure you appreciate how large this PR is and i'll need to investigate it thoroughly to get it over the line.

Thanks, would be very nice to get feedback about this.

I think to start, i'd like to know why you've had to update the minimum version of TS to 4.0? 😄

Bacause Variadic Tuple Types to get reasonable type inference for the ShaderMaterial. Actually take a look straight here

https://github.com/three-types/three-ts-types/pull/240/files#diff-fd87ebe379f9463f9373481514ae11c90bad2329b64743bca9b1e76270f47fd7

That would be attempt to infer types of ShaderNode and friends, including overloaded constructors for the MathNode. It's questionable if you guys wanna have something like that in tree, however as I said, I find it sweet how syntax completion work so I decided to give it a try.

The rest is rather boring. That's also the reason why I didn't cover it by tests.

As a side note, typically unused files in examples have to be listed in OTHER_FILES.txt however, this is considered "bad practice" by DT and we should avoid it, to do so adding tests is the solution, I can see you've added some already & it'd be great to flesh more so we don't have to add any to the file, e.g. looking at the CI, CameraNode is not used but i'm sure we could sort this out.

Yes pretty much just the dreaded ShaderNode is tested for now.

Edit: of course I can and will gladly add more tests, I am open to suggestions how to do it properly. I could at least copy official examples of nodes here plus some random chunks of code in three.js three as I did for theShaderNode. An update of the PR will follow in some hours.

[Methuselah96]

I think to start, i'd like to know why you've had to update the minimum version of TS to 4.0? 😄

That should be fine anyway, the current support window on DefinitelyTyped is 4.0 and greater.

[miko3k]

I pushed an update, yarn run test-all is clean now and I also do not import DOM Node anywhere (ups).

Tests were extended by a few source files from three itself.

I unchecked that I am ready to merge as this obviously will need more work.

I also updated the initial description regarding the types I've taken liberty to define myself.

@joshuaellis One more note regarding your initial comment. Almost everything is used indirectly through Nodes.d.ts, although there are some omissions like the renderer related stuff.

[miko3k]

Okay I don't want to overload this PR with feedback.

Feedback is definitely appreciated and it's a big change. I would like to reach the mergable state eventually.

Overall a few points are we should avoid any as much as humanly possible, inferring the type through reading more threejs stuff would be better (even if its wrong) because people will fix it, if it's wrong (i know that sounds bad but i'm not suggesting we intentionally make it wrong).

This is a valid point. I will update the typings to reflect this. I generally used any as a synoynym for "object". Is there an agreement how to express an object in @types/three? {} or object? Object3D uses { [key: string]: any }; as userData. I would prefer Record<string,unknown>, I guess I will stick to to what Object3D does.

Secondly a lot of the parent classes e.g. Node do not take the arguments they're defined as in here, the children classes do and we should be reflecting this. It might be that we have to use abstract to help with this (not totally sure).

I disagree with abstract as every node would have to override everything and it's very common for nodes not to simply fall back to default implementation. I explained in one of my previous commets why including the parameters is the correct choice. I think what I did is both relatively simple and correct form OO perspective.

edit: I will add abstract where nodes explictly say console.warn( 'Abstract function.' );. Almost. Bacause Node.update() is only used when nodeUpdateType !== none.

Thirdly, it's still a great amount of work & a lot of it looks spot on, it's just about finessing the rough edges.

Thanks, I appreciate your recognition. I am sure there are bugs and the more can find now, the less breakage and anger for clients in the future.

[miko3k]

Anyways. The current status

any was ditched... I introduced aliases AnyJson, AnyObject and NodeUserData in constants.d.ts so it's more manageable. The "object" and the "userData" are the same thing and they are compatible with Object3D.userData. AnyJson is still an alias for any.

Other uses any have been replaced by an actual type. I am quite proud of LightsNode.setReference alhough noone will ever use it :)

abstract has been sparingly introduced where appropriate. As I said earlier, Node.update() cannot be abstract, as it is required only if updateType is not "none".

The point with inheritance and the respective types stays. Not sure how to proceed there. An example or closer description would be nice. No matter how I look at it, I cannot get the inheritance right.

[joshuaellis]

Hey @miko3k sorry, I've been away the last week so will try to look at this for you over the next couple of days.

[miko3k]

Wow! Let's be brave I guess. I am pretty excited about this :)