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 usage of index offsets in RenderingDevice #86852

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

pkdawson
Copy link
Contributor

@pkdawson pkdawson commented Jan 5, 2024

Fixes #86838

Removed duplicate usage of index offset. I tested Direct3D on Windows 11, it looks like it works the same way as Vulkan in this respect, so it also required a fix to render correctly.

@pkdawson pkdawson requested a review from a team as a code owner January 5, 2024 21:54
@clayjohn clayjohn added this to the 4.3 milestone Jan 6, 2024
@clayjohn clayjohn requested a review from RandomShaper January 6, 2024 00:23
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

The change makes sense to me. But I would like @RandomShaper to confirm that its fine to merge

@RandomShaper
Copy link
Member

I acknowledge the issue boils down to the offset being applied twice. However, I'd like to state a different take on the solution.

To me, the drivers are doing the right thing. I mean, at their level of abstraction, they have no knowledge about what the offset is for, so they must just apply it. The problem lies at the RenderingDevice level, because that's the one doing the double-application. It binds with offset and then asks to render with the offset again. My suggested fix would be then to stop RenderingDevice from re-applying the offset at draw time.

This (untested) patch is what I'd do:
p.txt

@pkdawson pkdawson force-pushed the fix-index-offsets-again branch from 6623a19 to 53c0582 Compare January 8, 2024 15:47
@pkdawson pkdawson changed the title Fix usage of index offsets in RenderingDevice drivers Fix usage of index offsets in RenderingDevice Jan 8, 2024
@pkdawson
Copy link
Contributor Author

pkdawson commented Jan 8, 2024

As described previously in #68293, RenderingDevice::index_array_create takes the offset as a number of indices, but the drivers want the offset in bytes, so that needed an adjustment. Confirmed that this solution works too.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great! Sorry for breaking this twice now 😬

@akien-mga
Copy link
Member

This has conflicts following the merge of #84976, could you rebase (and I would suggest re-testing, as #84976 changed things significantly).

@pkdawson pkdawson force-pushed the fix-index-offsets-again branch from 53c0582 to a0f9bcc Compare January 9, 2024 14:53
@pkdawson
Copy link
Contributor Author

pkdawson commented Jan 9, 2024

Rebased. Confirmed that this works with my MRP and my more complex test case of Dear ImGui meshes (https://github.com/pkdawson/imgui-godot), with no other obvious bugs.

@akien-mga akien-mga merged commit 476cbbd into godotengine:master Jan 9, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

RenderingDevice: Index offset incorrectly applied twice
4 participants