Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AABB projection for meshlet occlusion culling #12884

Closed
wants to merge 5 commits into from

Conversation

JMS55
Copy link
Contributor

@JMS55 JMS55 commented Apr 5, 2024

The previous code was incorrect for non-power-of-2 view sizes, as e.g. view_size = 70 -> depth_pyramid_size = 64, and 64 / 70 != 0.5.

@JMS55 JMS55 added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Apr 5, 2024
@JMS55 JMS55 mentioned this pull request Apr 5, 2024
40 tasks
Copy link
Contributor

@pcwalton pcwalton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine.

As an aside, this kind of bug is exactly why I want to consolidate as much of the meshlet and non-meshlet code for Hi-Z testing as possible. Hi-Z test code is tricky and it'd be best to solve these bugs only once.

Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not correct, but for unrelated reasons.
my original argument for the 0.5 factor was predicated on the copy_material_depth pass being correct, but it is not. the copy_material_depth misses some pixels as it samples a non-power-of-two texture at rounded-down power-of-to-two resolution.

illustration:

|___|___|___|___|___|  view depth (5 pixels wide)
|____|____|____|____|  hzb mip 0 (4 pixels wide)
current copy_material_depth:
|___|___|___|___|___|  4 pixels are sampled
  |   |       |   |
  v   v       v   v
|____|____|____|____|  hzb loses middle pixel depth
what copy_material_depth should be doing:
|___|___|___|___|___| 
  |  /|  / \  |\  |
  v v v v   v v v v
|____|____|____|____| hzb samples 2x2 of all view depth pixels touching current pixel

two examples

view: 33 hzb0: 32
in this case every view pixel is 1/33 wide and every hzb0 pixel is 1/32 wide, every hzb0 pixel covers 2 view pixels so each represents 2/33 of the view buffer.
to transform uv from view to hzb0: uv * hzb0_pixel_width / (2*view_pixel_width) = uv * (1/32) / (2/33) = uv * 33/64
but uvs are in integer pixel space, not floats, so we also need to account for texture resolution:
uv * 33/64 * (hzb0_res/view_res) = uv * 33/64 * 32/33 = uv * 32/64 = uv * 0.5

view: 31 hzb0: 16
in this case every view pixel is 1/31 wide and every hzb0 pixel is 1/16wide, every hzb0 pixel covers 2 view pixels so each represents 2/31 of the view buffer.
to transform uv from view to hzb0: uv * hzb0_pixel_width / (2*view_pixel_width) = uv * (1/16) / (2/31) = uv * 31/32
but uvs are in integer pixel space, not floats, so we also need to account for texture resolution:
uv * 31/32 * (hzb0_res/view_res) = uv * 31/32 * 16/31 = uv * 32/64 = uv * 0.5

conclusion:
the terms cancel out when doing the math in pixel space. it would initially appear that a dynamic aspect fraction is needed but that is not the case when working in integer pixel coordinates. we just need to scale by 0.5 and fix copy_material_depth.

@JMS55 JMS55 marked this pull request as draft April 9, 2024 14:45
@JMS55 JMS55 closed this Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants