Workbench Next: Hair and Curves support #109456

Miguel Pozo · 2023-06-28T17:38:45+02:00

Miguel Pozo commented

2023-06-28 17:38:45 +02:00

Add Hair and Curves support to the new Draw Manager. The sub_pass_setup functions bind the required data to an already created sub-pass and return the batches needed for drawing.
Add Hair and Curves rendering to Workbench Next:
- Refactor MeshPasses so it's possible to retrieve the pass instead of letting the class handle the drawing itself.
- Refactor object_sync so the emitter handle can be passed to the hair_sync function (needed to avoid outlines between hair and emitter).
- Refactor the ObjectState constructor so it's easier to add support for other object types.
- Update get_material so the slot index starts from 1, to match BKE_object_material_get_eval.

Notes:

Curves don't support the Texture and Attribute modes. This matches the current Workbench behavior, but I'm not sure if that's the right thing to do.
Maybe MeshPasses should be renamed to ObjectPasses?

* Add Hair and Curves support to the new Draw Manager. The `sub_pass_setup` functions bind the required data to an already created sub-pass and return the batches needed for drawing. * Add Hair and Curves rendering to Workbench Next: * Refactor MeshPasses so it's possible to retrieve the pass instead of letting the class handle the drawing itself. * Refactor object_sync so the emitter handle can be passed to the hair_sync function (needed to avoid outlines between hair and emitter). * Refactor the ObjectState constructor so it's easier to add support for other object types. * Update get_material so the slot index starts from 1, to match BKE_object_material_get_eval. Notes: * Curves don't support the Texture and Attribute modes. This matches the current Workbench behavior, but I'm not sure if that's the right thing to do. * Maybe MeshPasses should be renamed to ObjectPasses?

Miguel Pozo added 10 commits 2023-06-28 17:38:59 +02:00

38f9fc7bc9 Add curves_sub_pass_setup

8015f4d3e6 Curves rendering

bcabf41938 Hair Rendering

c7b3b5b16a Merge branch 'main' into pull-workbench-next-curves

800a727a50 Fix overlapping samplers

5c5a02418b Merge branch 'main' into pull-workbench-next-curves

bcc86a62fb Fix hair outline

93e1f9a6bf Texture and Attribute fallback for Curves

43d75aa561 Fix sampler slot overlaps and clean-up

bf2482a1da Fix emitter_object_id

Miguel Pozo added this to the EEVEE & Viewport project 2023-06-28 17:39:56 +02:00

Miguel Pozo requested review from Clément Foucault 2023-06-28 17:40:08 +02:00

Miguel Pozo added the

Module

EEVEE & Viewport

label 2023-06-28 17:40:24 +02:00

Miguel Pozo added 1 commit 2023-06-28 18:00:17 +02:00

68740e067f Cleanup: Remove unused parameter

Clément Foucault requested changes 2023-06-29 17:21:05 +02:00

Clément Foucault left a comment

Looks good and quite safe. So will accept once my comments are addressed.

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

						
				@ -135,3 +136,3 @@

				    }

				    if (ob->type == OB_MESH && ob->modifiers.first != nullptr) {

				    bool render_ob_data = DRW_object_visibility_in_active_context(ob) & OB_VISIBLE_SELF;

Clément Foucault commented

2023-06-29 16:49:58 +02:00

Prefer using is_ and do_ prefix for boolean to describe what state they describe. In this case it is the object visibility so is_object_data_visible.

Prefer using `is_` and `do_` prefix for boolean to describe what state they describe. In this case it is the object visibility so `is_object_data_visible`.

pragma37 marked this conversation as resolved

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

						
				@ -138,0 +138,4 @@

				    bool render_ob_data = DRW_object_visibility_in_active_context(ob) & OB_VISIBLE_SELF;

				    render_ob_data = render_ob_data && (ob->dt >= OB_SOLID || DRW_state_is_scene_render());

				    if (!(ob->base_flag & BASE_FROM_DUPLI)) {

Clément Foucault commented

2023-06-29 16:41:03 +02:00

Split this block to a separate function called Instance::object_visibility_get.

Split this block to a separate function called `Instance::object_visibility_get`.

Miguel Pozo commented

2023-06-29 18:22:28 +02:00

volume_ps.object_sync_modifier is called from this block of code.
It would be pretty bad to hide that inside a seemingly unrelated helper function, IMO.

`volume_ps.object_sync_modifier` is called from this block of code. It would be pretty bad to hide that inside a seemingly unrelated helper function, IMO.

Clément Foucault commented

2023-06-29 19:10:03 +02:00

Oops I missed that. Forget my comment then.

fclem marked this conversation as resolved

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

						
				@ -370,0 +383,4 @@

				    PassMain::Sub &pass = get_mesh_pass(ob_ref, mat.is_transparent())

				                              .get_subpass(eGeometryType::CURVES, image, sampler_state, iuser)

				                              .sub(ob_ref.object->id.name);

Clément Foucault commented

2023-06-29 17:02:08 +02:00

This is pretty volatile! ob_ref.object could be the same pointer for many duplis instances. It could also be allocated on the stack for the duration of the sync process. It is not advisable to reference any of the data inside an object.

This is pretty volatile! `ob_ref.object` could be the same pointer for many duplis instances. It could also be allocated on the stack for the duration of the sync process. It is not advisable to reference any of the data inside an object.

pragma37 marked this conversation as resolved

source/blender/draw/engines/workbench/workbench_mesh_passes.cc Outdated

						
				@ -107,0 +103,4 @@

				  return *passes_[int(geometry_type)][int(eShaderType::MATERIAL)];

				}

				void MeshPass::draw(ObjectRef &ref,

Clément Foucault commented

2023-06-29 16:58:10 +02:00

Remove this. It would make it easier to understand if there is less functions to a class that do almost the same thing.

pragma37 marked this conversation as resolved

source/blender/draw/intern/draw_curves.cc

						
				@ -540,0 +630,4 @@

				  sub_ps.bind_ubo("drw_curves", curves_infos);

				  sub_ps.push_constant("hairStrandsRes", &curves_cache->final[subdiv].strands_res, 1);

Clément Foucault commented

2023-06-29 17:12:30 +02:00

I would be tempted to ask for bundling these into one UBO for each Curve object, but that's better left to another PR.

Miguel Pozo commented

2023-06-29 18:25:49 +02:00

Yep, there was a TODO on the original draw_hair shader info too.
However, as long as we need to bind a separate hairPointBuffer for each call, I don't think is going to make a huge difference?

Yep, there was a TODO on the original `draw_hair` shader info too. However, as long as we need to bind a separate `hairPointBuffer` for each call, I don't think is going to make a huge difference?

Clément Foucault commented

2023-06-29 19:11:49 +02:00

It could make a difference for some backends depending on how we update that. Also it would clean the global namespace.

fclem marked this conversation as resolved

Miguel Pozo added 2 commits 2023-06-29 18:18:06 +02:00

d066bd8c97 Cleanup

d2ca7a8fd0 Remove id.name references

Clément Foucault approved these changes 2023-06-29 19:12:27 +02:00

Miguel Pozo added 3 commits 2023-06-30 19:03:53 +02:00

5235f7bfeb Merge branch 'main' into pull-workbench-next-curves

f9728f321d Merge branch 'main' into pull-workbench-next-curves

69c2198d3c Fix merge