EEVEE-Next: Add Inflate Bounds option #114200

Miguel Pozo · 2023-10-27T16:57:50+02:00

Miguel Pozo commented

2023-10-27 16:57:50 +02:00

Add an Inflate Bounds option to Material settings, so frustum culling can work correctly with vertex displacement.

Miguel Pozo added the

labels 2023-10-27 16:57:51 +02:00

Miguel Pozo added 2 commits 2023-10-27 16:58:04 +02:00

9177ac2601 Inflate Bounds

de2502b700 Move setting to material

Miguel Pozo requested review from Clément Foucault 2023-10-27 16:59:55 +02:00

Clément Foucault requested changes 2023-10-30 13:19:35 +01:00

Clément Foucault left a comment

I am a bit unsure if we want this to be on the material. I think it depends much on the geometry. Cycles has a per object clipping property and other game engine consider the bounds a per object property.

I think it is more tedious to set up per material as you could forget one is set too high.

Not to mention this adds a bit more complexity compared to per object.

I am a bit unsure if we want this to be on the material. I think it depends much on the geometry. Cycles has a per object clipping property and other game engine consider the bounds a per object property. I think it is more tedious to set up per material as you could forget one is set too high. Not to mention this adds a bit more complexity compared to per object.

scripts/startup/bl_ui/properties_material.py Outdated

						
				@ -299,7 +299,7 @@ class EEVEE_NEXT_MATERIAL_PT_settings_surface(MaterialButtonsPanel, Panel):

				        col.prop(mat, "use_backface_culling", text="Camera")

				        col.prop(mat, "use_backface_culling_shadow", text="Shadow")

				        # TODO(fclem): Displacement option

Clément Foucault commented

2023-10-30 13:12:27 +01:00

This is meant for the displacement type. Not the bounds.

pragma37 marked this conversation as resolved

Miguel Pozo commented

2023-10-30 15:39:47 +01:00

I am a bit unsure if we want this to be on the material. I think it depends much on the geometry.

The amount of displacement is always driven by the material itself (unless using an attribute node).
For the latter case, we can add the per-object property as well and just use the max value.

I think it is more tedious to set up per material as you could forget one is set too high.

Consider you have a material and change its displacement. Now you have to search all the objects that use it (across scenes and even blend files) for it to work properly.
There are also cases where it may not be even possible to set the property (GN instancing procedural geometry).

> I am a bit unsure if we want this to be on the material. I think it depends much on the geometry. The amount of displacement is always driven by the material itself (unless using an attribute node). For the latter case, we can add the per-object property as well and just use the max value. > I think it is more tedious to set up per material as you could forget one is set too high. Consider you have a material and change its displacement. Now you have to search all the objects that use it (across scenes and even blend files) for it to work properly. There are also cases where it may not be even possible to set the property (GN instancing procedural geometry).

Miguel Pozo force-pushed pull-extend-bbox from 104e54d1e3 to de2502b700

2023-10-30 18:53:27 +01:00

Compare

Clément Foucault commented

2023-10-30 19:59:05 +01:00

So had a lot of discussion with both artists and programmers. Here is some considerations:

In favor of per Object:

The object scale is what "can" influence the displacement after the material. Object attributes can also affect the final displacement.
This is only a setting when things fail and it is changing a per object behavior. So if an object has issues, you don't want to go through all materials to check which one is creating the issue.
If you have linked data, the object is the one you can easily override / localize.
Object granularity is what this is applied to.
Only one setting.

In favor of per material:

Discoverability. The setting is closer to the nodetree that produces the displacement.
Reusable. No need to set it per object.

This is kind of similar to the step size of cycles volume material where a two level control should be working.

Since it is quite unclear for me how the object propertie would influence the material one, I would just add the material one first in the end.

I think object space is fine for now. We can add a world space option if needed later.

So had a lot of discussion with both artists and programmers. Here is some considerations: #### In favor of per Object: - The object scale is what "can" influence the displacement after the material. Object attributes can also affect the final displacement. - This is only a setting when things fail and it is changing a per object behavior. So if an object has issues, you don't want to go through all materials to check which one is creating the issue. - If you have linked data, the object is the one you can easily override / localize. - Object granularity is what this is applied to. - Only one setting. #### In favor of per material: - Discoverability. The setting is closer to the nodetree that produces the displacement. - Reusable. No need to set it per object. This is kind of similar to the step size of cycles volume material where a two level control should be working. Since it is quite unclear for me how the object propertie would influence the material one, I would just add the material one first in the end. I think object space is fine for now. We can add a world space option if needed later.

Clément Foucault requested changes 2023-10-30 20:07:25 +01:00

source/blender/draw/engines/eevee_next/eevee_instance.cc Outdated

						
				@ -225,0 +227,4 @@

				    if (is_drawable_type) {

				      for (auto i : IndexRange(DRW_cache_object_material_count_get(ob))) {

				        if (::Material *material = BKE_object_material_get(ob, i + 1)) {

				          inflate_bounds = math::max(inflate_bounds, material->inflate_bounds);

Clément Foucault commented

2023-10-30 20:05:55 +01:00

This is a bit annoying because this can make material that have no displacement inflate the bounding box for no reason.

I'm wondering if we could amend the bounds after the drawcall loop instead of trying to compute it upfront. This way, we would know if there is displacement based on the GPU material flag and the material flag.

Moreover that would avoid one loop over materials that are scattered across memory.

This is a bit annoying because this can make material that have no displacement inflate the bounding box for no reason. I'm wondering if we could amend the bounds after the drawcall loop instead of trying to compute it upfront. This way, we would know if there is displacement based on the GPU material flag and the material flag. Moreover that would avoid one loop over materials that are scattered across memory.

pragma37 marked this conversation as resolved

source/blender/draw/intern/draw_resource.hh Outdated

						
				@ -160,3 +160,3 @@

				}

				inline void ObjectBounds::sync(Object &ob)

				inline void ObjectBounds::sync(Object &ob, float inflate_bounds /* = 0.0f*/)

Clément Foucault commented

2023-10-30 20:01:09 +01:00

I don't think writing the default parameter is part of the code style.

pragma37 marked this conversation as resolved

Miguel Pozo added 5 commits 2023-11-02 17:04:48 +01:00

95d33150d5 Merge branch 'main' into pull-extend-bbox

226d1958fa Remove defaults

28aa4243b7 Add Manager::update_handle_bounds

f597a7cdc6 Add GPU_material_has_displacement_output

9eb062c15a Inflate bounds after material sync

Clément Foucault requested changes 2023-11-02 18:28:15 +01:00

source/blender/draw/engines/eevee_next/eevee_sync.cc Outdated

						
				@ -260,0 +269,4 @@

				  BKE_pbvh_bounding_box(ob_ref.object->sculpt->pbvh, min, max);

				  float3 center = (min + max) * 0.5;

				  float3 half_extent = max - center;

				  half_extent += float3(inflate_bounds);

Clément Foucault commented

2023-11-02 18:27:10 +01:00

No need to cast to float3. VecBase &operator+=(const T &b) is defined.

No need to cast to `float3`. `VecBase &operator+=(const T &b)` is defined.

pragma37 marked this conversation as resolved

source/blender/makesrna/intern/rna_material.cc Outdated

						
				@ -955,6 +955,15 @@ void RNA_def_material(BlenderRNA *brna)

				      "Determines which inner part of the mesh will produce volumetric effect");

				  RNA_def_property_update(prop, 0, "rna_Material_draw_update");

				  prop = RNA_def_property(srna, "inflate_bounds", PROP_FLOAT, PROP_NONE);

Clément Foucault commented

2023-11-02 18:22:43 +01:00

Since this is related to displacement, I would name it accordingly. Maybe call it Max Vertex Displacement Distance ("max_vertex_displacement") but I wouldn't clamp the displacement (at least not yet and definitively not in this commit).

Also this should be a PROP_DISTANCE.

Since this is related to displacement, I would name it accordingly. Maybe call it `Max Vertex Displacement Distance` (`"max_vertex_displacement"`) but I wouldn't clamp the displacement (at least not yet and definitively not in this commit). Also this should be a `PROP_DISTANCE`.

Miguel Pozo commented

2023-11-06 15:57:51 +01:00

Done, but I've renamed the UI text to just "Max Displacement", so it can fit in the default panel width.

source/blender/makesrna/intern/rna_material.cc Outdated

						
				@ -957,1 +957,4 @@

				  prop = RNA_def_property(srna, "inflate_bounds", PROP_FLOAT, PROP_NONE);

				  RNA_def_property_float_sdna(prop, nullptr, "inflate_bounds");

				  RNA_def_property_range(prop, 0.0f, 1000.0f);

Clément Foucault commented

2023-11-02 18:21:02 +01:00

Max should be FLT_MAX

pragma37 marked this conversation as resolved

Miguel Pozo added 1 commit 2023-11-03 15:37:26 +01:00

af85e3cc95 Address feedback

Miguel Pozo added 2 commits 2023-11-06 15:56:15 +01:00

d0ec48c156 Rename Python inflate_bounds to max_displacement

3a194f133f Merge branch 'main' into pull-extend-bbox

Miguel Pozo added 1 commit 2023-11-06 16:09:08 +01:00

e2250203a0 Merge branch 'main' into pull-extend-bbox

Clément Foucault approved these changes 2023-11-06 20:52:32 +01:00

source/blender/makesrna/intern/rna_material.cc Outdated

						
				@ -967,0 +969,4 @@

				  RNA_def_property_range(prop, 0.0f, FLT_MAX);

				  RNA_def_property_ui_text(prop,

				                           "Max Vertex Displacement",

				                           "Extends the bounds used for frustum culling. "