Skip to content

vk: implement DrawPicScaledCol for crosshair colouring - #64

Merged
0lvin merged 1 commit into
yquake2:masterfrom
runlevel5:crosshair-color
May 5, 2026
Merged

0lvin merged 1 commit into
yquake2:masterfrom
runlevel5:crosshair-color

Conversation

@runlevel5

Copy link
Copy Markdown
Contributor

@0lvin

0lvin commented May 4, 2026

Copy link
Copy Markdown
Contributor

@devnexen could you please review? It's looks as duplication of your change yquake2/yquake2remaster#126

Comment thread src/vk/vk_common.c
{
vk_drawTintedTexQuadPipeline[i].depthTestEnable = VK_FALSE;
QVk_CreatePipeline(samplerUboDsLayouts, 2, &vertInfoRG_RG, &vk_drawTintedTexQuadPipeline[i], &vk_renderpasses[i], shaders, 2);
QVk_DebugSetObjectName((uint64_t)vk_drawTintedTexQuadPipeline[i].layout, VK_OBJECT_TYPE_PIPELINE_LAYOUT,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

QVk_DrawColoredTexRect pushes 24 bytes at offset 68 (gamma + 8 pad + vec3), whereas QVk_DrawTexRect only pushes 4 at the same offset. if QVk_CreatePipeline sizes the
fragment push range from a fixed value rather than reflecting the SPIR-V, this layout is too small and the later vkCmdPushConstants either fails validation or hits UB
on drivers that enforce ranges. can you confirm the range covers [68, 92) here, ideally with VK_LAYER_KHRONOS_validation on while cycling crosshair_color_r/g/b?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the range is fine. QVk_CreatePipeline declares the fragment range as offset=68, size=11*sizeof(float)=44, covering [68, 112), which is a strict superset of the [68, 92) that QVk_DrawColoredTexRect
writes (24 bytes: 4 gamma + 8 pad + 12 vec3). Validation won't fire on the range check

Comment thread src/vk/vk_common.c
vkCmdPushConstants(vk_activeCmdbuffer, vk_drawTintedTexQuadPipeline[vk_state.current_renderpass].layout,
VK_SHADER_STAGE_FRAGMENT_BIT, VK_BASIC_FRAG_PC_OFFSET, sizeof(fragPC), &fragPC);

vkCmdBindDescriptorSets(vk_activeCmdbuffer, VK_PIPELINE_BIND_POINT_GRAPHICS,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is the call that depends on the tinted pipeline's fragment push range covering [68, 92) — see comment on the pipeline creation site.

Comment thread src/vk/vk_image.c Outdated
@0lvin
0lvin requested a review from devnexen May 5, 2026 18:33
@0lvin

0lvin commented May 5, 2026

Copy link
Copy Markdown
Contributor

@devnexen Could you please approve and I will merge if no objection?

@devnexen devnexen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LCTM

@0lvin
0lvin merged commit dcbb32b into yquake2:master May 5, 2026
9 checks passed
@0lvin

0lvin commented May 5, 2026

Copy link
Copy Markdown
Contributor

Thank you, merged!

@runlevel5
runlevel5 deleted the crosshair-color branch May 5, 2026 23:11
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