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

Should D3D11 sampler state actually be clamp? #5999

Closed
ghost opened this issue Dec 14, 2022 · 8 comments
Closed

Should D3D11 sampler state actually be clamp? #5999

ghost opened this issue Dec 14, 2022 · 8 comments

Comments

@ghost
Copy link

ghost commented Dec 14, 2022

(Click "Preview" above ^ to turn URL into clickable links)

  1. FOR FIRST-TIME USERS ISSUES COMPILING/LINKING/RUNNING or LOADING FONTS, please use GitHub Discussions.

  2. PLEASE CAREFULLY READ: FAQ

  3. PLEASE CAREFULLY READ: Contributing Guidelines

  4. PLEASE MAKE SURE that you have: read the FAQ; explored the contents of ShowDemoWindow() including the Examples menu; searched among Issues; used your IDE to search for keywords in all sources and text files; and read the links above.

  5. Be mindful that messages are being sent to the e-mail box of "Watching" users. Try to proof-read your messages before sending them. Edits are not seen by those users.

  6. Delete points 1-6 and PLEASE FILL THE TEMPLATE BELOW before submitting your issue.

Thank you!


(you may also go to Demo>About Window, and click "Config/Build Information" to obtain a bunch of detailed information that you can paste here)

Version/Branch of Dear ImGui:

All of them. I'm specifically on the docking branch, but the code in question is the same in all branches.

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_dx11.cpp
Compiler: n/a
Operating System: Windows

My Issue/Question:

The sampler state used in the D3D11 backend uses wrap modes instead of clamp:

desc.AddressU = D3D11_TEXTURE_ADDRESS_WRAP;
desc.AddressV = D3D11_TEXTURE_ADDRESS_WRAP;
desc.AddressW = D3D11_TEXTURE_ADDRESS_WRAP;

This is causing some artifacts when I try to render an image where I'm seeing some of the right side bleed over to the left.

My question is basically whether there's a need for the wrap clamp mode or if it would be reasonable for me to open a PR that changes this to clamp. I don't see any issues when I change it from wrap to clamp but I'm also not an expert in all the ways ImGui renders things. If that's not reasonable, then perhaps we need a way to specify so that I can render images without using the wrap mode.

Screenshots/Video

Here's what I see with the default wrap mode (notice the little green artifacts on the far left; you might have to make the image larger or zoom in to see them as they're quite small):

with-wrap

If I modify it to clamp I get the result I'd expect (notice there are no green artifacts):

with-clamp

Standalone, minimal, complete and verifiable example: (see #2261)

I don't have any minimal example on hand. I'm simply using ImGui::Image with a render target of my scene.

@GamingMinds-DanielC
Copy link
Contributor

GamingMinds-DanielC commented Dec 16, 2022

Simply changing from wrapping to clamping could break a lot of UIs, f.e. if a repeatable pattern gets drawn into the background. A switchable mode (by way of a parameter stack with the usual PushXyz() / PopXyz() combo) would avoid problems like that. In case of zoomed in render targets, a real border mode (D3D11_TEXTURE_ADDRESS_BORDER here, along with the correct border color which should then be part of the parameter stack) should yield even better results than just clamping.

Just to be sure: your render target is supposed to be zoomed in, right? If it is 1:1, there should be no artifacts with wrapping unless pixel centers are somehow messed up. And pixel centers look fine, otherwise the font would be blurry as well. Zooming out should not be a problem either. If it is supposed to be zoomed in, a workaround would be to rescale the render target into another render target yourself (maybe even properly upscaled like with FSR) for an exact pixel/texel match. If it is not supposed to be zoomed in, you should take a look into your render target handling and resize them as needed.

@ghost
Copy link
Author

ghost commented Dec 16, 2022

Thanks. I didn't know if there were repeated patterns or anything in ImGui. Totally makes sense not to change out the default, then. A parameter in the stack also sounds great since that would solve this as well.

This is zoomed in. It's a 720p render target scaled up to almost 4k.

@GamingMinds-DanielC
Copy link
Contributor

Of course, if you don't use wrap mode, you could just switch your sampler state to clamping. Would be an easy solution, but won't hold up if you ever need wrapping as well. But even if not, you would have to apply this change again every time you update sources, not very appealing.

Another solution that should work very well and would leave ImGui itself untouched: you could make a function that builds a "clamped" image for you. I didn't test this, but my approach would be something like this...

  • submit a dummy item and retrieve its corner positions
  • obtain the window draw list
  • submit the image rectangles for what is usually known as a "9-patch image"

The idea would be to split your image where the center of your first and last texel rows and columns are. For the top left corner patch, replicate uv_max into uv_min (don't start at 0, 0) to effectively clamp to the texel center. For the left border, replicate uv_max.x into uv_min.x. Correspondingly for the other patches, the result should then look exactly like clamping would. That function would need to know the image dimensions to make the correct splits.

Even more effective: building the 16 vertices and 18 triangles directly. And since you could then write the color per vertex, you could fade out to black (or whatever color you want) for the outer vertices and this way emulate proper border sampling. Set a color other than white for the inner vertices (or all vertices in case of clamping) and you basically have a neat little channel filter on top.

@PathogenDavid
Copy link
Contributor

Like Daniel said, this has been the default for a while and changing it out from under people would cause issues. (The sampler state is wrap by default in other backends too.)

If you're only targeting Direct3D 11 you can use a drawlist callback to change the sampler state:

// The callback
// (This assumes g_pd3dDeviceContext is a global like in example_win32_directx11)
static void SetSamplerCallback(const ImDrawList* parent_list, const ImDrawCmd* cmd)
{
    g_pd3dDeviceContext->PSSetSamplers(0, 1, (ID3D11SamplerState**)&cmd->UserCallbackData);
}
// Somewhere in your setup code
ID3D11SamplerState* clampedSampler;
{
    D3D11_SAMPLER_DESC desc;
    ZeroMemory(&desc, sizeof(desc));
    desc.Filter = D3D11_FILTER_MIN_MAG_MIP_LINEAR;
    desc.AddressU = D3D11_TEXTURE_ADDRESS_CLAMP;
    desc.AddressV = D3D11_TEXTURE_ADDRESS_CLAMP;
    desc.AddressW = D3D11_TEXTURE_ADDRESS_CLAMP;
    desc.MipLODBias = 0.f;
    desc.ComparisonFunc = D3D11_COMPARISON_ALWAYS;
    desc.MinLOD = 0.f;
    desc.MaxLOD = 0.f;
    g_pd3dDevice->CreateSamplerState(&desc, &clampedSampler);
}
// To use the callback:
ImDrawList* drawList = ImGui::GetWindowDrawList();
drawList->AddCallback(SetSamplerCallback, clampedSampler);
ImGui::Image(io.Fonts->TexID, { 256, 256 }, { 0, 0 }, { 2, 2 }); // Replace with your ::Image call
drawList->AddCallback(ImDrawCallback_ResetRenderState, nullptr);

@ghost
Copy link
Author

ghost commented Dec 18, 2022

Oh cheers that’s a great solution for me. Thanks again!

@ghost ghost closed this as completed Dec 18, 2022
@ocornut
Copy link
Owner

ocornut commented Dec 18, 2022

I would like if we can appreciate the initial problem, which is a texel outside of expected image is sampled and incorporated into bilinear filtering result.

Can you inspect the vertices values for that textured quad? (Available in Metrics/Debugger>Drawlists) and share them here?

@GamingMinds-DanielC
Copy link
Contributor

I would like if we can appreciate the initial problem, which is a texel outside of expected image is sampled and incorporated into bilinear filtering result.

Since this is a zoomed in image, that's the expected and correct outcome with wrapping. After all, sample positions get closer to the texture border than the texel centers of the first/last texel column/row due to the zoom. So that's not a problem in ImGui.

ocornut added a commit that referenced this issue Oct 7, 2024
…t texture sampler to Clamp instead of Repeat/Wrap. (#7468, #7511, #5999, #5502)
@ocornut
Copy link
Owner

ocornut commented Oct 7, 2024

I have changed all samplers to default to clamp: 42206b3
See #7230 (comment) for more details.

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

No branches or pull requests

3 participants