blender/blender
168
525
Fork 797
Code Issues 6.7k Pull Requests 947 Projects 21 Wiki Activity

Fix: Cycles BVH2 and Embree missing some transparent shadow bounces #125739

Merged
Weizhen Huang merged 5 commits from weizhen/blender:fix-bvh2-n-closest-hits into main 2024-08-06 15:38:06 +02:00
Conversation 8 Commits 5 Files Changed 2 +40 -35
2 changed files with 40 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

52
intern/cycles/kernel/bvh/shadow_all.h
View File

@ -62,6 +62,8 @@ ccl_device_inline
const float tmax = ray->tmax;
float tmax_hits = tmax;
uint isect_index = 0;
*r_num_recorded_hits = 0;
*r_throughput = 1.0f;
@ -250,38 +252,32 @@ ccl_device_inline
if (record_intersection) {
/* Test if we need to record this transparent intersection. */
const uint max_record_hits = min(max_hits, INTEGRATOR_SHADOW_ISECT_SIZE);
if (*r_num_recorded_hits < max_record_hits || isect.t < tmax_hits) {
/* If maximum number of hits was reached, replace the intersection with the
* highest distance. We want to find the N closest intersections. */
const uint num_recorded_hits = min(*r_num_recorded_hits, max_record_hits);
uint isect_index = num_recorded_hits;
if (num_recorded_hits + 1 >= max_record_hits) {
float max_t = INTEGRATOR_STATE_ARRAY(state, shadow_isect, 0, t);
uint max_recorded_hit = 0;
for (uint i = 1; i < num_recorded_hits; ++i) {
const float isect_t = INTEGRATOR_STATE_ARRAY(state, shadow_isect, i, t);
if (isect_t > max_t) {
max_recorded_hit = i;
max_t = isect_t;
}
}
if (num_recorded_hits >= max_record_hits) {
isect_index = max_recorded_hit;
}
/* Limit the ray distance and stop counting hits beyond this. */
tmax_hits = max(isect.t, max_t);
}
integrator_state_write_shadow_isect(state, &isect, isect_index);
}
/* Always increase the number of recorded hits, even beyond the maximum,
* so that we can detect this and trace another ray if needed. */
++(*r_num_recorded_hits);
weizhen marked this conversation as resolved Outdated
Sergey Sharybin commented 2024-08-05 17:05:47 +02:00
Outdated
Review
Copy Link

Can we avoid such not-so-obvious calculation and be more explicit:

uint next_isect_index = 0;

...

if (record_intersection) {
  integrator_state_write_shadow_isect(state, &isect, next_isect_index);
  /* Plus one to account for the new insertion. */
  if (*r_num_recorded_hits + 1 >= max_record_hits) {
    next_isect_index = /* index of the element with max distance */
  } else {
    ++next_isect_index;
  }
}
Can we avoid such not-so-obvious calculation and be more explicit: ```C uint next_isect_index = 0; ... if (record_intersection) { integrator_state_write_shadow_isect(state, &isect, next_isect_index); /* Plus one to account for the new insertion. */ if (*r_num_recorded_hits + 1 >= max_record_hits) { next_isect_index = /* index of the element with max distance */ } else { ++next_isect_index; } } ```
const uint max_record_hits = min(max_hits, INTEGRATOR_SHADOW_ISECT_SIZE);
if (*r_num_recorded_hits <= max_record_hits || isect.t < tmax_hits) {
integrator_state_write_shadow_isect(state, &isect, isect_index);
if (*r_num_recorded_hits >= max_record_hits) {
/* If the maximum number of hits is reached, find the furthest intersection to
replace it with the next closer one. We want N closest intersections. */
isect_index = 0;
tmax_hits = INTEGRATOR_STATE_ARRAY(state, shadow_isect, 0, t);
for (uint i = 1; i < max_record_hits; ++i) {
const float isect_t = INTEGRATOR_STATE_ARRAY(state, shadow_isect, i, t);
if (isect_t > tmax_hits) {
isect_index = i;
tmax_hits = isect_t;
}
}
}
else {
isect_index = *r_num_recorded_hits;
}
}
}
}
}

23
intern/cycles/kernel/device/cpu/bvh.h
View File

@ -393,24 +393,32 @@ ccl_device_forceinline void kernel_embree_filter_occluded_shadow_all_func_impl(
float max_t = INTEGRATOR_STATE_ARRAY(ctx->isect_s, shadow_isect, 0, t);
numhit_t max_recorded_hit = numhit_t(0);
float second_largest_t = 0.0f;
weizhen marked this conversation as resolved Outdated
Sergey Sharybin commented 2024-08-05 17:16:46 +02:00
Outdated
Review
Copy Link

Can as well be 0. Not sure if it is some implicit logic to somehow ignore multiple intersections at the distance of zero (which could happen if triangles are overlapping, or are intersection). Or it is some correction for denormal distance?

Or maybe is just a confusion with INT_MIN: the INT_MIN is some large negative value, while FLT_MIN is the minimal normalized positive floating point value.

Can as well be 0. Not sure if it is some implicit logic to somehow ignore multiple intersections at the distance of zero (which could happen if triangles are overlapping, or are intersection). Or it is some correction for denormal distance? Or maybe is just a confusion with `INT_MIN`: the `INT_MIN` is some large negative value, while `FLT_MIN` is the minimal normalized positive floating point value.
Weizhen Huang commented 2024-08-05 17:32:33 +02:00
Outdated
Review
Copy Link

0 or any small value is fine, is just an initialization before running the loop, I can change to 0 if that seems more understandable.

0 or any small value is fine, is just an initialization before running the loop, I can change to 0 if that seems more understandable.
Sergey Sharybin commented 2024-08-05 18:17:35 +02:00
Outdated
Review
Copy Link

I think 0 is better.
Otherwise it is not really clear how it's supposed to work when you have multiple overlapping/intersecting triangles.

I think 0 is better. Otherwise it is not really clear how it's supposed to work when you have multiple overlapping/intersecting triangles.
for (numhit_t i = numhit_t(1); i < max_record_hits; ++i) {
const float isect_t = INTEGRATOR_STATE_ARRAY(ctx->isect_s, shadow_isect, i, t);
if (isect_t > max_t) {
second_largest_t = max_t;
max_recorded_hit = i;
max_t = isect_t;
}
else if (isect_t > second_largest_t) {
second_largest_t = isect_t;
}
}
if (isect_index == max_record_hits && current_isect.t >= max_t) {
/* `ctx->max_t` was initialized to `ray->tmax` before the index exceeds the limit. Now that
* we have looped through the array, we can properly clamp `ctx->max_t`. */
weizhen marked this conversation as resolved Outdated
Sergey Sharybin commented 2024-08-05 17:20:42 +02:00
Outdated
Review
Copy Link

Not sure why max_t is not computed: the code above runs even on the first overflow and sets max_t. Is also a bit strange to see code that uses max_t after comment saying it is not yet computed.

Not sure why `max_t` is not computed: the code above runs even on the first overflow and sets `max_t`. Is also a bit strange to see code that uses `max_t` after comment saying it is not yet computed.
Weizhen Huang commented 2024-08-05 17:34:42 +02:00
Outdated
Review
Copy Link

I should say ctx->max_t was not yet computed.
Because we have not looped through the array at this point, so we don't have ctx->max_t available. I just initialzed it to ray->tmax to ensure this loop is run the first time the index exceeds the limit, after this loop ctx->max_t is available.

I should say `ctx->max_t` was not yet computed. Because we have not looped through the array at this point, so we don't have `ctx->max_t` available. I just initialzed it to `ray->tmax` to ensure this loop is run the first time the index exceeds the limit, after this loop `ctx->max_t` is available.
Sergey Sharybin commented 2024-08-05 18:20:32 +02:00
Outdated
Review
Copy Link

ctx->max_t makes more sense. The wording is still a bit strange, as "not yet computed" reads more like "is uninitialized", which is not true. Maybe we can phrase it something like "ctx->max_t is not yet clamped to the furthest away intersection" ?

`ctx->max_t` makes more sense. The wording is still a bit strange, as "not yet computed" reads more like "is uninitialized", which is not true. Maybe we can phrase it something like "`ctx->max_t` is not yet clamped to the furthest away intersection" ?
ctx->max_t = max_t;
return;
}
isect_index = max_recorded_hit;
/* Limit the ray distance and avoid processing hits beyond this. */
ctx->max_t = max_t;
/* If it's further away than max_t, we don't record this transparent intersection. */
if (current_isect.t >= max_t) {
return;
}
/* After replacing `max_t` with `current_isect.t`, the new largest t would be either
* `current_isect.t` or the second largest t before. */
ctx->max_t = max(second_largest_t, current_isect.t);
}
integrator_state_write_shadow_isect(ctx->isect_s, &current_isect, isect_index);
@ -870,6 +878,7 @@ ccl_device_intersect bool kernel_embree_intersect_shadow_all(KernelGlobals kg,
ctx.opaque_hit = false;
ctx.isect_s = state;
ctx.max_hits = numhit_t(max_hits);
ctx.max_t = ray->tmax;
ctx.ray = ray;
RTCRay rtc_ray;
kernel_embree_setup_ray(*ray, rtc_ray, visibility);