Issues and missing features on node type generics

[shotamatsuda]

The addition of node type generics in r183 is great. But it seems to have many breakages and need refinements, which appear to be already planned in #2049.

The purpose of this issue is to list the concrete issues and feature requests that are blocking me from migrating an existing codebase to r183 types as an example. I could work on most of them, but 2 and 3 would need your assistance because the underlying issues seem to be much broader.

1. Chained methods on Node<unknown>

Basic chained methods like toConst(), toVar(), context() and swizzling are absent unless the node has a type other than unknown. This leads to many type breakages.

This is understandable, but given that most advanced nodes (e.g. compute nodes and those in the addon) don't have definite types right now, it would be more reasonable to include these methods on Node<unknown>.

2. Function types on vectors

Most math functions should work with vectors component-wise, as well as integer types, but their types only accept float.

pow(vec3(), 1.5) // expected: Node<'vec3'>
sqrt(vec3()) // expected: Node<'vec3'>
fwidth(vec3()) // expected: Node<'vec3'>
greaterThan(vec3(), 0) // expected: Node<'bvec3'>
int(0).negate() // expected: Node<'int'>
ivec2().sign() // expected: Node<'ivec2'>

3. Coerced types on arithmetic operations

Arithmetic operations like vec2 * ivec2 and ivec2 * vec2 are allowed at the TSL level, and their types are coerced to vec2 * vec2(ivec2) -> vec2 and ivec2 * ivec2(vec2) -> ivec2 respectively, but these are not supported in the current types and result in type errors instead.

vec2().mul(ivec2()) // expected: Node<'vec2'>
ivec2().mul(vec2()) // expected: Node<'ivec2'>

4. Member types on StructNode

StructNode.get() always returns Node, so arithmetic operations require type casting.

It would be ideal to add generics to StructNode and resolve member types accordingly.

const S = struct({
  a: 'vec3',
  b: 'float'
})
const s = S(vec3(), float())
const a = s.get('a') // expected: Node<'vec3'>, actual: Node
const b = s.get('b') // expected: Node<'float'>, actual: Node

5. ivecN, uvecN, bvecN functions with VectorN instances

ivecN, uvecN, and bvecN functions should also accept VectorN instances, just like vecN functions do.

ivec2(new Vector2()) // expected: Node<'ivec2'>

6. Generics on compute nodes

Types for most compute shader nodes are missing. This would be largely alleviated if 1 is addressed.

globalId.xy // expected: Node<'ivec2'>

declare const storage: StorageBufferNode<'int'>
atomicAdd(storage.element(0), 1) // expected: Node<'int'>

7. Refinement on UniformNode inference

UniformNode now takes two type parameters, but the second one can be trivially inferred from the first.

const node: UniformNode<'vec3'> // UniformNode<'vec3', Vector3>

8. Array functions with struct

Array functions like instancedArray and attributeArray can take StructTypeNode. But this would require reworking the type of StorageBufferNode, especially if we will support generics on StructNode mentioned in 4.

const S = struct({ ... })
instancedArray(1, S) // expected: StorageBufferNode<StructNode>

9. TextureNode with DepthTexture always samples float

A quick workaround would be to overload PassNode.getTextureNode('depth') to return TextureNode<'float'>, because this is the most common way users access such nodes.

const passNode = pass(scene, camera)
const depthNode = passNode.getTextureNode('depth') // expected: TextureNode<'float'>

[Methuselah96]

Thanks for writing this up! I agree that the TSL types need a lot of refinement, sorry to break your code when upgrading. In this early stage of TSL I figured it was more important to get the changes started than to get it perfect on the first try. I look forward to fixing and/or providing feedback on these as I have time.

[Methuselah96]

#2125 should fix "3. Coerced types on arithmetic operations." In that PR I added implicit coercion from int or uint to float (and their vector variants), however I'm hesitant to allow float to be implicitly coerced to int or uint since oftentimes such a conversion can be a mistake since it will truncate the float. It seems safer to require the user to explicitly cast those using .toInt()/to.Uint() (or the vector versions of those methods).

Do you have any thoughts about this?

[shotamatsuda]

I understand your concern, but TSL's implicit type casting is already confusing to me, and I suppose the situation gets even worse if the types deviate from it.

I recently encountered the bug below, which would have been detected if the node type generics inferred the correct type.

const index = uniform('uint').setName('index')
const count = Math.sqrt(8)
const i = index.div(count).add(0.5) // expected: float, actual: uint
const a = i.div(count) // expected: float, actual: uint
// result: ( ( ( index / 3u ) + 1u ) / 3u )

I expected the above code to work because I'm too used to C-like types, where operand types are promoted to broader types (i.e. int × float -> float and float × int -> float), but TSL doesn't work that way.

The correct code was:

const index = uniform('uint').setName('index')
const count = Math.sqrt(8)
const i = float(index.div(count)).add(0.5) // actual: float
const a = i.div(count) // actual: float
// result: ( ( f32( ( index / 3u ) ) + 0.5 ) / 2.8284271247461903 )

Please note that the values being coerced in this case are a JS literal and number, which are first regarded as f32. If we disallow such operation in types, we will end up having to specify every JS literal used as operand with integer types, even when it might otherwise seem obvious:

uint(4).div(2) // type error because uint × float

[Methuselah96]

Yeah, there's not much we can do about JS literal numbers being able to represent either a float or integer type. But when they are Nodes we do know which type it is, in which case I do think it makes sense to prevent dangerous cases like the one you demonstrated.

For example, if we made count a Node<'float'> like so:

const index = uniform('uint').setName('index');
const count = float(Math.sqrt(8));
const i = index.div(count).add(0.5);
const a = i.div(count);

Before the changes in #2125, this would have type-checked fine, but after that PR, it will no longer pass type-checking since it knows that index.div(count) is potentially dangerous because it would be implicitly coercing a Node<'float'> to a Node<'uint'>. To fix the error, you would have to explicitly cast the index to a Node<'float'> using index.toFloat(), which would fix the latent bug as well.

So, given that example, I think that bolsters the case for not allowing implicit coercion from Node<'float'> to Node<'int'> or Node<'uint'>.

[shotamatsuda]

Yes, in this case or others, disallowing assignment of float to int can detect suspicious operations because the allowed types are narrower than TSL's actual behavior. However, it will also introduce many type breakages, and even legitimate example code in JS will fail to transpile without type errors, such as when passing JS literals on Node parameters.

I would suggest that the type behavior should adhere to the actual behavior of TSL. But after all, I really appreciate your effort on maintaining this awesome repository, I won't complain if you decide to go that direction.

[Methuselah96]

However, it will also introduce many type breakages, and even legitimate example code in JS will fail to transpile without type errors, such as when passing JS literals on Node parameters.

Can you provide some examples of what you're thinking of? I don't think I'm making any changes to where JS literals will be allowed, they should be allowed almost anywhere that a Node<'float'>, Node<'int'>, or Node<'uint'> is allowed.

Thank you for all your feedback so far, I really do appreciate it.

[shotamatsuda]

I don't think I'm making any changes to where JS literals will be allowed, they should be allowed almost anywhere that a Node<'float'>, Node<'int'>, or Node<'uint'> is allowed.

Sorry, then I think I got ahead of what you had in mind.😅 I thought if we didn't allow float to be implicitly coerced to integer types, all JS literals would also be disallowed by the types because they are implicitly cast to f32. So, am I correct that you were thinking of disallowing arithmetic functions from implicitly coercing one operand to a narrower result type?

const f = float(2)
const i = int(4)

f.div(i) // OK
i.div(f) // Type error

// Will these still be allowed?
i.div(4.2) 
vec2().element(1)

// How about these?
f.pow(i) // -> int
i.pow(f) // -> float

// Which cases will require explicit type casting?
f.step(i) // -> float
i.step(f) // -> int
step(i, f) // -> float
step(f, i) // -> int

To me, it's not clear which operand would require explicit type casting. My intuition is that, without such additional restrictions, type inconsistencies can usually be caught at assignment, or by reviewing the static analysis in editors.

const fn = Fn(([f, i]: [Node<'float'>, Node<'int'>]): Node<'float'> => {
  return i.div(f) // Type error
))

[Methuselah96]

Sorry, then I think I got ahead of what you had in mind.😅 I thought if we didn't allow float to be implicitly coerced to integer types, all JS literals would also be disallowed by the types because they are implicitly cast to f32.

Makes sense. The difference in my mind is that for JS literals we don't know whether it's intended to be a float or an int, but with Nodes we do. As you've pointed out, it would break too much existing code to disallow cases that use JS literals as integers.

So, am I correct that you were thinking of disallowing arithmetic functions from implicitly coercing one operand to a narrower result type?

That's correct.

// Will these still be allowed?
i.div(4.2) 
vec2().element(1)

Yes. The first one is allowed because we don't know whether the number is a float or integer at the type-level. The second one should also be allowed (but is currently broken and needs to be fixed).

// How about these?
f.pow(i) // -> int
i.pow(f) // -> float

The first is not allowed because it's rounding f to an int, and is therefore not "safe." The second is allowed because it's widening i to a float and is therefore "safe" (but this case is also currently broken and needs to be fixed).

// Which cases will require explicit type casting?
f.step(i) // -> float
i.step(f) // -> int
step(i, f) // -> float
step(f, i) // -> int

When it returns an int it will require explicit type casting (since it's implicitly rounding f to an int). When it returns a float it won't (since there's no rounding going on).

To me, it's not clear which operand would require explicit type casting.

Upcasting the int to a float will almost always be the way to go, so that the math is done using floats instead of ints.

My intuition is that, without such additional restrictions, type inconsistencies can usually be caught at assignment, or by reviewing the static analysis in editors.

const fn = Fn(([f, i]: [Node<'float'>, Node<'int'>]): Node<'float'> => {
  return i.div(f) // Type error
))

The problem is that it's common not to declare the return type, so it's easy for oversights to slip through. For example, your original example provided here is pretty obviously dangerous and hard to catch. I think it makes sense to prevent those cases when we can.

[Methuselah96]

I expected the above code to work because I'm too used to C-like types, where operand types are promoted to broader types (i.e. int × float -> float and float × int -> float), but TSL doesn't work that way.

What do you think about creating a three.js issue for this? I suspect most users have similar expectations and in my opinion it would make sense for the current logic to be changed.

This would also make most of these edge cases that you're presenting go away, since almost all of them would return a float instead of an int and therefore be safe.

[shotamatsuda]

Yes. The first one is allowed because we don't know whether the number is a float or integer at the type-level. The second one should also be allowed (but is currently broken and needs to be fixed).

Okay, that made it clear that even if your "safety" is introduced, the example I provided earlier would not be resolved, because the values coerced to integers were JS numbers and there is not much we can do about it.

The reason I added the pow example was that float ^ int is mathematically valid and there is no reason to type-cast the operands (that is because why there are pow2 and such). The step example shows how difficult it is to determine which operand should be regarded as being type-cast. Let's notice f.step(i) feels like f is being narrowed down, and step(i, f) feels like i is being narrowed (no, it's not narrowed), even though they are an identical expression.

To sum up, your plan is to make the type of all arithmetic functions to accept equal operand types only. Even then, I still think it is a premature workaround and should not be done in the types if possible.

What do you think about creating a three.js issue for this? I suspect most users have similar expectations and in my opinion it would make sense for the current logic to be changed.

Good point. I agree that this is the ideal way to address this arithmetic type issue.

I just refered to the WGSL spec of pow, and it says the input is f32 or f16, whereas in GLSL, the input type can be genType. If we can presume that the behavior of TSL should adhere more to WGSL, the operands should be first coersed to float types.

The situation is different on arithmetic expressions like + and *. Neither WGSL nor GLSL (at least ES3) defines the behavior of such expressions when operands have different types, although it makes more sense to first coerce the operands to the broadest type (though in JS, it has to be FP64...).

I anticipate it will be rejected because it might also introduce many breakages. Nevertheless, it's worth trying. I will give it a try.