[p5.strands] should instanceID() be auto-converted to a float?

[davepagurek]

Topic

I was working on this sketch https://openprocessing.org/sketch/2664809 and instanceID() / 10 gives a type error, but 0.1 * instanceID() does not. For consistency and to reduce the number of times a programmer has to manually convert between numeric types, do you think we should just auto-convert instanceID() to a float behind the scenes?

[perminder-17]

Topic

I was working on this sketch https://openprocessing.org/sketch/2664809 and instanceID() / 10 gives a type error, but 0.1 * instanceID() does not. For consistency and to reduce the number of times a programmer has to manually convert between numeric types, do you think we should just auto-convert instanceID() to a float behind the scenes?

Hi @davepagurek, thanks for bringing this up. It’s a great discussion. I haven’t used p5.strands, but instanceID() corresponds to gl_InstanceID, which is declared as an int. According to the OpenGL spec, gl_InstanceID contains the index of the current primitive in an instanced draw call.

It says ;

gl_InstanceID — contains the index of the current primitive in an instanced draw command
gl_InstanceID is a vertex language input variable that holds the integer index of the current primitive

Since array indexing requires an integer, dynamic lookups must be done with an int. Integer IDs remain useful for indexing arrays or performing lookups, If we auto-converted instanceID() to a float, the user would still have to cast it back to an int whenever they needed, which is inconvenient.

Also, with very large draw calls, a 32-bit float loses exact integer precision beyond a certain threshold.

I don't know these two points still an issue here but if it matters, without chainging instanceID() to float behind the scene, how about creating a new one named instanceIDf() whose type is float and let the instanceID() one remain int.

If these are not the concern users gonna use, we can convert instanceID() to float I think.

[davepagurek]

While those are true, I think it might be worth compromising on those more advanced features for the sake of ease of use.

Another thought: for array indexing, we could make our array[n] operator in JavaScript convert to array[int(n)] when converting to GLSL whenever we add support for arrays. We might want to shift the mental model from being "operators behave differently on different numeric types" to something like "there are separate operators for int and float." So we could maybe treat numbers all as floats, so a * b always does float multiplication, but add a JavaScript method like a.intDivide(b) that translates to int(a) * int(b) in GLSL.

[davepagurek]

also cc @lukeplowden in case you have thoughts!

[perminder-17]

Another thought: for array indexing, we could make our array[n] operator in JavaScript convert to array[int(n)] when converting to GLSL whenever we add support for arrays. We might want to shift the mental model from being "operators behave differently on different numeric types" to something like "there are separate operators for int and float." So we could maybe treat numbers all as floats, so a * b always does float multiplication, but add a JavaScript method like a.intDivide(b) that translates to int(a) * int(b) in GLSL.

Aah, yes. That's sounds great. I agree that prioritizing ease of use over more complex features is sensible. This approach makes a lot of sense and we could do that after making instanceID() to float.

[lukeplowden]

Firstly, this is actually a bug coming from here:
FunctionCallNode

This is the error

Expected argument types: (genType, genType)
Provided argument types: (int, float)

It infers genType to be an int, when really those should only be float or vec2/3/4 types. The error thrown does stop the GLSL code from generating which would be invalid code, but it is really a bug in the p5.strands compiler. As a separate issue, we should definitely make function calls convert from int to float type.

On the actual topic of this issue, this does raise some other issues:

p5.strands Expression	Converted GLSL Expression
0.1 * instanceID()	0.1000 * float(gl_InstanceID)
instanceID() * 0.1	float(gl_InstanceID * 0)
instanceID() / 10	float((gl_InstanceID / 10))

0.1 * instanceID() works how you would expect want it to, because it wraps 0.1 into dynamicNode(0.1) and that function always makes floats from single values. I think the left-hand operand takes precedent for determining type, which is a bit strange for sure.

I do basically agree that everything could be converted to float, and I like the mental model of "there are separate operators for int and float". For the most part, we do always use floats at least in vertex and fragment shaders. I think though, for compute (which may be added in the future) there are generally more integer operations. At least for now, I think it's better to presume that all binary operations using * / + - in JavaScript, are on float/vec types

[davepagurek]

Does it go through FunctionCallNode for multiplication/division, or is that all through BinaryExpressionNode's determineType?

Initially I was thinking the fix might be in here:

p5.js/src/webgl/ShaderGenerator.js

Lines 1385 to 1387 in 2606c21

	fn.instanceID = function() {
	return variableConstructor('gl_InstanceID', 'int');
	}

Possibly returning something like new FloatNode(variableConstructor('gl_InstanceID', 'int')).

[lukeplowden]

That would also fix it, but genType should also never be an int, and the actual error comes from calls to pow and floor receiving (and accepting) an IntNode as arguments, which isn't a valid signature. If I change the FunctionCallNode:

let userType = getType(userArg);
...
if (expectedArgType === 'genType') {
  if (userType === 'int') userType = 'float';
...

Then it does properly cast the type to float, and handle the error. It does leave the weirdness from the table above, where different syntaxes for the same operation currently give different results. It will just render unexpectedly, instead of not compiling as GLSL or throwing the type error about function signatures.

BinaryExpressionNode should also be changed to stop the confusion of the different outputs based on what's on the left or right hand side of the operation.

There are a few different options as you've highlighted, about whether to just assume all scalars are floats (which can already be cast to vecs easily). That way we can cast back to int if needed.

Otherwise, I suppose the compiler would have to be a bit smarter about knowing what type something should be based on what it's used in... maybe nodes could be duplicated if used in different contexts? This is probably unnecessary though...

Your change would definitely work for instanceID() in this case though, and give a consistent output. If we make all scalars floats, this would be necessary.

[davepagurek]

got it, thanks for the explanation! That works too.

I'm going to mark this one as "help wanted" in case any contributors want to try tackling this based on this discussion.