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

Vulkan dump resources: Fix in vertex buffer size calculation #1915

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

panos-lunarg
Copy link
Contributor

No description provided.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 322779.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5543 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5543 passed.

@hongSARC
Copy link

hongSARC commented Dec 18, 2024

I have tested this build, but now the vertices points I get back for one draw call is huge.

  • The file size for same drawindex call now generates vertexBuffer file that is 1,500kb when it was previously 3kb.
  • For example, I get a draw call with 191896 vertices when index data is in range 0~371.

I am still getting back clusters of position data that does not match the index for different draw calls so the mesh looks the same. I just have bigger vertices data.

@hongSARC
Copy link

It seems like the current implementation only adds the vertex_offset to the greatest index, which is incomplete.
This does not account for the smallest index in the index_buffer, leading to an large vertex range being fetched.

vertex_count = greatest_vertex_index + 1;

This only considers the greatest vertex index, which assumes the vertex range starts at 0.
Instead, it needs to be caculated based on the smallest and greatest indices (adjusted by vertex_offset)

So something like
vertex_count = (greatest_vertex_index - smallest_vertex_index) +1

@hongSARC
Copy link

I did confirm after adjusting my parser directly with the vertex_offset, my mesh does look correct.

@panos-lunarg
Copy link
Contributor Author

I did confirm after adjusting my parser directly with the vertex_offset, my mesh does look correct.

Great.
So if I understood correctly you get correct results with this PR applied and after modifying the vertex count line from
vertex_count = greatest_vertex_index + 1;
into
vertex_count = (greatest_vertex_index - smallest_vertex_index) +1.
Is that correct?

@hongSARC
Copy link

hongSARC commented Dec 19, 2024

No, what I have tested is the actual data that was pulled from this pr, I have parsed the same draw call with adding vertex_offset directly to that same draw call. Which means that the draw call contains all the correct data, but with additional unused data for that draw call.

vertex_count = greatest_vertex_index + 1; into vertex_count = (greatest_vertex_index - smallest_vertex_index) +1. Is that correct?

This is what I am assuming to be the issue with current pr, which is only accounting vertex_offset for greatest_vertex_index only.

For example,
if the smallest index is 3 and greatest index is 7 inside index values, and vertex_offset is 2,
current vertex_count calcualtion gives

greatest index: 7 + 2 = 9
vertex_count = 9 + 1 = 10

which incorrectly counts the range from 0 to 9.

If we adjust both smallest and greatest indices by the vertex_offset,

smallest index: 3+(2) = 5
greatest index: 7+(2) = 9

now calculating
vertex_count = (9 - 5) + 1 = 5

which shows range 0 to 4.

This would be the correct vertex_count, representing the exact number of vertices used for that draw call.

@panos-lunarg
Copy link
Contributor Author

OK I think I understand your point which makes sense. I forced pushed some change to incorporate the logic you describe.
If I didn't get this right and since you have the ability to quickly test the code please don't hesitate to create a PR for this PR with the correct logic.

@panos-lunarg panos-lunarg force-pushed the vulkan_dump_resources_include_vertexOffset_when_dumping_vertex_buffers branch from b681a81 to 9445601 Compare December 20, 2024 06:25
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 328500.

@panos-lunarg panos-lunarg marked this pull request as ready for review December 20, 2024 06:25
@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5624 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5624 passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants