Fix: Draw: EEVEE-Next shadow flickering and other uninitialized bounds fixes #120591

As discussed on blender chat, I would rather see this fixed at the consumer shaders level than trying to not invoke the consumer shaders. We should have some utility functions to check if the bounds are corrects (bounding sphere and bounding box).

Pros:

Defines a clear way to interact with bounds.
Future proof if we modify object bounds on GPU.

Cons:

More error prone in the process of changing existing shaders.

As discussed on blender chat, I would rather see this fixed at the consumer shaders level than trying to not invoke the consumer shaders. We should have some utility functions to check if the bounds are corrects (bounding sphere and bounding box). Pros: - Defines a clear way to interact with bounds. - Future proof if we modify object bounds on GPU. Cons: - More error prone in the process of changing existing shaders.

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

						
				@ -904,2 +904,4 @@

				  }

				  const ObjectBounds &bounds =

				      inst_.manager->bounds_buf.current()[resource_handle.resource_index()];

Clément Foucault commented

2024-04-13 21:33:29 +02:00

Reading the data of the draw manager is prohibited by design. The fact these members are public are only for them to be used as shader resources.

Miguel Pozo added 2 commits 2024-04-15 14:04:20 +02:00

9d1dbec0a4 Merge branch 'main' into pull-draw-fix-uninitialized-bounds

9f3312172b Feedback

Clément Foucault requested changes 2024-04-15 23:32:59 +02:00

Dismissed

Clément Foucault left a comment

There is still two occurrences of if (bounds.bounding_sphere.w != -1.0) { inside draw/intern/shaders

There is still two occurrences of `if (bounds.bounding_sphere.w != -1.0) {` inside `draw/intern/shaders`

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

						
				@ -205,1 +205,4 @@

				inline bool drw_bounds_are_valid(ObjectBounds bounds)

				{

				  return bounds.bounding_sphere.w != -1.0f;

Clément Foucault commented

2024-04-15 23:31:10 +02:00

bounds.bounding_sphere.w == -1.0f only means the culling is disabled (which does not always means the bounding box is invalid (see snippet).

    if (bounds.bounding_sphere.w > 1e12) {
      bounds.bounding_sphere.w = -1.0;
    }

Maybe set it to -2 when bounding box is invalid.

`bounds.bounding_sphere.w == -1.0f` only means the culling is disabled (which does not always means the bounding box is invalid (see snippet). ``` if (bounds.bounding_sphere.w > 1e12) { bounds.bounding_sphere.w = -1.0; } ``` Maybe set it to `-2` when bounding box is invalid.

Miguel Pozo commented

2024-04-16 16:11:04 +02:00

bounds.bounding_sphere.w == -1.0f only means the culling is disabled (which does not always means the bounding box is invalid

At the moment disabling culling effectively means the bounding box is invalid, since the only way to disable culling is to create a ResourceHandle without bounds.

Right now culling is only explicitly disabled for objects with invalid bounding boxes (curves and hair), and for the lookdev spheres.

Maybe we need a better design for this.

About the diagonal > 1e12 case, I think is fair to treat those bounds as invalid. That's going to wreak havoc on any system that relies on object bounds.

> `bounds.bounding_sphere.w == -1.0f` only means the culling is disabled (which does not always means the bounding box is invalid At the moment disabling culling effectively means the bounding box is invalid, since the only way to disable culling is to create a ResourceHandle without bounds. Right now culling is only explicitly disabled for objects with invalid bounding boxes (curves and hair), and for the lookdev spheres. Maybe we need a better design for this. About the diagonal > 1e12 case, I think is fair to treat those bounds as invalid. That's going to wreak havoc on any system that relies on object bounds.

👍 1

fclem marked this conversation as resolved

Miguel Pozo added 1 commit 2024-04-16 16:12:58 +02:00

eae0286c64 Remove all bounds.bounding_sphere.w != -1.0 checks

Clément Foucault approved these changes 2024-04-16 17:17:08 +02:00

Miguel Pozo merged commit cec57e9138 into main

2024-04-16 17:19:29 +02:00

Miguel Pozo referenced this issue from a commit

2024-04-16 17:19:29 +02:00

Fix: Draw: EEVEE-Next shadow flickering and other uninitialized bounds fixes

Miguel Pozo deleted branch pull-draw-fix-uninitialized-bounds

2024-04-16 17:19:32 +02:00