bgfx icon indicating copy to clipboard operation
bgfx copied to clipboard

D3D12: Fix blit when MSAA is enabled and ResolveSubResource texture format parameter

Open julianxhokaxhiu opened this issue 2 years ago • 5 comments

julianxhokaxhiu avatar Jun 26 '22 10:06 julianxhokaxhiu

I don't understand the dstLocation.pResource change. You're blitting to the resolve target of a different MSAA texture. If you need to blit from a normal texture or resolve an MSAA texture, you should just use a non-MSAA target texture to begin with, instead of abusing texture internals.

pezcode avatar Jun 26 '22 11:06 pezcode

No I'm just using the blit command to copy one texture to another, as it is. This change is all about allowing this.

Also, if the dest texture has not MSAA set, it will fall back to the current code case ( previous to this patch ). But if it does, it should inherit that logic, I think.

julianxhokaxhiu avatar Jun 26 '22 13:06 julianxhokaxhiu

No I'm just using the blit command to copy one texture to another, as it is

You're blitting from non-MSAA or resolved-MSAA to MSAA, that logic seems nonsensical to me.

Imagine this blit scenario:

  • src is MSAA
  • dst is MSAA

Previously: With BGFX_TEXTURE_MSAA_SAMPLE on src, this correctly blits the raw MSAA texture*. Without it, you get an error because you're asking for a resolve, but your resolve target is MSAA (which makes no sense).

With your change: With BGFX_TEXTURE_MSAA_SAMPLE it now errors, so you can no longer blit between raw MSAA textures. For the second case, you now "resolve" src into dst. But it's now out of sync, because the next time you use it in a framebuffer, it still has the old content - you only overwrote the temporary resolve texture (m_singleMsaa).

The solution is straight-forward: blit to a non-MSAA texture if you want the resolved version.


* This BGFX_TEXTURE_MSAA_SAMPLE is a bit awkward and would ideally be detected automatically. So if you blit MSAA -> MSAA it should always copy the raw texture.

pezcode avatar Jun 26 '22 14:06 pezcode

I don't understand why I should use another texture while this patch just auto-detects on which case are you, and uses the right way to approach the copy. All cases are covered and nothing was "broken". Also, how am I supposed to copy an MSAA texture into another one ( when both have the MSAA flag ) right now with the current APIs on DX12?

Anyway, I'm open to suggestions for this patch, but at the moment the fact is that if you try to blit from an MSAA flagged texture to another MSAA one it will just not work. So which option do you give here? This works perfectly fine under all renderers but DX12 ( also, using exactly this logic ) at the current stage.

julianxhokaxhiu avatar Aug 06 '22 15:08 julianxhokaxhiu

This works perfectly fine under all renderers but DX12 ( also, using exactly this logic ) at the current stage.

Just so I understand the issue exactly:

You blit from MSAA to MSAA.

  • This works with the Vulkan backend, but not D3D12?
  • Does it work when you add the BGFX_TEXTURE_MSAA_SAMPLE flag on the source texture?
  • What do you do with the destination texture after blitting? Sample as texture? Keep rendering into?

pezcode avatar Aug 06 '22 22:08 pezcode

This works with the Vulkan backend, but not D3D12?

Yes, also OpenGL and DX11 works

Does it work when you add the BGFX_TEXTURE_MSAA_SAMPLE flag on the source texture?

Didn't try but I can try to replicate this situation on a bgfx demo

What do you do with the destination texture after blitting? Sample as texture? Keep rendering into?

It's the blit of a depth texture and we keep rendering on it

julianxhokaxhiu avatar Aug 07 '22 07:08 julianxhokaxhiu