Geometry Nodes: Support rendering generated attributes. #85075

New Issue

Jacques Lucke · 2021-01-26T12:21:19+01:00

Jacques Lucke commented

2021-01-26 12:21:19 +01:00

This can be used as a parent task for e.g. tasks specific to Cycles and Eevee.
I started looking into this a bit.

The Cycles implementation seems to be relative straight forward, I'll try that.

The Eevee implementation seems more tricky, because DRW_MeshCDMask is not general enough.

This can be used as a parent task for e.g. tasks specific to Cycles and Eevee. I started looking into this a bit. The Cycles implementation seems to be relative straight forward, I'll try that. The Eevee implementation seems more tricky, because `DRW_MeshCDMask` is not general enough.

Jacques Lucke commented

2021-01-26 12:21:19 +01:00

Added subscriber: @JacquesLucke

Jacques Lucke commented

2021-01-26 12:34:49 +01:00

Added subscribers: @brecht, @fclem

Victor-Louis De Gusseme commented

2021-01-26 13:57:47 +01:00

Added subscriber: @victorlouis

Miro Horváth commented

2021-02-08 15:25:50 +01:00

Added subscriber: @MiroHorvath

Sergey commented

2021-02-09 06:41:43 +01:00

Added subscriber: @randum

Simon Thommes commented

2021-02-11 12:26:10 +01:00

Added subscriber: @SimonThommes

blender-admin commented

2021-02-17 12:07:59 +01:00

This issue was referenced by blender/cycles@a484a197ea

This issue was referenced by blender/cycles@a484a197eadf8373382bef1a7b80ceef83ba0dc8

blender-admin commented

2021-02-17 12:07:59 +01:00

This issue was referenced by 3a6d6299d7

This issue was referenced by 3a6d6299d7a0f2a6a156bce6731a0b29804d911d

hyyou commented

2021-03-18 05:17:57 +01:00

Added subscriber: @hyyou

Eric commented

2021-07-13 09:31:41 +02:00

Added subscriber: @EricM

Ray commented

2021-09-02 17:29:30 +02:00

Added subscriber: @VDC

Dalai Felinto commented

2021-10-13 12:58:01 +02:00

Added subscriber: @dfelinto

Kévin Dietrich self-assigned this 2021-10-13 13:09:52 +02:00

Clément Foucault commented

2021-10-13 16:06:35 +02:00

After looking at the issue I can say that DRW_MeshCDMask will not be enough. To me we have to treat the generic attribs as a special case. For this to work, we need a list of index (uint? ushorts? uchar?) or a bitmap that identifies the needed generic attributes inside the mesh. The issue is that modifying this needs to be threadsafe. Having a per Mesh mutex might be ok for now.

Note that the threadsafety is needed for future per object multithreading but it is not actually done at the moment. So you will not be able to test it.

mesh_cd_calc_used_gpu_layers is the function that matches the needed attribute name requested by the shader to a generic attribute.

After looking at the issue I can say that `DRW_MeshCDMask` will not be enough. To me we have to treat the generic attribs as a special case. For this to work, we need a list of index (uint? ushorts? uchar?) or a bitmap that identifies the needed generic attributes inside the mesh. The issue is that modifying this needs to be threadsafe. Having a per Mesh mutex might be ok for now. Note that the threadsafety is needed for future per object multithreading but it is not actually done at the moment. So you will not be able to test it. `mesh_cd_calc_used_gpu_layers` is the function that matches the needed attribute name requested by the shader to a generic attribute.

Kévin Dietrich commented

2021-10-13 16:32:54 +02:00

Actually I already started implementing this (I added a few extra members to DRW_MeshCDMask as a start, but that will change). Attributes can be on different domains (or in terms of CustomData: cd_vdata, cd_ldata, cd_pdata, etc.). So we need to keep track of the index, but also the domain. So that would mean a list or bitmap per domain. I'll try to figure something out.

Also I am currently using a different VBO for each attribute type which could allow to optimize memory a bit in the future, although that might be premature. All attributes are by default vec4 in the shaders (I think it is due to the color output of the attribute node, and to simplify things?), so that's what I have to use to build VBOs, which is a waste for float and float2.

Actually I already started implementing this (I added a few extra members to `DRW_MeshCDMask` as a start, but that will change). Attributes can be on different domains (or in terms of `CustomData`: `cd_vdata`, `cd_ldata`, `cd_pdata`, etc.). So we need to keep track of the index, but also the domain. So that would mean a list or bitmap per domain. I'll try to figure something out. Also I am currently using a different VBO for each attribute type which could allow to optimize memory a bit in the future, although that might be premature. All attributes are by default `vec4` in the shaders (I think it is due to the color output of the attribute node, and to simplify things?), so that's what I have to use to build VBOs, which is a waste for `float` and `float2`.

Clément Foucault commented

2021-10-13 17:22:50 +02:00

Attributes can be declared vec4 in the shader but only have float or vec2 input from the VBO. OpenGL will convert by adding the extra components using the vector (0,0,0,1).

There is also a limit on the number of VBO a GPU batch can contain. I don't remember the implication of increasing this maximum (GPU_BATCH_VBO_MAX_LEN), but it would be nice if we could just have one VBO per attrib and start less worrying about interlaced vertex layout or packing attribs together.

If you profile what I propose and if the overhead is reasonnable, I would be happy to set GPU_BATCH_VBO_MAX_LEN to 16.

Attributes can be declared `vec4` in the shader but only have `float` or `vec2` input from the VBO. OpenGL will convert by adding the extra components using the vector `(0,0,0,1)`. There is also a limit on the number of VBO a GPU batch can contain. I don't remember the implication of increasing this maximum (`GPU_BATCH_VBO_MAX_LEN`), but it would be nice if we could just have one VBO per attrib and start less worrying about interlaced vertex layout or packing attribs together. If you profile what I propose and if the overhead is reasonnable, I would be happy to set `GPU_BATCH_VBO_MAX_LEN` to 16.

Kévin Dietrich commented

2021-10-15 13:57:53 +02:00

How do handle color attributes though? They are using CD_PROP_COLOR which is also used by sculpt vertex colors, which is behind an experimental feature flag, but also merged with regular vertex colors in the vcol buffer. Although this is not a supported feature yet, Alembic (via D11591) and USD in the future may import data as CD_PROP_COLOR.

How do handle color attributes though? They are using `CD_PROP_COLOR` which is also used by sculpt vertex colors, which is behind an experimental feature flag, but also merged with regular vertex colors in the `vcol` buffer. Although this is not a supported feature yet, Alembic (via [D11591](https://archive.blender.org/developer/D11591)) and USD in the future may import data as `CD_PROP_COLOR`.

Brecht Van Lommel commented

2021-10-15 15:39:22 +02:00

For sculpt vertex colors, the reason they are behind a feature flag in the code is because the current implementation of that only works with either old or new vertex colors. D12587 has work towards making both work.

For the purpose of this I think they could be handled like any other attribute? And then later there could be some way to unify handling of vertex colors and general attributes, to make vertex colors just a special case of attributes.

For sculpt vertex colors, the reason they are behind a feature flag in the code is because the current implementation of that only works with either old or new vertex colors. [D12587](https://archive.blender.org/developer/D12587) has work towards making both work. For the purpose of this I think they could be handled like any other attribute? And then later there could be some way to unify handling of vertex colors and general attributes, to make vertex colors just a special case of attributes.

Jacques Lucke commented

2021-10-18 10:22:06 +02:00

Is the work happening on this in some branch?

Kévin Dietrich commented

2021-10-18 11:11:31 +02:00

Part of it was added in the tmp-abc-features branch that I pushed to make some experimental builds, rest is sill local, but will be either pushed there for new test builds, or sent to code review, or both.

Part of it was added in the `tmp-abc-features` branch that I pushed to make some experimental builds, rest is sill local, but will be either pushed there for new test builds, or sent to code review, or both.

Rahul Chaudhary commented

2021-10-18 14:11:58 +02:00

Added subscriber: @RC12

Hans Goudey commented

2021-10-18 17:12:02 +02:00

Added subscriber: @HooglyBoogly

Trung Phạm commented

2021-10-19 10:56:37 +02:00

Added subscriber: @zNight

Kévin Dietrich commented

2021-10-21 21:28:12 +02:00

I removed the sculpt vertex colors form DRW_MeshCDMask so they are handled with the other attributes.

I wrote a script to generate random attributes with random types on random domains to stress test the code. So far it is pretty stable, all domains and types seem to work.

However, the limit of 15 attributes (from GPU_MAX_ATTR) does not take into account other attributes like normals. So with the default cube with a Principled BSDF, I can only add 14 attributes, then shader compilation fails and it just renders purple. Apart from a message in the console, there is nothing much to help debugging for users. Can we improve this a bit, like adding a proper UI message? I don't know if shader compilation can easily take all attributes into consideration. That's orthogonal to the patch, so not important.

I ran two versions of the code in a profile build using the aforementioned script to generate 100 objects. One version of the code is for one VBO per attribute, the other is one packed VBO per data type (like uvs or vcols currently). The objects are randomly selected from the list of default mesh objects (cube, UV sphere, monkey, etc.), as the idea was to measure the overhead of having one VBO per attribute vs one packed VBO for each type, so the size of the data does not really matter. The shaders are also randomized a bit to avoid having a single shader/ShadingGroup. Essentially, the shaders are made of Attribute nodes mixed together using MixRGB nodes where the blend type is randomized (then the result is plugged in the Principled BSDF node diffuse input).

Having one VBO per attribute has some overhead indeed, here are some screenshots from HotSpot, with 100 objects, we can see that GLBatch::draw is getting chunky:

Packed VBOs on first run (with shader compilation, not shown):

Packed VBOs on second run (after going out of and back in rendered mode):

Separate VBOs on first run (with shader compilation, not shown):

Separate VBOs on second run (after going out of and back in rendered mode):

Here is the .blend file: random_geo_nodes_attributes.blend

I did not time it in a release build, so I do not have numbers, but by the looks of things, it seems that only initialization is much costlier. As it stands, I am using a compile time definition to switch between the two versions of the code, so depending on what we choose, I can simply remove the other.

I removed the sculpt vertex colors form `DRW_MeshCDMask` so they are handled with the other attributes. ------- I wrote a script to generate random attributes with random types on random domains to stress test the code. So far it is pretty stable, all domains and types seem to work. However, the limit of 15 attributes (from `GPU_MAX_ATTR`) does not take into account other attributes like normals. So with the default cube with a Principled BSDF, I can only add 14 attributes, then shader compilation fails and it just renders purple. Apart from a message in the console, there is nothing much to help debugging for users. Can we improve this a bit, like adding a proper UI message? I don't know if shader compilation can easily take all attributes into consideration. That's orthogonal to the patch, so not important. I ran two versions of the code in a profile build using the aforementioned script to generate 100 objects. One version of the code is for one VBO per attribute, the other is one packed VBO per data type (like uvs or vcols currently). The objects are randomly selected from the list of default mesh objects (cube, UV sphere, monkey, etc.), as the idea was to measure the overhead of having one VBO per attribute vs one packed VBO for each type, so the size of the data does not really matter. The shaders are also randomized a bit to avoid having a single shader/ShadingGroup. Essentially, the shaders are made of Attribute nodes mixed together using MixRGB nodes where the blend type is randomized (then the result is plugged in the Principled BSDF node diffuse input). Having one VBO per attribute has some overhead indeed, here are some screenshots from HotSpot, with 100 objects, we can see that `GLBatch::draw` is getting chunky: Packed VBOs on first run (with shader compilation, not shown): ![typed_vbos.png](https://archive.blender.org/developer/F11371407/typed_vbos.png) Packed VBOs on second run (after going out of and back in rendered mode): ![typed_vbos_after_compilation.png](https://archive.blender.org/developer/F11371419/typed_vbos_after_compilation.png) Separate VBOs on first run (with shader compilation, not shown): ![per_attrib_vbos.png](https://archive.blender.org/developer/F11371426/per_attrib_vbos.png) Separate VBOs on second run (after going out of and back in rendered mode): ![per_attrib_vbos_after_compilation.png](https://archive.blender.org/developer/F11371480/per_attrib_vbos_after_compilation.png) Here is the .blend file: [random_geo_nodes_attributes.blend](https://archive.blender.org/developer/F11371652/random_geo_nodes_attributes.blend) I did not time it in a release build, so I do not have numbers, but by the looks of things, it seems that only initialization is much costlier. As it stands, I am using a compile time definition to switch between the two versions of the code, so depending on what we choose, I can simply remove the other.

Dalai Felinto commented

2021-10-22 14:22:27 +02:00

@kevindietrich hi, if the alembic changes are not aimed at 3.0, can you separate the code early on and have in its own branch so we can try it? (or even if they are aimed at 3.0, it would be helpful to look at this issue/feature isolated from alembic changes).