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

RenderingDevice: Index offset incorrectly applied twice #86838

Closed
pkdawson opened this issue Jan 5, 2024 · 1 comment · Fixed by #86852
Closed

RenderingDevice: Index offset incorrectly applied twice #86838

pkdawson opened this issue Jan 5, 2024 · 1 comment · Fixed by #86852

Comments

@pkdawson
Copy link
Contributor

pkdawson commented Jan 5, 2024

Tested versions

Reproducible in 4.3.dev1 and later.
Not reproducible in 4.2.1 and earlier.

System information

Godot v4.3.dev.mono (89cc635) - Windows 10.0.22631 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 4070 Ti (NVIDIA; 31.0.15.4629) - 13th Gen Intel(R) Core(TM) i5-13600K (20 Threads)

Issue description

The bug described in #68293 has crept back in during the RenderingDevice refactor, affecting both Vulkan and Direct3D 12. The index array is bound with an offset, but then that offset is applied again when drawing, giving the wrong result.

dl->validation.index_array_offset = index_array->offset;
driver->command_render_bind_index_buffer(dl->command_buffer, index_array->driver_id, index_array->format, index_array->offset);

driver->command_render_draw_indexed(dl->command_buffer, to_draw, p_instances, dl->validation.index_array_offset, 0, 0);

The same solution (#68303) fixes this. The p_offset parameter in command_render_bind_index_buffer could just be removed, but maybe it's needed for console graphics APIs, I dunno.

Steps to reproduce

Run the MRP, check whether there's a nice red-green-blue triangle centered in the screen.

index-bug-42

index-bug-43

Minimal reproduction project (MRP)

index-bug.zip

@clayjohn
Copy link
Member

clayjohn commented Jan 5, 2024

I think your proposed solution is correct. IIRC Vulkan is unique in that it lets you specify offsets at draw time. If other platforms need the offset at draw time, we can make the change in the Vulkan RDD and just ignore the p_offset parameter

CC @RandomShaper

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Immediate Blocker
Development

Successfully merging a pull request may close this issue.

2 participants