Skip to content

Vulkan: Viewport and Scissor offset#9327

Closed
GDBobby wants to merge 6 commits into
ocornut:masterfrom
GDBobby:master
Closed

Vulkan: Viewport and Scissor offset#9327
GDBobby wants to merge 6 commits into
ocornut:masterfrom
GDBobby:master

Conversation

@GDBobby
Copy link
Copy Markdown

@GDBobby GDBobby commented Mar 28, 2026

The goal was to improve viewport and scissor customization. Currently, it's assumed the viewport will start in the top left corner, and there's no way to change that.

I made it so the VkViewport.x and VkViewport.y can be set via draw_data->OwnerViewport->Pos

A few bugs were found, most likely because the values were always 0 and never changed. I'll leave review comments where applicable.

2026-03-27.23-25-47.mp4

if (clip_min.y < 0.0f) { clip_min.y = 0.0f; }
if (clip_max.x > fb_width) { clip_max.x = (float)fb_width; }
if (clip_max.y > fb_height) { clip_max.y = (float)fb_height; }
if (clip_max.x <= clip_min.x || clip_max.y <= clip_min.y)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

these are acceptable values, as seen immediately below at line 645, scissor is a position and size, as opposed to a min and max.

Comment thread backends/imgui_impl_vulkan.cpp Outdated
{
// Project scissor/clipping rectangles into framebuffer space
ImVec2 clip_min((pcmd->ClipRect.x - clip_off.x) * clip_scale.x, (pcmd->ClipRect.y - clip_off.y) * clip_scale.y);
ImVec2 clip_min(pcmd->ClipRect.x * clip_scale.x, pcmd->ClipRect.y * clip_scale.y);
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

this double applied the offset, and cut pixels (fragments?) from the right when they were supposed to be removed from the left.

Comment thread imgui.cpp
// FIXME-VIEWPORT: Size is driven by backend/user code for backward-compatibility but we should aim to make this more consistent.
ImGuiViewportP* main_viewport = g.Viewports[0];
main_viewport->Flags = ImGuiViewportFlags_IsPlatformWindow | ImGuiViewportFlags_OwnedByApp;
main_viewport->Pos = ImVec2(0.0f, 0.0f);
Copy link
Copy Markdown
Author

@GDBobby GDBobby Mar 28, 2026

Choose a reason for hiding this comment

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

previously, this was a wasted instruction as Pos was initialized as 0 and never changed. now, it would cause the viewport and scissor to be out of sync if left alone.

@ocornut
Copy link
Copy Markdown
Owner

ocornut commented Mar 30, 2026

The goal was to improve viewport and scissor customization. Currently, it's assumed the viewport will start in the top left corner, and there's no way to change that.

Whar's the purpose of changing that? What are you trying to achieve?

This whole PR seems confusing and clumsy but I would like to understand what you are trying to do.

-- ImVec2 clip_min((pcmd->ClipRect.x - clip_off.x) * clip_scale.x, (pcmd->ClipRect.y - clip_off.y) * clip_scale.y);
++ ImVec2 clip_min(pcmd->ClipRect.x * clip_scale.x, pcmd->ClipRect.y * clip_scale.y);

this double applied the offset, and cut pixels (fragments?) from the right when they were supposed to be removed from the left.

This transformed clip rectangle from global coordinates into framebuffer coordinates.

these are acceptable values, as seen immediately below at line 645, scissor is a position and size, as opposed to a min and max.

What's the purpose of changing this?

@GDBobby
Copy link
Copy Markdown
Author

GDBobby commented Mar 30, 2026

It was clumsy of me to fix the bug but leave clip_min and clip_max as they were. In my lastest commit, I combined the two into framebuffer_scissor_rect, which is more reflective of how VkScissor works. Does that clear up the previous confusion?

The end goal is to have multiple contexts and viewports within 1 window. This PR is step 1 towards that goal.

@ocornut
Copy link
Copy Markdown
Owner

ocornut commented Mar 30, 2026

The end goal is to have multiple contexts and viewports within 1 window.

Why?

This PR is step 1 towards that goal.

Please try to reach your goal before making intermediary PR that would be breaking existing API.

Note that the multi-viewports system sets viewport->Pos as values are in global coordinates space.

@ocornut
Copy link
Copy Markdown
Owner

ocornut commented Mar 31, 2026

I've merged the vkCmdPushConstants() change as part of b003a85. (#9183)

For the rest, I'm closing for now as presently I can't trust any of it. Considering that: coding style is off, repeated commits are clumsy, there is a lack of explanation as to what is probably a XY Problem and you stated yourself this is an intermediary change toward a bigger thing which you haven't achieved yet.

Please consider reaching your target and then you can evaluate what you need and make a case for it and I would happily evaluate your idea. But any time there's a coding style mismatch or unexplained or breaking changes you are exhibiting carelessness and losing trust points. So please get back to the drawing board first. Good luck :)

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.

2 participants