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

D3D: Replaced shader-based depth range remap with viewport #1392

Merged
merged 4 commits into from Dec 3, 2014

Conversation

10 participants
@kayru
Contributor

kayru commented Oct 24, 2014

This fixes UI rendering in some games mentioned in https://code.google.com/p/dolphin-emu/issues/detail?id=7785

It's not clear why the vertex shader depth remap was introduced in the first place. It's not present in OGL back-end.

@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@Linktothepast

Linktothepast Oct 24, 2014

Contributor

Do you need to erase shader cache to test? While it seems better than before in the games affected i am getting texture errors everywhere.

Contributor

Linktothepast commented Oct 24, 2014

Do you need to erase shader cache to test? While it seems better than before in the games affected i am getting texture errors everywhere.

@Sonicadvance1

This comment has been minimized.

Show comment
Hide comment
@Sonicadvance1

Sonicadvance1 Oct 24, 2014

Contributor

Shader cache should automatically wipe as soon as you switch to a dolphin version that says a different git hash.

Contributor

Sonicadvance1 commented Oct 24, 2014

Shader cache should automatically wipe as soon as you switch to a dolphin version that says a different git hash.

@kayru

This comment has been minimized.

Show comment
Hide comment
@kayru

kayru Oct 24, 2014

Contributor

@Linktothepast what kind of texture errors?

Contributor

kayru commented Oct 24, 2014

@Linktothepast what kind of texture errors?

@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@Linktothepast

Linktothepast Oct 24, 2014

Contributor

Then i am just getting texture issues everywhere it seems :-( ...
Examples: http://prntscr.com/4zfjel
http://prntscr.com/4zfk22
etc.

Contributor

Linktothepast commented Oct 24, 2014

Then i am just getting texture issues everywhere it seems :-( ...
Examples: http://prntscr.com/4zfjel
http://prntscr.com/4zfk22
etc.

@kayru

This comment has been minimized.

Show comment
Hide comment
@kayru

kayru Oct 24, 2014

Contributor

Is per-pixel lighting enabled?

Contributor

kayru commented Oct 24, 2014

Is per-pixel lighting enabled?

@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@Linktothepast

Linktothepast Oct 24, 2014

Contributor

Oh, per pixel lighting seems to be the cause and it is broken, my bad.

Contributor

Linktothepast commented Oct 24, 2014

Oh, per pixel lighting seems to be the cause and it is broken, my bad.

@kayru

This comment has been minimized.

Show comment
Hide comment
@kayru

kayru Oct 24, 2014

Contributor

Yes, looks like PPL is broken in current master :(

Contributor

kayru commented Oct 24, 2014

Yes, looks like PPL is broken in current master :(

@@ -402,7 +402,7 @@ static inline void GenerateVertexShader(T& out, u32 components, API_TYPE api_typ
//if not early z culling will improve speed
if (api_type == API_D3D)
{
out.Write("o.pos.z = " I_DEPTHPARAMS".x * o.pos.w + o.pos.z * " I_DEPTHPARAMS".y;\n");

This comment has been minimized.

@degasus

degasus Oct 24, 2014

Member

Are I_DEPTHPARAMS.xy used anymore at all? If not, do you also want to remove this consts?

@degasus

degasus Oct 24, 2014

Member

Are I_DEPTHPARAMS.xy used anymore at all? If not, do you also want to remove this consts?

This comment has been minimized.

@kayru

kayru Oct 24, 2014

Contributor

Looks like they are indeed unused. Can remove, sure.

@kayru

kayru Oct 24, 2014

Contributor

Looks like they are indeed unused. Can remove, sure.

@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@Linktothepast

Linktothepast Oct 24, 2014

Contributor

Anyway, back on topic, this pr seems to fix https://code.google.com/p/dolphin-emu/issues/detail?id=7785. That was plaguing dolphin for years with no fix so congrats!

Contributor

Linktothepast commented Oct 24, 2014

Anyway, back on topic, this pr seems to fix https://code.google.com/p/dolphin-emu/issues/detail?id=7785. That was plaguing dolphin for years with no fix so congrats!

@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@Linktothepast

Linktothepast Oct 25, 2014

Contributor

I have found an issue with Mario Sports Mix pal http://prntscr.com/4zkhsv. The bottom of the screen is cut with d3d while opengl is fine.

Contributor

Linktothepast commented Oct 25, 2014

I have found an issue with Mario Sports Mix pal http://prntscr.com/4zkhsv. The bottom of the screen is cut with d3d while opengl is fine.

@kayru

This comment has been minimized.

Show comment
Hide comment
@kayru

kayru Oct 25, 2014

Contributor

@Linktothepast I will investigate. Thanks!

Contributor

kayru commented Oct 25, 2014

@Linktothepast I will investigate. Thanks!

@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@Linktothepast

Linktothepast Oct 25, 2014

Contributor

Here is an opengl pic http://prntscr.com/4zkiqm. If you need any fifo log just say it.

Contributor

Linktothepast commented Oct 25, 2014

Here is an opengl pic http://prntscr.com/4zkiqm. If you need any fifo log just say it.

@ZephyrSurfer

This comment has been minimized.

Show comment
Hide comment
@ZephyrSurfer

ZephyrSurfer Oct 26, 2014

Contributor

/offtopic:
Is there anywhere I can PM you @kayru ? Like join the dolphin forum or something.
I don't have twitter.

Edit: Sent

Contributor

ZephyrSurfer commented Oct 26, 2014

/offtopic:
Is there anywhere I can PM you @kayru ? Like join the dolphin forum or something.
I don't have twitter.

Edit: Sent

@kayru

This comment has been minimized.

Show comment
Hide comment
@kayru

kayru Oct 26, 2014

Contributor

@ZephyrSurfer you can just email me.

Contributor

kayru commented Oct 26, 2014

@ZephyrSurfer you can just email me.

@kayru

This comment has been minimized.

Show comment
Hide comment
@kayru

kayru Oct 27, 2014

Contributor

@Linktothepast I've tested Mario Sports Mix and it renders the menu correctly. Though this was Amarican version, not PAL.

Contributor

kayru commented Oct 27, 2014

@Linktothepast I've tested Mario Sports Mix and it renders the menu correctly. Though this was Amarican version, not PAL.

@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@Linktothepast

Linktothepast Oct 27, 2014

Contributor

It's probably pal issue then, do you want a fifo log?

Contributor

Linktothepast commented Oct 27, 2014

It's probably pal issue then, do you want a fifo log?

@ZephyrSurfer

This comment has been minimized.

Show comment
Hide comment
@ZephyrSurfer

ZephyrSurfer Oct 27, 2014

Contributor

@Linktothepast I think you should link him one.

Contributor

ZephyrSurfer commented Oct 27, 2014

@Linktothepast I think you should link him one.

@kayru

This comment has been minimized.

Show comment
Hide comment
@kayru

kayru Oct 27, 2014

Contributor

Yeah, fifo would be good.

Contributor

kayru commented Oct 27, 2014

Yeah, fifo would be good.

@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@JMC47

This comment has been minimized.

Show comment
Hide comment
@JMC47

JMC47 Oct 27, 2014

Contributor

Everything seems good here, even though I only have one of the games afflicted by the bug.

Contributor

JMC47 commented Oct 27, 2014

Everything seems good here, even though I only have one of the games afflicted by the bug.

@JMC47

This comment has been minimized.

Show comment
Hide comment
@JMC47

JMC47 Nov 4, 2014

Contributor

Any update on this? It makes OpenGL and D3D nearly identical in behavior.

Contributor

JMC47 commented Nov 4, 2014

Any update on this? It makes OpenGL and D3D nearly identical in behavior.

@kayru

This comment has been minimized.

Show comment
Hide comment
@kayru

kayru Nov 4, 2014

Contributor

The supplied log doesn't render anything on both OGL and D3D in master and in this branch :(

But I believe FIFO playback is just generally broken right now, not just this log.

Contributor

kayru commented Nov 4, 2014

The supplied log doesn't render anything on both OGL and D3D in master and in this branch :(

But I believe FIFO playback is just generally broken right now, not just this log.

@ZephyrSurfer

This comment has been minimized.

Show comment
Hide comment
@ZephyrSurfer

ZephyrSurfer Nov 4, 2014

Contributor

That fifo works here on master. ??
You've disabled dual core?

Edit: Ok only on OpenGL

Edit 2: Scratch that I reset my settings. I works here on D3D as well...

Edit 3: :D yeah, we must have changed some settings :p

Contributor

ZephyrSurfer commented Nov 4, 2014

That fifo works here on master. ??
You've disabled dual core?

Edit: Ok only on OpenGL

Edit 2: Scratch that I reset my settings. I works here on D3D as well...

Edit 3: :D yeah, we must have changed some settings :p

@kayru

This comment has been minimized.

Show comment
Hide comment
@kayru

kayru Nov 4, 2014

Contributor

What did you reset exactly?

Update: ah, deleted gfx_dx11.ini and now I can run FIFOs again!

Contributor

kayru commented Nov 4, 2014

What did you reset exactly?

Update: ah, deleted gfx_dx11.ini and now I can run FIFOs again!

@kayru

This comment has been minimized.

Show comment
Hide comment
@kayru

kayru Nov 4, 2014

Contributor

The good news is that I can now definitely reproduce the problem :)

Contributor

kayru commented Nov 4, 2014

The good news is that I can now definitely reproduce the problem :)

@kayru

This comment has been minimized.

Show comment
Hide comment
@kayru

kayru Nov 5, 2014

Contributor

Found the culprit. Viewport min/max depth must be in range [0..1], as per http://msdn.microsoft.com/en-us/library/windows/desktop/ff476260(v=vs.85).aspx. The FIFO has viewport min/max depth as -1000 and 1000. D3D simply ignores the RSSetViewports call in this case. It is caught correctly by the D3D debug validation layer.

D3D debug layer also specifies that min depth must be <= max depth, so things might still be broken in some cases. Are there games that actually do this?

Contributor

kayru commented Nov 5, 2014

Found the culprit. Viewport min/max depth must be in range [0..1], as per http://msdn.microsoft.com/en-us/library/windows/desktop/ff476260(v=vs.85).aspx. The FIFO has viewport min/max depth as -1000 and 1000. D3D simply ignores the RSSetViewports call in this case. It is caught correctly by the D3D debug validation layer.

D3D debug layer also specifies that min depth must be <= max depth, so things might still be broken in some cases. Are there games that actually do this?

@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@Linktothepast

Linktothepast Nov 5, 2014

Contributor

I tried various pal games and everything seems ok here, didn't notice any problems somewhere. It LGTM.

Contributor

Linktothepast commented Nov 5, 2014

I tried various pal games and everything seems ok here, didn't notice any problems somewhere. It LGTM.

@ZephyrSurfer

This comment has been minimized.

Show comment
Hide comment
@ZephyrSurfer

ZephyrSurfer Nov 5, 2014

Contributor

Maybe a 2D game could do something like that?
I haven't noticed any other problems either.

Contributor

ZephyrSurfer commented Nov 5, 2014

Maybe a 2D game could do something like that?
I haven't noticed any other problems either.

@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@Linktothepast

Linktothepast Nov 21, 2014

Contributor

Bump, any people to review this pr for merge? Testing it seems fine.

Contributor

Linktothepast commented Nov 21, 2014

Bump, any people to review this pr for merge? Testing it seems fine.

@JMC47

This comment has been minimized.

Show comment
Hide comment
@JMC47

JMC47 Nov 21, 2014

Contributor

I've had no problems with it either; it's way better than the current situation in Master as well.

Contributor

JMC47 commented Nov 21, 2014

I've had no problems with it either; it's way better than the current situation in Master as well.

@cyberkitsune

This comment has been minimized.

Show comment
Hide comment
@cyberkitsune

cyberkitsune Nov 23, 2014

Can confirm that this fixes my issue as well, definitely fixes Custom Robo's UI. Haven't noticed any other problems with it.

cyberkitsune commented Nov 23, 2014

Can confirm that this fixes my issue as well, definitely fixes Custom Robo's UI. Haven't noticed any other problems with it.

@Tinob

This comment has been minimized.

Show comment
Hide comment
@Tinob

Tinob Nov 25, 2014

Contributor

I'm the one who commited the code that is now in master, the reazon for the "Misterius" comment was that some games use inverted min and max values and some of them, alien hominid for example make use of values outside the valid range for dx.

Contributor

Tinob commented Nov 25, 2014

I'm the one who commited the code that is now in master, the reazon for the "Misterius" comment was that some games use inverted min and max values and some of them, alien hominid for example make use of values outside the valid range for dx.

@Buddybenj

This comment has been minimized.

Show comment
Hide comment
@Buddybenj

Buddybenj Nov 29, 2014

Contributor

Merge?

Contributor

Buddybenj commented Nov 29, 2014

Merge?

@lioncash

This comment has been minimized.

Show comment
Hide comment
@lioncash

lioncash Nov 29, 2014

Member

Needs a rebase

Member

lioncash commented Nov 29, 2014

Needs a rebase

kayru added some commits Oct 24, 2014

D3D: Removed somewhat mysterious comment
It would be good to know which games exactly exhibited the issue.
Renamed DEPTHPARAMS to PIXELCENTERCORRECTION
This shader constant was previously used for depth remapping in D3D and for pixel center correction. Now it only serves one purpose and the new name makes it clear.
@ZephyrSurfer

This comment has been minimized.

Show comment
Hide comment
@ZephyrSurfer

ZephyrSurfer Dec 3, 2014

Contributor

Sounds like Alien Hominid is the 2D game that needs a test with this PR.
I don't have it though.

Perhaps it's broken here though with Kayru comment that there may still be an issue.

Contributor

ZephyrSurfer commented Dec 3, 2014

Sounds like Alien Hominid is the 2D game that needs a test with this PR.
I don't have it though.

Perhaps it's broken here though with Kayru comment that there may still be an issue.

@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@Linktothepast

Linktothepast Dec 3, 2014

Contributor

Testing wise ZephyrSurfer there is no known issue with this, it also fixes current issues in master, it's up to the devs.

Contributor

Linktothepast commented Dec 3, 2014

Testing wise ZephyrSurfer there is no known issue with this, it also fixes current issues in master, it's up to the devs.

lioncash added a commit that referenced this pull request Dec 3, 2014

Merge pull request #1392 from kayru/d3d_viewport_depth
D3D: Replaced shader-based depth range remap with viewport

@lioncash lioncash merged commit 88cd27b into dolphin-emu:master Dec 3, 2014

1 check passed

default Build succeeded on the Buildbot.
Details
@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@Linktothepast

Linktothepast Dec 8, 2014

Contributor

Damn, found one game with issues, original report in the forums: https://forums.dolphin-emu.org/Thread-wii-dragon-ball-z-budokai-tenkaichi-3. It breaks ingame graphics http://prntscr.com/5eihkx , results remind me a bit of what happened with projection epsilon original merge.

Contributor

Linktothepast commented Dec 8, 2014

Damn, found one game with issues, original report in the forums: https://forums.dolphin-emu.org/Thread-wii-dragon-ball-z-budokai-tenkaichi-3. It breaks ingame graphics http://prntscr.com/5eihkx , results remind me a bit of what happened with projection epsilon original merge.

@ZephyrSurfer

This comment has been minimized.

Show comment
Hide comment
@ZephyrSurfer

ZephyrSurfer Dec 8, 2014

Contributor

Can you test the second last commit @Linktothepast
ie. With the last commit reverted?

Contributor

ZephyrSurfer commented Dec 8, 2014

Can you test the second last commit @Linktothepast
ie. With the last commit reverted?

@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@Linktothepast

Linktothepast Dec 8, 2014

Contributor

Nope, can't build anything here for a while now, i have local building issues.

Contributor

Linktothepast commented Dec 8, 2014

Nope, can't build anything here for a while now, i have local building issues.

@ZephyrSurfer

This comment has been minimized.

Show comment
Hide comment
@ZephyrSurfer

ZephyrSurfer Dec 8, 2014

Contributor

@Linktothepast
I'll send a compile to the forum when I get back to my home PC then.

Contributor

ZephyrSurfer commented Dec 8, 2014

@Linktothepast
I'll send a compile to the forum when I get back to my home PC then.

@ZephyrSurfer

This comment has been minimized.

Show comment
Hide comment
@ZephyrSurfer

ZephyrSurfer Dec 8, 2014

Contributor

@Linktothepast
Try this then: http://s000.tinyupload.com/?file_id=08108239456399687474

It's commit 3 here. I compiled myself.

Contributor

ZephyrSurfer commented Dec 8, 2014

@Linktothepast
Try this then: http://s000.tinyupload.com/?file_id=08108239456399687474

It's commit 3 here. I compiled myself.

@kayru

This comment has been minimized.

Show comment
Hide comment
@kayru

kayru Dec 8, 2014

Contributor

@Linktothepast do you have a FIFO?

Contributor

kayru commented Dec 8, 2014

@Linktothepast do you have a FIFO?

@Linktothepast

This comment has been minimized.

Show comment
Hide comment
@Linktothepast

Linktothepast Dec 8, 2014

Contributor

@ZephyrSurfer tried the build but no dice, the result is the same.
@kayru here is a fifo http://www.mediafire.com/download/8ag9ta4dpe583i5/Dragon_Ball_Z_fifo.7z

Contributor

Linktothepast commented Dec 8, 2014

@ZephyrSurfer tried the build but no dice, the result is the same.
@kayru here is a fifo http://www.mediafire.com/download/8ag9ta4dpe583i5/Dragon_Ball_Z_fifo.7z

@kayru kayru deleted the kayru:d3d_viewport_depth branch Dec 11, 2014

mrgreywater added a commit to mrgreywater/dolphin that referenced this pull request Feb 20, 2015

mrgreywater added a commit to mrgreywater/dolphin that referenced this pull request Feb 20, 2015

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