I get a compiler error: P2532.

Worked well for the most part so far.

I get a crash when try to use an integer attribute: P2533.

Just type the name of the attribute in the Attribute node.

The compiler error is fixed btw.

Solid work, I have little to add about the code itself.

As for the result I'm not sure what to expect. Some of the objects have NaN artifacts but this seems related to be blend modes used by the Mix RGB nodes.

To me the setup cost of having 16 separated VBO is negligible, since using all of them on an animated object is quite unlikely.

Yes this is the expected result. The red color is because most attributes are scalar values converted to vec4(v, 0, 0, 1).

Just in case, here is my startup file with a prettier output:

Yes this is the expected result. The red color is because most attributes are scalar values converted to vec4(v, 0, 0, 1).

I don't think this is the expected behavior. Everywhere we convert scalar values to vectors with vec3(v, v, v) (e.g. when you connect a float socket to a vector socket in shading, compositing or geometry nodes).

In D12969#341509, @Jacques Lucke (JacquesLucke) wrote:

Yes this is the expected result. The red color is because most attributes are scalar values converted to vec4(v, 0, 0, 1).

I don't think this is the expected behavior. Everywhere we convert scalar values to vectors with vec3(v, v, v) (e.g. when you connect a float socket to a vector socket in shading, compositing or geometry nodes).

In the shaders, all attributes are declared as vec4 (this is out of scope of this patch). If we do not give a vec4 type, it will be implicitly converted (or padded actually) to vec4 by OpenGL, and will give vec4(v, 0, 0, 1) for scalars

If you were to plug the float output socket of the attribute node to a vec3 socket, only the red channel will be read, and then converted to vec3(v, v, v) as we usually do. In the file that Clément used, which I posted in the task, the color socket of the attribute node is used, so the padded vec4(v, 0, 0, 1) is read directly with no conversions.

Note that this is already happening with UVs, they are implicitly converted to vec4(u, v, 0, 1) and this is what is read out of the color socket.

With UVs that behavior is expected, but not with scalar values. The cube really should be white in the example below, not red. Connecting Fac or Color to the output should yield the same result, because the same implicit conversion should be used internally. When converting scalars to colors in geometry nodes we use vec4(v, v, v, 1).
The fact that OpenGL gives you vec4(v, 0, 0, 1) is an implementation detail and should not impact our decision here imo.

The only real way of controlling how attributes are converted to vec4 is to do it ourselves. However, attribute type information is not available by the time shaders are compiled to GLSL (i.e. in the Attribute node GPU code generator). So we would need to pass this information at render time, but are limited by the amount of uniforms we can have overall. This would then be more of a technical limitation, than an "implementation detail". If there is an easy solution to this problem that I can look into today, which does not sacrifice artistic control too much, I'll try to do it. Otherwise, I'd consider this a do-do, and we should know, and document, that if we have a scalar attribute, and we want the scalar replicated in all dimensions of the output vector, the scalar output should be used.

The only way (that I can think of) to make scalar expanded as vec3, without having the overhead of using a bigger buffer, is to use buffer texture for these attributes and use texture state swizzle to map the attribute correctly. But this would require much more deeper code changes. This is how volume attributes do it so this might be useful to take a look at them.

Using individual uniforms for this (or even a 16bit bitmap) might also work.

I don't really like having to pass custom uniforms or samplers for drawing geometry, but if we have to do it, I would wait for EEVEE rewrite first as it is much cleaner to do it in the new code. Or you can try to implement that in the branch eevee-rewrite. ;)

No idea if this can work, but maybe for the color and vector output we can read the value as float and as a vec4 and by comparing them in some way figure out the texture data type. I don't know the implicit conversions opengl does here, so it may not work. If it does work, I think we should use this hack instead of having inconsistent implicit conversions in common cases.
If this does not work, I'm fine with merging the patch as is, but we should try think of ways to warn the user about this behavior.. Definitely create a follow up task for it.

Your idea is going to fail if you have a redish color somewhere, or a float3 with a only the X-axis set. The conversion as I already mentioned before is to pad the values with vec4(0, 0, 0, 1), so a scalar or a vec3 with only the X-axis being non-zero will already become vec4(v, 0, 0, 1), and we would have no idea if this was a scalar, a vec3, or a color to begin with.

For now I'll edit the commit message to tell about this, and make sure the release notes are clear about this. I can also create a to-do task for it. As for warning the users, I guess we could also make a patch for the UI team to review to add a warning in the node or somewhere (I for now like when the UI tells me something). I remember there used to be warning on the SSS node in Cycles back when it was an experimental feature, so that should be doable.

A possibly related issue I just ran into. Shouldn't the brightness of the cube be the same when Fac vs. Value is connected to Surface?
Currently, when Fac is connected, the cube has a brightness of 1/3 in a render, while when Value is connected, it will be 1. That seems highly unintuitive and is inconsistent with cycles.

Good catch, the code for the float output socket is indeed outf = avg(attr.xyz);. Since this is for Blender 3.0, we could break compatibility with older files and let outf = attr.x for the time being.

Actually now that I think about it, "Fac" is a generic factor value, we could add a "Value" socket for scalars that will only select the first component of the vec4, thus not breaking compatibility.

I think having yet another output to circumvent an implementation issue is not great for the UX. If it is half working I would either delay it until it is fully working or ship it under "experimental".

I thought this patch was for 3.1 and not 3.0.

It can go in 3.0 if it is ready on time (as noted in the rendering meeting).

I think the fact that these attributes can't be rendered right now is almost a bug, I would rather have this in 3.0 even if it's not perfect.

The simplest solution to me seems to be to store floats as float3 always, and optimize the memory usage in 3.1.

Seems to work as expected for me now :)

I implemented that macro madness in order to safely free a batch.
Committed a fix for the build issue. rB3e3ff1a464b9
(It would be nice if that part of the code was simplified).