zeux / niagara

A Vulkan renderer written from scratch on stream
MIT License
1.26k stars 72 forks source link

Incorrect frustum symmetry culling in bottom right quadrant #5

Closed gwihlidal closed 5 years ago

gwihlidal commented 5 years ago

I noticed incorrect frustum culling (relying on frustum symmetry assumption) in the bottom right quadrant.

Culling Off:

CullingOff

Culling On:

CullingOn

Difference: Difference

The difference view clearly shows the issue. The other differences in the frame are likely from the infinite projection, so it should be fine to ignore those ones (can be tuned).

I started with a canonical frustum symmetry test:

// fit the center to the positive quadrant to reduce view plane tests; only use positive planes
vec3 pos_center = vec3(abs(center.x), abs(center.y), center.z);

// unpack the view space plane equations
vec3 plane1 = vec3(cullData.frustum[0], 0.0f, cullData.frustum[1]);
vec3 plane2 = vec3(0.0f, cullData.frustum[2], cullData.frustum[3]);

visible = visible && !((dot(plane1.xyz, pos_center.xyz) < -radius) || (dot(plane2.xyz, pos_center.xyz) < -radius));

Then I expanded it, and then rearranged the equations to match the original code.

visible = visible && dot(plane1.xyz, pos_center.xyz) > -radius;
visible = visible && dot(plane2.xyz, pos_center.xyz) > -radius;

visible = visible && (plane1.x * pos_center.x + plane1.y * pos_center.y + plane1.z * pos_center.z) > -radius;
visible = visible && (plane2.x * pos_center.x + plane2.y * pos_center.y + plane2.z * pos_center.z) > -radius;

visible = visible && pos_center.z * plane1.z + plane1.x * pos_center.x > -radius;
visible = visible && pos_center.z * plane2.z + plane2.y * pos_center.y > -radius;

The issue ended up being the subtractions instead of additions from the expanded dot products. Fixing the operations produces the expected image, while maintaining expected culling rate.

Fixed in: https://github.com/zeux/niagara/pull/4

zeux commented 5 years ago

I definitely see the bug using pirate.obj at 1440p full screen, but I'm not sure the fix is the one you suggest. Or maybe it is, but I want to convince myself that it is...

The reason why I'm doubting the fix is that the culling rates post-fix are much worse as far as I can tell. With just left/top/right/bottom (no far plane) and just frustum culling - no occlusion - before this fix I get 678M -> 98M reduction (~7x), and after this fix I get 678M -> 457M which seems way too small. I suspect the bug or fix is different somehow. I'll need to re-derive the culling equations to try to understand what's going on here.

zeux commented 5 years ago

The problematic mesh has mesh id 303737. The issue is in this line:

vec3 center = mesh.center draws[di].scale + draws[di].position; float radius = mesh.radius draws[di].scale;

Whenever I introduced rotation support, I forgot to rotate the center :( even though bounding sphere is rotationally invariant, the mesh has an offset bounding sphere so the center of that sphere needs to be rotated. I'll submit a fix.