Draw: Custom IDs #105261

Miguel Pozo · 2023-02-27T18:50:14+01:00

Miguel Pozo commented

2023-02-27 18:50:14 +01:00

(Opening a new pull request because Gitea refuses to reopen the previous one (#105034))

This pull request adds a new tipe of resource handles (thin handles).
These are intended for cases where a resource buffer with more than one entry for each object is needed (for example, one entry per material slot).
While it's already possible to have multiple regular handles for the same object, they have a non-trivial overhead in terms of uploaded data (matrix, bounds, object info) and computation (visibility culling).
Thin handles store an indirection buffer pointing to their "parent" regular handle, therefore multiple thin handles can share the same per-object data and visibility culling computation.

Thin handles can only be used in their own Pass type (PassMainThin), so passes that don't need them don't have to pay the overhead.

This pull request also includes the update of the Workbench Next pre-pass to use PassMainThin, which is the main reason for the implementation of this feature.

The main change from the previous PR is that the thin handles are now stored directly in the main resource_id_buf, to avoid wasting an extra bind slot.

(Opening a new pull request because Gitea refuses to reopen the previous one (#105034)) This pull request adds a new tipe of resource handles (thin handles). These are intended for cases where a resource buffer with more than one entry for each object is needed (for example, one entry per material slot). While it's already possible to have multiple regular handles for the same object, they have a non-trivial overhead in terms of uploaded data (matrix, bounds, object info) and computation (visibility culling). Thin handles store an indirection buffer pointing to their "parent" regular handle, therefore multiple thin handles can share the same per-object data and visibility culling computation. Thin handles can only be used in their own Pass type (PassMainThin), so passes that don't need them don't have to pay the overhead. This pull request also includes the update of the Workbench Next pre-pass to use PassMainThin, which is the main reason for the implementation of this feature. The main change from the previous PR is that the thin handles are now stored directly in the main resource_id_buf, to avoid wasting an extra bind slot.

Miguel Pozo added 7 commits 2023-02-27 18:50:31 +01:00

bf570dc333 Thin Handles

34a7ecd846 DrawMultiBufBase

aaf3578995 Draw Command generation

d946db9bf0 resource_id macro redefinition

8d6b7804b6 Use PassMainThin in Workbench Next

390200bb85 Merge thin handles into the main thin_id_buf

Avoid the need of an extra SSBO slot

cd8e6c214b Support draw_resource_id_with_thin_fallback in the OGL backend

Miguel Pozo added this to the EEVEE & Viewport project 2023-02-27 18:51:06 +01:00

Miguel Pozo requested review from Clément Foucault 2023-02-27 18:51:15 +01:00

Clément Foucault requested changes 2023-02-27 21:39:44 +01:00

Clément Foucault left a comment

I'm not convinced Thin is a good subtype prefix.

As one of my comment point out, DrawPrototype has some padding byte you should reuse for the "Thin" ID. This alone should remove the need for many duplicate types. I would like to avoid this for this particular purpose. So maybe a runtime boolean to enable this is preferable (doesn't add much overhead I think).

I'm also considering if this is better to just let the engine choose what data to pass to this ID and call it drw_CustomID. This way it could be used for selection IDs for instance without being specifically target at a "Thin" or subobject concept.

As for the thin handle, well, this would become obsolete, and you would just have an extra parameter for the draw() methods. This parameter is going to be sent with the DrawPrototype anyway. The only saving you get for not using it is the reduced size of the final resource_id_buf.

I'm not convinced `Thin` is a good subtype prefix. As one of my comment point out, `DrawPrototype` has some padding byte you should reuse for the "Thin" ID. This alone should remove the need for many duplicate types. I would like to avoid this for this particular purpose. So maybe a runtime boolean to enable this is preferable (doesn't add much overhead I think). I'm also considering if this is better to just let the engine choose what data to pass to this ID and call it `drw_CustomID`. This way it could be used for selection IDs for instance without being specifically target at a "Thin" or subobject concept. As for the thin handle, well, this would become obsolete, and you would just have an extra parameter for the `draw()` methods. This parameter is going to be sent with the `DrawPrototype` anyway. The only saving you get for not using it is the reduced size of the final `resource_id_buf`.

source/blender/draw/intern/shaders/draw_command_generate_comp.glsl

						
				@ -56,1 +56,4 @@

				  uint resource_index = (proto.resource_handle & 0x7FFFFFFFu);

				  uint visibility_index = resource_index;

				#ifdef WITH_THIN_HANDLES

				  visibility_index = thin_map_buf[resource_index] & 0x7FFFFFFFu;

Clément Foucault commented

2023-02-27 21:15:06 +01:00

Scrap this thin_map_buf. There is a uint _pad0 you can replace inside DrawPrototype.

Scrap this `thin_map_buf`. There is a `uint _pad0` you can replace inside `DrawPrototype`.

pragma37 marked this conversation as resolved

source/blender/draw/intern/shaders/draw_view_info.hh

						
				@ -206,0 +212,4 @@

				GPU_SHADER_CREATE_INFO(draw_resource_id_new_with_thin)

				    .define("UNIFORM_RESOURCE_ID_NEW")

				    .define("WITH_THIN_HANDLES")

				    /*TODO (Miguel Pozo): Should be uint2 ? */

Clément Foucault commented

2023-02-27 20:10:33 +01:00

It isn't uint2 because of compatibility with older code. This should become a TODO.

It isn't `uint2` because of compatibility with older code. This should become a TODO.

pragma37 marked this conversation as resolved

source/blender/gpu/opengl/gl_vertex_array.cc

						
				@ -121,12 +121,16 @@ void GLVertArray::update_bindings(const GLuint vao,

				  if (batch->resource_id_buf) {

				    const ShaderInput *input = interface->attr_get("drw_ResourceID");

				    int size = 1;

Clément Foucault commented

2023-02-27 20:08:15 +01:00

Use comp_len or component_len.
size is generally reserved for size in bytes use _len suffix for the rest.
Try to avoid calling variable just len or size. They are always size of length of something.

Use `comp_len` or `component_len`. `size` is generally reserved for size in bytes use `_len` suffix for the rest. Try to avoid calling variable just `len` or `size`. They are always size of length of something.

pragma37 marked this conversation as resolved

Miguel Pozo commented

2023-02-28 13:10:17 +01:00

Yes, the Thin prefix is just a leftover.
I used that term because that's the way we were initially refering to them.

The other patch used Sub, but IMO the drw_CustomID approach makes more sense.

The only saving you get for not using it is the reduced size of the final resource_id_buf.

So, do you mean that custom handles should always be added to resource_id_buf?

Yes, the `Thin` prefix is just a leftover. I used that term because that's the way we were initially refering to them. The other patch used `Sub`, but IMO the `drw_CustomID` approach makes more sense. > The only saving you get for not using it is the reduced size of the final `resource_id_buf`. So, do you mean that custom handles should always be added to `resource_id_buf`?

Miguel Pozo force-pushed pull-thin-handles from bea7e1e18a to 242fc6ff92

2023-02-28 16:33:03 +01:00

Compare

Clément Foucault reviewed 2023-02-28 16:45:35 +01:00

source/blender/draw/engines/workbench/workbench_engine.cc

						
				@ -210,1 +209,3 @@

				            Material &mat = resources.material_buf.get_or_resize(_handle.resource_index());

				            uint material_buf_index = material_buf_len++;

				            Material &mat = resources.material_buf.get_or_resize(material_buf_index);

Clément Foucault commented

2023-02-28 16:41:07 +01:00

I think we should add append_and_get_index to the StorageVectorBuffer class. This would remove the need for the material_buf_index.

I think we should add `append_and_get_index` to the `StorageVectorBuffer` class. This would remove the need for the `material_buf_index`.

Miguel Pozo commented

2023-02-28 17:51:40 +01:00

It can be retrieved with .size().

This is what I tried at first, but it didn't work correctly with get_or_resize(), which I think it's the real issue.

Maybe in .get_or_resize() we should do items_len_ = max(items_len_, index)?

It can be retrieved with `.size()`. This is what I tried at first, but it didn't work correctly with `get_or_resize()`, which I think it's the real issue. Maybe in `.get_or_resize()` we should do `items_len_ = max(items_len_, index)`?

source/blender/draw/intern/shaders/draw_view_info.hh

						
				@ -210,6 +219,13 @@ GPU_SHADER_CREATE_INFO(draw_resource_id_fallback)

				    .define("UNIFORM_RESOURCE_ID_NEW")

				    .vertex_in(15, Type::INT, "drw_ResourceID");

				GPU_SHADER_CREATE_INFO(draw_resource_and_custom_id_fallback)

Clément Foucault commented

2023-02-28 16:44:30 +01:00

Rename draw_resource_and_custom_id to draw_resource_with_custom_id.

Rename `draw_resource_and_custom_id` to `draw_resource_with_custom_id`.

pragma37 marked this conversation as resolved

source/blender/draw/intern/shaders/draw_view_info.hh

						
				@ -220,3 +236,3 @@

				 * \{ */

				GPU_SHADER_CREATE_INFO(draw_modelmat_new)

				GPU_SHADER_CREATE_INFO(draw_modelmat_new_common_)

Clément Foucault commented

2023-02-28 16:44:56 +01:00

Remove the _ suffix here.

Remove the `_` suffix here.

pragma37 marked this conversation as resolved

Miguel Pozo force-pushed pull-thin-handles from 242fc6ff92 to 68d46fb6f4

2023-02-28 16:55:11 +01:00

Compare

Miguel Pozo force-pushed pull-thin-handles from 68d46fb6f4 to 462da1c87c

2023-02-28 17:16:40 +01:00

Compare

Miguel Pozo added 1 commit 2023-02-28 17:37:39 +01:00

e39bbec7c2 Use StorageVectorBuffer size()

Miguel Pozo added 1 commit 2023-02-28 17:46:04 +01:00

b5f1ef22af Cleanup

Miguel Pozo changed title from ~~WIP: Draw: Thin Handles~~ to Draw: Custom IDs

2023-02-28 18:09:39 +01:00

Miguel Pozo requested review from Clément Foucault 2023-02-28 18:09:57 +01:00

Clément Foucault approved these changes 2023-03-01 21:36:45 +01:00

Clément Foucault added 1 commit 2023-03-01 21:37:28 +01:00

14dfd53017 Merge branch 'main' into pull-thin-handles

Clément Foucault merged commit 59b9bb0849 into main

2023-03-01 21:42:37 +01:00

Clément Foucault referenced this issue from a commit

2023-03-01 21:42:37 +01:00

Draw: Custom IDs

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Draw: Custom IDs #105261