Project

General

Profile

Actions

Emulator Issues #5056

closed

Zelda SS: (EFB to texture) Improperly calculated DOF? results in banding on any surface.

Added by nop_jne over 12 years ago.

Status:
Fixed
Priority:
Normal
Assignee:
Category:
GFX
% Done:

0%

Operating system:
N/A
Issue type:
Bug
Milestone:
Regression:
No
Relates to usability:
No
Relates to performance:
No
Easy:
No
Relates to maintainability:
No
Regression start:
Fixed in:

Description

1) The Legend Of Zelda Skyward Sword SOUE01

2) After reading a lot of posts and guides on how to setup zelda to work on dolphin, I am not able to run it with FEB to texture. Running with EFB to ram eliminates problem.

I have captured a frame with pix and found the texture that is used as input for a PS to generate the DOF (I assume) effect.

3) On previous versions of the emulator the DOF effect seems to have a "Peter Panning" effect. And this problem does therefore not show up.

4) To reproduce get in game and get to the Skyloft area, this banding how ever is already noticeable in links room.
This problem reproduces on all of the Video Plugins, OGL, Dx9 Dx11. When emulating EFB through texture.

5) D3.0 204-dirty

6)
Stage before the effect texture is calculated:
http://min.us/mGZA7z9JW

The effect texture itself:
http://min.us/mvTM8thNy

The next SetRenderTargetState after the effects texture has been applied over the rendered geometry:
http://min.us/mbaU3wwXDL

[System info]
Core i7 - 3.07ghz
12Gb Ram
Geforce GTX 480

I think this issue is similar to issue:
http://code.google.com/p/dolphin-emu/issues/detail?id=4989

But I felt it did not give enough context on the problem.

Actions #1

Updated by nop_jne over 12 years ago

I messed up the links in the initial issue: last and second link are supposed to be fliped.

Actions #2

Updated by delroth over 12 years ago

Can you maybe post the effect texture with Texture to RAM so we can see how different it is?

Actions #3

Updated by nop_jne over 12 years ago

Yes definitely, here are a few things I noticed.
The texture in question is definitely supposed to be the Depth texture used for the DOF effect. It is however improperly computed because it seems to have extra white vertical lines in it for every other line which is correctly computed.

The EFB to ram seems to almost correctly generate this texture (except for the white lines). So there is a deeper issue here.

The reason this texture is less noticeable with EFB to ram is because the depth is actually in the texture opposed to strange ramp patterns seen in the EFB to texture.

You will also notice the source textures being smaller than the EFB to texture.

Stage before DOF:
http://minus.com/m4IjLPzKF#1

Depth texture itself:
http://minus.com/m4IjLPzKF#2

First SetRenderTarget after DOF:
http://minus.com/m4IjLPzKF#3

Actions #4

Updated by delroth over 12 years ago

Is that with native IR resolution or x2? That's definitely a strange bug, maybe the EFB to RAM encoder is broken.

Actions #5

Updated by nop_jne over 12 years ago

This capture was made with 2x native IR.

Actions #6

Updated by delroth over 12 years ago

Can you reproduce these vertical lines with 1x native IR?

Actions #7

Updated by nop_jne over 12 years ago

It does reproduce even with the native resolution. However the captures I've made so far were made with the DX9 plugin.

On DX11 I do see the same ramp pattern over my screen when using EFB to Texture hacks.
But it does not seem to exhibit the same vertical line behavior as the DX9 plugin when using EFB to ram.

I will take captures from DX11 later today.

Actions #8

Updated by kamm.christian over 12 years ago

This patch removed the banding from EFB to Texture for me (linux, Open GL):

diff --git a/Source/Core/VideoCommon/Src/TextureCacheBase.cpp b/Source/Core/VideoCommon/Src/TextureCacheBase.cpp
index d3b7ca1..fb1a8f6 100644
--- a/Source/Core/VideoCommon/Src/TextureCacheBase.cpp
+++ b/Source/Core/VideoCommon/Src/TextureCacheBase.cpp
@@ -473,7 +473,7 @@ void TextureCache::CopyRenderTargetToTexture(u32 dstAddr, unsigned int dstFormat
break;

            case 12: // Z16L
  • colmat[2] = colmat[6] = colmat[10] = colmat[13] = 1.0f;
  • colmat[3] = colmat[7] = colmat[11] = colmat[14] = 1.0f; cbufid = 6; break;
Actions #9

Updated by Anonymous over 12 years ago

Testing patch now, will report changes on dx11

Actions #10

Updated by kostamarino over 12 years ago

While i don't have a clue what kamm.christian changed he certainly seems to have fixed the issue for all the video backends with efb to texture. Below is a proper patch file for anyone to test with the latest dolphin version.

Actions #11

Updated by kostamarino over 12 years ago

And a link to a x64 build for further testing:
http://www.mediafire.com/?aagf5fjkhdw5a3u

Actions #12

Updated by Anonymous over 12 years ago

Lines removed on Dx11 :)

Actions #13

Updated by delroth over 12 years ago

@kamm.christian: could you explain a bit why your patch fixes that? I checked the color matrix values and can't find out why the previous values are wrong.

Actions #14

Updated by nop_jne over 12 years ago

kamm.christian, could you give more context on what you've done?

I took a look at the Shader that gets generated to consume these parameters, but have a feel the new values just generate a white depth texture. (far away texture)
I say this because the scene gets over blurred on DX11. You can see a difference between the EFB to RAM and TEXTURE very clearly. And you need to be hugging objects before the blur goes away. (I was not able to get PIX to work with the DX11 stream)

DX9 does not seem to have any DOF effect anymore.

Could you give a small breakdown of what your values are supposed to do?

Thanks

Actions #15

Updated by kamm.christian over 12 years ago

Given the observations reported here the patch is likely to be incorrect.

I saw that the PIXELFMT_Z24 to Z16L conversion was used for the depth texture and started looking at whether something might be wrong with the colmat values that I could fix without reading the surrounding code. I noticed that the indices for Z8M are +1 from Z8 and Z8L are +2 from Z8. So I tried making Z16L +2 from Z16.

Since that produces a depth effect I didn't look further. I haven't read up on how the shader that accepts the colmat values actually uses them.

Actions #16

Updated by kostamarino over 12 years ago

Just a small correction, the effect is not entirely broken with direct 3d9, it works indoors, it doesn't while outdoors.

Actions #17

Updated by bastos.eder over 12 years ago

I disagree... it is definitely at least kind of broken in D3D9.

Outside Earth Temple: http://img830.imageshack.us/img830/1885/soue011.png
Inside Earth Temple: http://img713.imageshack.us/img713/6236/soue012.png

Outside there is no DOF at all. Inside there is far too much.

Compare with 7719 D3D11 + EFB Texture... look at the rug next to Link: http://img195.imageshack.us/img195/9601/soue016.png

Actions #18

Updated by kostamarino over 12 years ago

  • colmat[2] = colmat[6] = colmat[10] = colmat[13] = 1.0f;
  • colmat[3] = colmat[7] = colmat[11] = colmat[14] = 1.0f;

Ok i played a bit, changing stuff and rebuilding, and from the 4 values changed from above only the third is the one that matters when it comes to the banding issue (colmat 10 ->11). Unfortunately it is also the one that creates the problem with too much dof effect.

Actions #19

Updated by kostamarino over 12 years ago

Ho ho ho, success!
Instead of increasing the third value i decreased it to 9 and things work properly with direct 3d 11! Gotta test the rest...

colmat[2] = colmat[6] = colmat[9] = colmat[13] = 1.0f;

Actions #20

Updated by kostamarino over 12 years ago

It works fine with direct 3d 9 too, i think it is perfect now. Here is a build:
http://www.mediafire.com/?5ic6pfrorf4rwsk

Actions #21

Updated by kamm.christian over 12 years ago

I can verify that setting colmat[9] to 1 also gives me the same DOF effect as texcache-rewrite with OpenGL. A more consistent choice for colmat might be

colmat[1] = colmat[5] = colmat[9] = colmat[14] = 1.0f;

which gives me the same results. That way all the color channels use the same mid-frequency value while the alpha channel holds the high-freq one. So it'd be the same as the current setting - only reversed.

Actions #22

Updated by jpeterson57 over 12 years ago

good work!, the result speaks for itself. no more need for the texcache-rewrite build for zelda ss.

Actions #23

Updated by Nintendo.Wii.DS over 12 years ago

I don't want to sound pessimist, but doesn't this break other games ?

Anyway good job guys, hope it'll be committed soon :)

Actions #24

Updated by NeoBrainX over 12 years ago

  • Status changed from New to Accepted
  • Category set to gfx

fwiw, prolly shouldn't commit this until we actually have a clue what's going wrong and why.

Actions #25

Updated by delroth over 12 years ago

Yep, this smells like random code, and we all know how well that works.

As long as nobody can explain the colmat coeffs change correctly we can't commit that. For example, why are we duplicating a part of the depth value in r, g, and b? TCR does not do that as far as I know but packs two Z16L values into one output color.

Actions #26

Updated by kostamarino over 12 years ago

Delroth, compared to having people use old builds and complaining around for old issues that are actually fixed, i don't think it is such a bad idea trying your luck with this and see where it goes...

Actions #27

Updated by delroth over 12 years ago

I just checked the texcache-rewrite code about that (finally found it, VirtualEfbCopy.cpp) and it is indeed using GGGB like the values that kamm.christian is providing in comment 21.

However it's not documented and the comments in the TCR version explicitly say that it is untested, so I don't really know what to do about this. We probably need to code some samples to run on real hardware and compare with our results. For example, I still don't get why we should repeat the G value instead of using GB00.

Actions #28

Updated by delroth over 12 years ago

TCR is not even consistent about that btw.

DX11 does GB00 according to this:
http://code.google.com/p/dolphin-emu/source/browse/Source/Plugins/Plugin_VideoDX11/Src/VirtualEFBCopy.cpp?name=texcache-rewrite#246

DX9 and GL do GGGB according to this:
http://code.google.com/p/dolphin-emu/source/browse/Source/Plugins/Plugin_VideoDX9/Src/VirtualEFBCopy.cpp?name=texcache-rewrite#293

Are there any noticeable differences between DX9/DX11 with EFB to Texture on a texcache-rewrite enabled build? Can't test that here, I don't have Windows installed.

Actions #29

Updated by Anonymous over 12 years ago

Theres an issue with the values given in comment 21, while it looks good in almost everywhere there are afew areas where things do not look right, and the lines reappear.

Using Dx11 with comment 21:
http://i.imgur.com/XxWYj.jpg

Using Dx11 with texcache-rewrite:
http://i.imgur.com/YAqN2.jpg

Notice the border of the beam is much smoother on tcr, and is accurately represented. Going to test DX9 now.

Actions #30

Updated by Anonymous over 12 years ago

DX9 looks funky, but the edges of beam are normal in both versions:

Comment 21:
http://i.imgur.com/j9zAs.jpg

TCR:
http://i.imgur.com/Mlxy0.jpg

Actions #31

Updated by nop_jne over 12 years ago

Masterg.

Could you post screenshots of:
1: Latest version of Dolphin without patch.
2: Latest version with patch.
3: TCR.

Actions #32

Updated by Anonymous over 12 years ago

The first set of screenshots dx11 from 221 so lemme recompile without patch.

Actions #33

Updated by nop_jne over 12 years ago

Delroth now this is beginning to sound like a political issue :) However I do agree with you that without understanding exactly why the texture swizzling is broken for Z16L it should not be committed.

Therefore I suggest we discuss exactly what the Copy_depth_buffer shader does, and how it uses the colmatrix values.

Let us step through the implementation for D3D9 cause that's where I captured this from:

srcFormat = PIXELFMT_Z24
dstFormat = 12 // Some one could add enums for this, with names like Z16L, DEPTH_TEXTURE_16BIT?
colmat[2] = colmat[6] = colmat[10] = colmat[13] = 1.0f;
cbufid = 6;
scalebyhal = true // Because from the pix capture there is a call to SetSamplerState(0, D3DSAMP_MINFILTER, D3DTEXF_LINEAR) and I had msaa turned off.

void TextureCache::TCacheEntry::FromRenderTarget(u32 dstAddr, unsigned int dstFormat,
unsigned int srcFormat, const EFBRectangle& srcRect,
bool isIntensity, bool scaleByHalf, unsigned int cbufid,
const float *colmat)

We are going for the depth texture cause of srcFormat.
We will be calling GetDepthMatrixProgram(0, true);

The next few lines are a pix capture of the render to texture operation.
8298 IDirect3DDevice9::SetRenderState(D3DRS_COLORWRITEENABLE, D3DCOLORWRITEENABLE_RED | D3DCOLORWRITEENABLE_GREEN | D3DCOLORWRITEENABLE_BLUE | D3DCOLORWRITEENABLE_ALPHA) 138871953575

8299 IDirect3DTexture9::GetSurfaceLevel(0, 0x400292E8 --> 0x400293A8) 138871956940

8300 IDirect3DDevice9::SetDepthStencilSurface(NULL) 138871958622

8301 IDirect3DDevice9::SetRenderTarget(0x00000000, 0x400293A8) 138871961146

8302 IDirect3DDevice9::SetViewport(0x400292F0) 138871964511

8303 IDirect3DDevice9::SetPixelShaderConstantF(0, 0x40029230, 7) 138871966474

8304 IDirect3DDevice9::SetSamplerState(0, D3DSAMP_MINFILTER, D3DTEXF_LINEAR) 138871970119

8305 IDirect3DDevice9::SetVertexShader(0x400001B0) 138871973204

8309 IDirect3DDevice9::SetPixelShader(0x40000540) 138871976569

8310 IDirect3DDevice9::SetTexture(0, 0x40022B10) 138871982457

8311 IDirect3DDevice9::SetFVF(D3DFVF_XYZW | D3DFVF_TEX3) 138871984700

8312 IDirect3DDevice9::DrawPrimitiveUP(D3DPT_TRIANGLESTRIP, 2, 0x400292F8, 36) 138871986944 9429840

8313 IDirect3DDevice9::SetTexture(0, NULL) 138881516895

For my capture I wasn't using msaa, so this translates into the following shader:

const char depth_matrix_program[] = {
"sampler samp0 : register(s0);\n"
"Texture2D Tex0 : register(t0);\n"
"uniform float4 cColMatrix[7] : register(c0);\n"
"void main(\n"
"out float4 ocol0 : SV_Target,\n"
" in float4 pos : SV_Position,\n"
" in float2 uv0 : TEXCOORD0){\n"
"float4 texcol = Tex0.Sample(samp0,uv0);\n"
"float4 EncodedDepth = frac((texcol.r * (16777215.0f/16777216.0f)) * float4(1.0f,256.0f,256.0f*256.0f,1.0f));\n"
"texcol = round(EncodedDepth * (16777216.0f/16777215.0f) * float4(255.0f,255.0f,255.0f,15.0f)) / float4(255.0f,255.0f,255.0f,15.0f);\n"
"ocol0 = float4(dot(texcol,cColMatrix[0]),dot(texcol,cColMatrix[1]),dot(texcol,cColMatrix[2]),dot(texcol,cColMatrix[3])) + cColMatrix[4];\n"
"}\n"
};

(16777215.0f/16777216.0f) = 0.999999940395355224609375 // (nearing 1, I assume this is to hack around the float precision issues but this gets discarded by the shader compiler)
1.0f, 256.0f, 256.0f*256.0f, 1.0f, // This is a shader way of upshifting (left shifting) values. (<<0, <<8, <<16, <<0)
// float4(255.0f,255.0f,255.0f,15.0f)) / float4(255.0f,255.0f,255.0f,15.0f)
float4(dot(texcol,cColMatrix[0]),
dot(texcol,cColMatrix[1]),
dot(texcol,cColMatrix[2]),
dot(texcol,cColMatrix[3])) +
cColMatrix[4]; // This whole oneliner is a shader matrix multiplication.

Here is the compiled version

//
// Generated by Microsoft (R) HLSL Shader Compiler 9.29.952.3111
//
// Parameters:
//
// float4 cColMatrix[7];
// sampler2D samp0;
//
//
// Registers:
//
// Name Reg Size
// ------------ ----- ----
// cColMatrix c0 5
// samp0 s0 1
//

// init
ps_3_0
def c5, 256, 15, 0.00392156886, 0.0666666701
def c6, 0.249999985, 63.9999962, 16383.999, 0

// declarations
dcl_texcoord2 v0
dcl_texcoord3 v1
dcl_2d s0

// sampling
texld r0, v0, s0
texld r1, v0.wzzw, s0
add r0.x, r0.x, r1.x
texld r1, v1, s0
add r0.x, r0.x, r1.x
texld r1, v1.wzzw, s0
add r0.x, r0.x, r1.x

// encode depth
mul r0, r0.x, c6.xyzx // shift the data up.
frc r0, r0  
mul r0, r0, c5.xxxy 
frc r1, r0
add r0, r0, -r1
mul r0, r0, c5.zzzw

// matrix mask.
dp4 r1.x, r0, c0
dp4 r1.y, r0, c1
dp4 r1.z, r0, c2
dp4 r1.w, r0, c3
add oC0, r1, c4

I will dump the intermediate textures that get used for this shader and get back.

Actions #34

Updated by Anonymous over 12 years ago

Actions #35

Updated by Anonymous over 12 years ago

Dx9 looks spot on with comment 21, it is only with DX11 that an issue appears.

Actions #36

Updated by nop_jne over 12 years ago

Masterg.

Judging from your screens the patch does not break any old behavior.

I would the Blue lines are outside the scope of this issue. (Maybe you want to open a new issue?)

Actions #37

Updated by nop_jne over 12 years ago

The patch sofar looks pretty good to me, I've gotten hung up playing the actual game instead of debugging it :)

Here are a couple of other games I tested that do not seem to have been regressed by this change:
Zelda TP
Donkey Kong Returns
new Super Mario Bros.

Masterg, when I get to the same point you were at I'll try to debug that issue too. But I think it may be something temple specific, while this issue just bugged me to death.

On my todo list is to prove: that the old texture copy transform is broken.

Actions #38

Updated by WMRPoker over 12 years ago

There must be something wrong with the DOF in general and i hope this is the right place to post it.
Besides that i have the feeling, the effect is much smoother on the Wii compared to Dolphin with NATIVE 1x resolution, i noticed the following:
These Screenshots have been taken from the SS Intro. The scene on Dolphin is completely blurred, when the DOF should only be in the foreground.

Wii: http://i.imgur.com/SlRJL.jpg

Dolphin: http://i.imgur.com/xRTJ6.jpg

This issue occurs regardless what graphics plugin, settings, internal resolution or build you are using - even with the newest one posted on this page.
I hope you can figure out, what might be wrong.

Actions #39

Updated by kostamarino over 12 years ago

Delroth if what you say is true then there is an easy way to test for possible regressions, see the issue list that games have for texcache rewrite(Issue 4635) and testing them would be pretty sufficient. Personally i tested plenty of games and haven't found an issue.

Actions #40

Updated by kostamarino over 12 years ago

WMRPoker, that is one interesting observation that needs testing, did you use the build i posted or the values in comment 21 to test?

Actions #41

Updated by WMRPoker over 12 years ago

I tried the build you posted, the TCR and the WM+ emu ones John Peterson made - if i've got it right, he already implemented your code in the newest version (3.0-243).
Sidenote: I am no expert at all and wouldn't know how to use the values from #21.

Actions #42

Updated by kostamarino over 12 years ago

WMRPoker, it seems that the problem you talk about has also some relation with the bloom offset issue with dolphin. Whatever fixes the bloom offset will fix that too, but i think it is a different issue altogether...

Actions #43

Updated by kostamarino over 12 years ago

Ok i tried every game that had issues with texcache rewrite and even more and i couldn't spot any issues whatsoever caused by kamn.christian values:
colmat[1] = colmat[5] = colmat[9] = colmat[14] = 1.0f;
Imo it should be committed, it is very unlikely to cause any problems at all...

Actions #44

Updated by hatarumoroboshi over 12 years ago

The only way to definitely know if it causes problems is to commit it in master or in a separate branch (although it seems to me that very few people usually test branches).

Actions #45

Updated by NeoBrainX over 12 years ago

Or, you know, verify the new behavior against real HW or at least understand wtf is actually being done by replacing 10 with 9 ;)

Actions #46

Updated by Autoran1 over 12 years ago

Yep, you're right, ZSS might be fixed by replacing only third value 10 to 9, all others should be left by default to avoid additional problems with other games

Actions #47

Updated by WMRPoker over 12 years ago

OK, Kosta. Shall i delete my comments then?

Actions #48

Updated by kostamarino over 12 years ago

WMRPoker, there are a lot of literally irrelevant spam that don't get deleted around here, imo in your case there is no need, so it is your call if you want to or not.

Actions #49

Updated by NeoBrainX over 12 years ago

Okay, so let's drop the bomb...

As it seems right now the current efb2tex code is even more of an incredible hack than I thought before. Usually, EFB copies with a GX_CTF_* format can't directly be used as textures, but need to be loaded using a different format (GX_TF_*). Thus, it's not really determined how the copy target will "look" until the application has specified what format to use for the texture.
Now, the Dolphin implementation of efb2texture completely ignores this fact and just hardcodes a format for the efb copies, hoping that it's actually the format that will be used lateron. Obviously this causes lots of bugs and indeed given enough effort the efb2texture implementation could be made much more compatible (not perfect, but probably good enough to work with more games). However, that's not the whole story, yet:
In this particular case we have a Z16L target - and as far as I can see Dolphin hardcodes a texture format of IA8 (and that pretty badly btw). And I don't even know if that's the correct expectation in this case, but: We're most likely converting this stuff wrong.
Normal IA8 textures are represented internally using RGBA8 textures (in our D3D11 backend, that is) - the A channels are the same, the RGB and I channels have the same value. I.e. we'd have to store the upper 8 bits of the "lower 16 depth bits" as the RGB values (i.e. the same 8 bits three times) and the lower 8 bits of the "lower 16 depth bits" as the A values (not sure about the order, but you get the idea).
However, what we're doing right now is that we're storing the "lower 16 depth bits" as the A value and the "lower 24 [sic] depth bits" as the RGB bits. That's just plain wrong and partly a limitation of the depth matrix code and (most importantly) probably the cause of this issue.

This explanation isn't meant to be any useful for anyone who doesn't know the code, I just wanted to have it written down in case anyone comes around and wants to have a reasonable explanation of what's going on.

tl;dr: It's not simple to fix and whatever that colmat change is doing is just another hack on top of the existing bunch of other ones.

Actions #50

Updated by kostamarino over 12 years ago

Neobrain, if it helped to track the source of the issues it is still good imo, since the ending results of all this might be even better, not only for this game but for others as well. 2 questions though:

  1. Does that mean it needs a lot of work and rewriting of code?
  2. Do you have any "plan" let's say of what might be needed to fix it properly and are you willing-or have time to try to do it? Tnx man.
Actions #51

Updated by NeoBrainX over 12 years ago

1./2.: It's not a bad idea to hardcode stuff the way it was done, but a crapload of work will be needed to document the stuff (there's NO single mention anywhere what expectation is actually hardcoded). The proper fix would be an insane amount of work, but I guess it'll be enough to just implement the cases which are necessary to make games work. That'll take more time for testing, but shouldn't be too much work (given that anyone bothers to do it).

The amount of replaced code is probably small, it'll just add a bunch of new code... On top of our already crappy texcache code.

  1. Maybe, maybe not. Depends on how much I feel like wasting my time looking at the mess of code which is our texture cache :/
Actions #52

Updated by Nintendo.Wii.DS over 12 years ago

"This explanation isn't meant to be any useful for anyone who doesn't know the code"

It might not be useful, but it is really interesting.

Actions #53

Updated by nop_jne over 12 years ago

I call for comitting the hack, with a comment (and man are those scarse) and link to this issue.

//Hack: workaround for currently broken depthbuffer issue: http://code.google.com/p/dolphin-emu/issues/detail?id=5056

Actions #54

Updated by Nintendo.Wii.DS over 12 years ago

Make it a switchable hack, like projection hacks. Seems better than applying it to every games.

Actions #55

Updated by Anonymous over 12 years ago

That would take more work and bog down the performance unless implemented correctly. People often think "just add an option" is a good idea, but in software design it often not a good choice to make.

Actions #56

Updated by kostamarino over 12 years ago

Hmm, committing it for the time being would be nice for one reason (the same that i told delroth), it would prevent people of using and staying with an old and even more problematic build as a regular build. While i have commit rights though i am not really willing to do it guys, since i am mainly the ini database maintainer. As for the make it a toggling hack i don't think it is necessary, since it doesn't create any issues for other games and the logic behind of it is sound (texcache rewrite uses same values, yet the games reported to have issues with that branch don't have with this, nevermind other games were tested too with no issues).

Actions #57

Updated by NeoBrainX over 12 years ago

For the sake of not breaking other games until next release, I'm all against committing the hack. No one can test all games out there and say for sure that none of them regressed.

Actions #58

Updated by tommyhl2.SS over 12 years ago

kostamar: You can't just say commit it since it does not cause any issues, you don't know that.

Actions #59

Updated by nop_jne over 12 years ago

As I do not think the battle about commiting or not should be fought out in a bug report:

Neo: it looks to me like removing TCR broke a bunch of games. This "hack" reintroduces DX11 TCR behavior of grabbing the GGGB components of the depth texture. (as was stated in the constant definitions)

To me it seems that you are not introducing new behavior, but merely reverting to the old.

I do not have commit rights nor did I come up with the fix, but looking at the current implementation of Z16L and how it is handeled plus debugging this in pix. I cannot see how the current implementation is supposed to work anyway.

Neo I challange you to find a game that is currently working with the current implementation :)

Actions #60

Updated by NeoBrainX over 12 years ago

The current impl for Z16L is just broken altogether, but replacing it with something which is just a bad hack as well isn't much better.

Btw, could you check what format Zelda SS is using when that Z16L copy target is being applied as a texture?

Actions #61

Updated by delroth over 12 years ago

TCR has not been removed, it was never merged in the first place (if we ignore 2 or 3 weeks between 3.0 release and migration to Git) because it caused a lot of issues in a lot of games.

The current EFB to Texture is broken and trying to apply random fixes hoping it will solve problems won't work. Using TCR behavior for Z16L will likely break other games using Z16L copies.

BTW, if I understand things correctly GGGB is not correct for Z16L -> IA8 either: I should be the lower part of Z16 (B) and A should be the upper part (G), giving BBBG instead of GGGB. Also, DX11 TCR does not convert to IA8 and does not do GGGB (it does GB00, see comment 28.

+1 to no_cluez, I definitely won't commit such a hack without understanding the consequences it has.

Actions #62

Updated by kostamarino over 12 years ago

tommyhl2, i don't maintain the ini database for no reason you know ;-), i am among the most active testers of dolphin right now. I am 99.9% sure that it doesn't create any new issues. And my reason of wanting this to be committed as i said was this "It would prevent people of using and staying with an old and even more problematic build as a regular build".
Neobrain, you can commit it if you want and blame me if anything goes wrong for false information :-p.

Actions #63

Updated by nop_jne over 12 years ago

Btw, could you check what format Zelda SS is using when that Z16L copy target is being applied as a texture?

Do you mean the internal Emulator format, or the D3D texture format?

Actions #64

Updated by tommyhl2.SS over 12 years ago

kost: Not to get into a pissing match here, but to be 99% sure you would have to have tested every GC and Wii game known to exist with many different settings and scenarios. I highly doubt that you've done that in the last few minutes.

[SS]

Actions #65

Updated by kostamarino over 12 years ago

tommyhl2.SS
"http://code.google.com/p/dolphin-emu/source/detail?r=8d9061ac39ee8841557ce461c48fdc2791ec8a20"
....Dude try to be repetitive in a positive manner next time, works better in the end, you will see...

Actions #66

Updated by tommyhl2.SS over 12 years ago

Kost: Other than me being right in that issue and here, what does that issue have to do with this? Master is not the place to test half-cocked untested changes that are pure guesses(this is not SVN where you just commit stuff and cross your fingers, that's what a branch is for). Saying that you know for a fact that there aren't any games with issues because of this change is just plain ignorant and you know it. I'm with you in wanting Skyward to work well, but this is not the way.

Actions #67

Updated by kostamarino over 12 years ago

Tommy since you were right and i was wrong, do you want me to revert that change? I have no problem really, i will also thank you in the commit for making me see how wrong i was. I might even not try to help any more with dolphin, since according to you making mistakes while trying to help around here is like a habit of mine, and maybe leaving the project would be better for everyone. Personally i am in. What say you?

Actions #68

Updated by hatarumoroboshi over 12 years ago

There are still a lot of commits on master that break some things, and always will be.
I think that no one can be 100% sure of not breaking something even with the simplest commit on earth.
To be near 100% sure you need testing from a lot of users, but if you don't commit the changes you won't ever know if they really break anything.

Actions #69

Updated by NeoBrainX over 12 years ago

Again, it's sad that general consensus here seems to be "test stuff by committing it and potentially fucking up master!" rather than "wait until people who know what they're doing have figured out what's wrong and how to fix it".

Anyway, let's all calm down and don't take stuff personally. Remember, it's the internet and stuff >_>

Point is, this is unlikely to be committed in its current state. Also please refocus the discussion on the issue itself or I'll restrict commenting to project members.

Actions #70

Updated by NeoBrainX over 12 years ago

  • Status changed from Accepted to Work started

I just looked into this issue again and verified that the suggested patch indeed fixes the issue properly.

What happened was that the upper 8 depth bits were stored in the alpha channel and the lower depth bits were stored in the intensity channel, it needs to be the other way around though.
I'm going to commit this to master soonish, along with documentation of wtf's going on in the code..

Actions #71

Updated by NeoBrainX over 12 years ago

  • Status changed from Work started to Fixed

This issue was closed by revision 3d9c35f58e84.

Actions #72

Updated by nitsuja- over 12 years ago

This issue was closed by revision 3d9c35f58e84.

Actions #73

Updated by nitsuja- over 12 years ago

This issue was closed by revision 3d9c35f58e84.

Actions #74

Updated by lpfaint99 over 12 years ago

This issue was closed by revision 3d9c35f58e84.

Actions

Also available in: Atom PDF