Page MenuHome

Fix T96165: Incorrect render of barcelone scene with BVH2
ClosedPublic

Authored by Sergey Sharybin (sergey) on Mar 8 2022, 2:50 PM.

Details

Summary

The issue was caused by incorrect check for exceeded number
of transparent bounces: the same maximum distance was used
for picking up N closest intersections and counting overall
intersections count.

Now made it so intersection count is using ray distance which
matches the way how Embree and OptiX implementation works.

Additionally, T95462 is probably fixed with this change,
although some closer look is needed.

This is likely to cause some performance loss, but this is
unlikely to be avoided.


One thing I've notices is that the ray distance of FLT_MAX
will be "shortened" after instance push. Technically, this
can lead to missing intersection checks, but probably unlikely
to happen in practice. Can address it in a separate patch.

Diff Detail

Repository
rB Blender
Branch
fix_T96165 (branched from master)
Build Status
Buildable 20920
Build 20920: arc lint + arc unit

Event Timeline

Sergey Sharybin (sergey) requested review of this revision.Mar 8 2022, 2:50 PM
Sergey Sharybin (sergey) created this revision.

Benchmark result:

Interestingly enough, no big difference in performance rendering pabellon. The Victor is 4% slower, but the number is not really reliable :( The second run showed only 1% slowdown.

Brecht Van Lommel (brecht) requested changes to this revision.Mar 8 2022, 4:24 PM

Using a shorter t_max_current for intersection of BVH nodes and primitives was meant to be an optimization, to avoid unnecessary intersections (which is not possible with Embree / OptiX APIs since we can't modify it). But I guess it doesn't help that much in practice and it's better to unify things.

intern/cycles/kernel/bvh/shadow_all.h
257–275

isect.t < t_max_current is comparing a world space distance to a local space one. It should be isect.t < t_max_world.

That also means t_max_current is unused, so it can be removed. I suggest to rename ray_t to t_max_current then and update the comment.

This revision now requires changes to proceed.Mar 8 2022, 4:24 PM

Good catch about world vs. local space comparison!

Missed part of the change in the previous update.

This revision is now accepted and ready to land.Mar 8 2022, 5:49 PM