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

GSdx-D3D: Texture and channel shuffle improvements #2347

Merged
merged 1 commit into from Mar 23, 2018

Conversation

Projects
None yet
4 participants
@lightningterror
Member

lightningterror commented Mar 18, 2018

Texture Shuffle changes:
Always Enable Texture shuffle on D3D10/11.
Previously Texture shuffle was enabled if CRC hack
level was below Full, this was kinda not good since
D3D also relies on CRC hacks on Full so you could either
stick with texture shuffle or crc hacks.

Texture shuffle is not supported on D3D9, however we can do a partial
port where instead of vertical lines with the effect we get the effect
on the entire screen. Better than nothing I suppose.

Ported some of the code from OpenGL to D3D
( just a copy - paste job :) ),
part of the code misses a dedicated shader but we can still
use it to fix various issues on many games.

List of affected games tested so far:
The Godfather, Final Fight Streetwise, The Suffering Ties that Bind,
Urban Chaos have their vertical lines issues fixed
(highly possible for other games as well), MGS and Stolen see an improvement
but they are still broken without crc hacks. Other games that suffered
similar issues are probably affected as well.

Channel Shuffle changes:
Update Channel Shuffle detection. A lot of games should see an improvement,
MGS, Urban Chaos, Stolen have their top left corner issues resolved.
Other games should be affected as well that use similar logic.
They still miss a shader so some effects are still broken/show glitches
but it's a nice improvement for D3D users.
Improves #1318

Shared changes:
Texture Shuffle and Channel shuffle have been moved to their
own dedicated functions. Should make things a bit cleaner.

Move part of the code for Texture Shuffle to GSRendererHW to be shared
across all HW renderers, should aboid copy paste/duplicate code.

GS Dumps for testing:
Final Fight Streetwise.gs.zip
The.Godfather.gs.zip

@refractionpcsx2

This comment has been minimized.

Member

refractionpcsx2 commented Mar 19, 2018

Nice job dude :)

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Mar 19, 2018

Which shader do you miss ?

@lightningterror

This comment has been minimized.

Member

lightningterror commented Mar 19, 2018

#if PS_WRITE_RG == 1

void ps_fbmask(inout vec4 C)

Well blending too since it's connected but nvm those.

@lightningterror lightningterror force-pushed the lightningterror:cs-ts branch Mar 19, 2018

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Mar 19, 2018

OK, in this case you should add a note that they can't be supported on DX11. Unlike GL, you can't read the current framebuffer.

And did you check the behavior of shader for those parameters

PS_SHUFFLE and PS_READ_BA

By the way, did you check DX9 behavior ? DX9 GPUs don't support integer operation... (note I don't give a damn about dx9, it is only to be aware of the situation)

@lightningterror lightningterror force-pushed the lightningterror:cs-ts branch Mar 19, 2018

@lightningterror

This comment has been minimized.

Member

lightningterror commented Mar 19, 2018

OK, in this case you should add a note that they can't be supported on DX11. Unlike GL, you can't read the current framebuffer.

Done

And did you check the behavior of shader for those parameters
PS_SHUFFLE and PS_READ_BA

Yeah SHADER_MODEL >= 0x400

By the way, did you check DX9 behavior ? DX9 GPUs don't support integer operation... (note I don't give a damn about dx9, it is only to be aware of the situation)

It doesn't work since the shader is for dx10+

@MrCK1

This comment has been minimized.

Member

MrCK1 commented Mar 19, 2018

Forgot to mention this earlier.

It night be helpful to add comments to the code that doesn't work yet in DX11 to avoid confusion. (such as the Tomb Raider case)

@lightningterror

This comment has been minimized.

Member

lightningterror commented Mar 19, 2018

I'll also isolate the code or just make a skip value for the mask code on dx9 since it sometimes makes the render not show any image, don't want to break dx9 any further.

@lightningterror lightningterror added the WIP label Mar 19, 2018

@lightningterror lightningterror force-pushed the lightningterror:cs-ts branch Mar 19, 2018

@lightningterror lightningterror removed the WIP label Mar 19, 2018

@lightningterror

This comment has been minimized.

Member

lightningterror commented Mar 19, 2018

It night be helpful to add comments to the code that doesn't work yet in DX11 to avoid confusion. (such as the Tomb Raider case)

I think it's fine as it is since it's a partial port. Plus it also works with blending on gl.

@lightningterror lightningterror force-pushed the lightningterror:cs-ts branch 2 times, most recently Mar 19, 2018

plugins/GSdx/GSRendererDX.cpp Outdated
@@ -23,10 +23,14 @@
#include "GSRendererDX.h"
#include "GSDeviceDX.h"

bool s_IS_DX1011 = false;

This comment has been minimized.

@gregory38

gregory38 Mar 21, 2018

Contributor

You should make the variable local to the class and use the m_ prefix instead.

plugins/GSdx/GSRendererDX.cpp Outdated
size_t count = m_vertex.next;
GSVertex* v = &m_vertex.buff[0];

// Gregory: code is not yet ready but it fixes vertical lines issues(partially on some) (MGS, The Godfather, Final Fight Streetwise) so let's enable it.

This comment has been minimized.

@gregory38

gregory38 Mar 21, 2018

Contributor

maybe remove my old comment.

plugins/GSdx/GSRendererDX.cpp Outdated

void GSRendererDX::EmulateChannelShuffle(GSTexture** rt, const GSTextureCache::Source* tex)
{
dev = (GSDeviceDX*)m_dev;

This comment has been minimized.

@gregory38

gregory38 Mar 21, 2018

Contributor

This variable doesn't seem to be used in this function or did I miss it.

This comment has been minimized.

@lightningterror

lightningterror Mar 21, 2018

Member

No, it's only used for setting shader resource and cb. Thought it might come in handy in the future when(if) the shaders get ported. I can remove it.

This comment has been minimized.

@gregory38

gregory38 Mar 21, 2018

Contributor

Note that I did those cast in OpenGL because I only have a single device. This way the compiler can replace the dynamic selection of the device by a static call.

Here, it might not help the compiler because it would need to chose Dx9 device or Dx11 device.

}
} else if ((tex->m_texture->GetType() == GSTexture::DepthStencil) && !(tex->m_32_bits_fmt)) {
// So far 2 games hit this code path. Urban Chaos and Tales of Abyss.
// Lacks shader like usual but maybe we can still use it to skip some bad draw calls.

This comment has been minimized.

@gregory38

gregory38 Mar 21, 2018

Contributor

You should fire the exception GSDXRecoverableError to skip current draw call. And likely the same for all branches that are a-not-yet-emulated-in-shader channel effect.

This comment has been minimized.

@lightningterror

lightningterror Mar 21, 2018

Member

GSDXRecoverableError seems like an even better solution, I'm noticing a difference between this pr and my local changes. Top left corner issue seems to be resolved on many games.

@lightningterror lightningterror force-pushed the lightningterror:cs-ts branch 3 times, most recently Mar 21, 2018

plugins/GSdx/GSRendererDX.cpp Outdated
size_t count = m_vertex.next;
GSVertex* v = &m_vertex.buff[0];

bool m_IS_DX1011 = false;

This comment has been minimized.

@gregory38

gregory38 Mar 22, 2018

Contributor

No, you need to set (cache) the value during the init of the object.

Otherwise a much much nicer implementation would be to create a virtual function with 2 implementations. 1 for Dx9 and 1 for Dx10. You can look at SetupIA if you want an example.

@lightningterror lightningterror force-pushed the lightningterror:cs-ts branch Mar 22, 2018

@lightningterror

This comment has been minimized.

Member

lightningterror commented Mar 22, 2018

Otherwise a much much nicer implementation would be to create a virtual function with 2 implementations. 1 for Dx9 and 1 for Dx10. You can look at SetupIA if you want an example.

Something like this? Let me know so I can squash.

plugins/GSdx/GSRendererDX9.cpp Outdated
// We can do a partial port for D3D9 that skips the draw call to give it a slight improvement.
// It's still broken but more bearable. Broken effect is on the screen but fully instead of vertical lines.
throw GSDXRecoverableError();
}

This comment has been minimized.

@gregory38

gregory38 Mar 22, 2018

Contributor

Somehow, it feels like it miss an else part.

This comment has been minimized.

@lightningterror

lightningterror Mar 22, 2018

Member
else {
		//m_ps_sel.fmt = GSLocalMemory::m_psm[m_context->FRAME.PSM].fmt;

		om_bsel.wrgba = ~GSVector4i::load((int)m_context->FRAME.FBMSK).eq8(GSVector4i::xffffffff()).mask();
	}

Tho that's part of dx11 only.

This comment has been minimized.

@gregory38

gregory38 Mar 22, 2018

Contributor

why is it dx11 only ?

This comment has been minimized.

@lightningterror

lightningterror Mar 22, 2018

Member

Oh nvm, I thought it was part of the shader that's for texture shuffle.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Mar 22, 2018

Yes it is better this way. Just need to ensure fbmask is properly (well not too wrongly) emulated on both dx9/dx11.

@lightningterror lightningterror force-pushed the lightningterror:cs-ts branch Mar 22, 2018

@lightningterror lightningterror force-pushed the lightningterror:cs-ts branch 2 times, most recently Mar 22, 2018

plugins/GSdx/GSRendererDX9.cpp Outdated
size_t count = m_vertex.next;
GSVertex* v = &m_vertex.buff[0];

// Shadow_of_memories_Shadow_Flickering (Okami mustn't call this code)

This comment has been minimized.

@gregory38

gregory38 Mar 22, 2018

Contributor

Hum this code should be moved where we compute the value of m_texture_shuffle (likely in GSRendererHW). I don't like the copy-past for each renderers

This comment has been minimized.

@gregory38

gregory38 Mar 22, 2018

Contributor

Yes below this line in RendererHW. Add the below block, because this block correct the detection of m_texture_shuffle. It should be done as soon as we detect the effect.

@lightningterror lightningterror force-pushed the lightningterror:cs-ts branch Mar 22, 2018

plugins/GSdx/GSRendererHW.cpp Outdated
GSVertex* v = &m_vertex.buff[0];

// Shadow_of_memories_Shadow_Flickering (Okami mustn't call this code)
if (m_texture_shuffle && count < 3 && PRIM->FST && (m_context->FRAME.FBMSK == 0)) {

This comment has been minimized.

@gregory38

gregory38 Mar 22, 2018

Contributor

2 small nitpicks

  • Replace count by m_vertex.next (remove count declaration).
  • Move GSVertex* v = &m_vertex.buff[0]; below i.e just after the GL_INS("WARNING: Possible misdetection of a texture shuffle effect");

@lightningterror lightningterror force-pushed the lightningterror:cs-ts branch Mar 22, 2018

@lightningterror

This comment has been minimized.

Member

lightningterror commented Mar 22, 2018

Don't merge it yet, I'm noticing a small issue.

Edit: odd sometimes I can get the picture to freeze on Final Fight Streetwise when switching between renders as well as hw/sw.

@lightningterror lightningterror force-pushed the lightningterror:cs-ts branch Mar 22, 2018

@lightningterror

This comment has been minimized.

Member

lightningterror commented Mar 22, 2018

Never mind, the issue is also present on master branch, maybe something to do with sw blending on Final Fight Streetwise ? I'm not sure, anyway you can merge it if it looks good.

edit: switching between oglhw/sw to dx9 on master
switching between oglhw/sw or dx11hw/sw to dx9 on this pr seems to cause the frozen or black screen display with sound issue.
We can leave it at that 😄 Dx9 is a dead render anyway.

@lightningterror lightningterror force-pushed the lightningterror:cs-ts branch Mar 22, 2018

@lightningterror lightningterror added this to the Release 1.6 milestone Mar 22, 2018

GSdx-D3D: Texture and channel shuffle improvements.
Texture Shuffle changes:
Always Enable Texture shuffle on D3D10/11.
Previously Texture shuffle was enabled if CRC hack
level was below Full, this was kinda not good since
D3D also relies on CRC hacks on Full so you could either
stick with texture shuffle or crc hacks.

Texture shuffle is not supported on D3D9, however we can do a partial
port where instead of vertical lines with the effect we get the effect
on the entire screen. Better than nothing I suppose.

Ported some of the code from OpenGL to D3D
( just a copy - paste job :) ),
part of the code misses a dedicated shader but we can still
use it to fix various issues on many games.

List of affected games tested so far:
The Godfather, Final Fight Streetwise, The Suffering Ties that Bind,
Urban Chaos have their vertical lines issues fixed
(highly possible for other games as well), MGS and Stolen see an improvement
but they are still broken without crc hacks. Other games that suffered
similar issues are probably affected as well.

Channel Shuffle changes:
Update Channel Shuffle detection. A lot of games should see an improvement,
MGS, Urban Chaos, Stolen have their top left corner issues resolved.
Other games should be affected as well that use similar logic.
They still miss a shader so some effects are still broken/show glitches
but it's a nice improvement for D3D users.

Shared changes:
Texture Shuffle and Channel shuffle have been moved to their
own dedicated functions. Should make things a bit cleaner.

Move part of the code for Texture Shuffle to GSRendererHW to be shared
across all HW renderers, should aboid copy paste/duplicate code.

@lightningterror lightningterror force-pushed the lightningterror:cs-ts branch to 32335a4 Mar 23, 2018

@gregory38 gregory38 merged commit 0e83b89 into PCSX2:master Mar 23, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lightningterror lightningterror deleted the lightningterror:cs-ts branch Mar 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment