search

bo3b / 3Dmigoto

Chiri's DX11 wrapper to enable fixing broken stereoscopic effects.
Other
758 stars 118 forks source link

VS2015, VS2017 & VS2019 Port Status #92

Open DarkStarSword opened 6 years ago

DarkStarSword commented 6 years ago

VS2015:

VS2017:

VS2019:

Nintynuts is joining the 3DMigoto dev team and investigating this.

bo3b commented 6 years ago

Weird. Not the component I would predict to go south from the conversion. I took a quick look at the crc32 code, and it's really pretty straightforward and simple. I would not expect any C++ compiler difference to change this code.

Looking at the function going wrong here, this would be the spot I might expect to break with a change to a new C++ toolchain, which includes a new version of C++. In that CommandList piece, the template is used as the source item, and it would not be too shocking for C++ to put a kink in this usage on an upgrade. Probably unfounded, but it is my impression that templates are a mess in C++.

bo3b commented 6 years ago

I ran a quick test with 2017 version, to step through the code to the crc32c function. Just a small datapoint- it is calling the correct code, and using the CPU instruction version. I wanted to be sure that __cpuid didn't change in some bizarre way, or that it went to the fallback table based path.

DarkStarSword commented 6 years ago

crc32 bug (actually, resource pool bug) resolved: https://github.com/bo3b/3Dmigoto/commit/43f9c201542bb8ba88468eb5c16a8c322da661f9

This was very much a matter of "how did this ever work?", but it turns out that the previous tool chains laid out the stack such that what we wanted to hash was in the area we were hashing, and the other bytes were always consistent, while vs2017 lays out the stack differently.

DarkStarSword commented 6 years ago

Moved the resource pool / crc32 issue out of the top post since it is now resolved and to to keep the top post as the current status. The original report was:

Creating cached resource [customshaderautoconvergence] resourceautoconvergencedepthdownscale128 = copy_desc resourcedepthbuffer
  NOTICE: cache now contains 20 resources (new hash: 0xd86d0c0b, sizeof(desc): 44)
  Resource Type = Texture2D
    Width = 128
    Height = 128
    MipLevels = 1
    ArraySize = 1
    Format = R32_FLOAT (41)
    SampleDesc.Count = 1
    SampleDesc.Quality = 0
    Usage = 0
    BindFlags = 0x28
    CPUAccessFlags = 0x0
    MiscFlags = 0x0
80 00 00 00 80 00 00 00 01 00 00 00 01 00 00 00 29 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00 28 00 00 00 00 00 00 00 00 00 00 00
Creating cached resource [customshaderautoconvergence] resourceautoconvergencedepthdownscale128 = copy_desc resourcedepthbuffer
  NOTICE: cache now contains 21 resources (new hash: 0x67421c58, sizeof(desc): 44)
  Resource Type = Texture2D
    Width = 128
    Height = 128
    MipLevels = 1
    ArraySize = 1
    Format = R32_FLOAT (41)
    SampleDesc.Count = 1
    SampleDesc.Quality = 0
    Usage = 0
    BindFlags = 0x28
    CPUAccessFlags = 0x0
    MiscFlags = 0x0
80 00 00 00 80 00 00 00 01 00 00 00 01 00 00 00 29 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00 28 00 00 00 00 00 00 00 00 00 00 00
DarkStarSword commented 6 years ago

We potentially have a bit of an issue with ShaderRegex - any patterns matching the resource bind lines may be tripped up by the syntax changes there ("0" becomes "cb0", etc). The capitalisation of "cb" there and in the declarations section shouldn't be an issue - I intentionally made ShaderRegex case insensitive.

Might be worthwhile having a migration option to alter the syntax of the resource binds section to match the old syntax before running ShaderRegex.

Edit: This is done

DarkStarSword commented 5 years ago

Now that we have a regression test suite for the HLSL decompiler a new issue has been uncovered in the vs2017 branch Edit: the bug is actually in master, and the vs2017 toolchain happened to uncover it by sheer dumb luck.

FC4/9cc9ea03feed4639-vs.hlsl.chk
-  r3.xyzw = (int4)r2.xyzw & int4(0,0,0,0);
+  r3.xyzw = (int4)r2.xyzw & int4(65535,65535,65535,65535);

JC3/f064c0539aaa5254-vs.hlsl.chk
-  r2.xyz = (int3)r0.xzw & int3(0,0,255);
+  r2.xyz = (int3)r0.xzw & int3(65535,65535,255);

MirrorsEdge/00fe11cf0e780fea-cs.hlsl.chk
-  r1.yzw = (int3)r0.xxx & int3(0,0,0);
+  r1.yzw = (int3)r0.xxx & int3(65535,65535,65535);

MirrorsEdge/53a20a1570726154-cs.hlsl.chk
-  r1.yzw = (int3)r0.xxx & int3(0,0,0);
+  r1.yzw = (int3)r0.xxx & int3(65535,65535,65535);

MirrorsEdge/798abdcf8eeed9df-cs.hlsl.chk
-  r1.yzw = (int3)r0.xxx & int3(0,0,0);
+  r1.yzw = (int3)r0.xxx & int3(65535,65535,65535);
...
-    r1.yz = (int2)r1.yz & int2(0,0);
+    r1.yz = (int2)r1.yz & int2(65535,65535);

MirrorsEdge/cdc7497832359d0d-cs.hlsl.chk
-  r1.yzw = (int3)r0.xxx & int3(0,0,0);
+  r1.yzw = (int3)r0.xxx & int3(65535,65535,65535);

MirrorsEdge/e6db2e1b78527470-cs.hlsl.chk
-  r1.yzw = (int3)r0.xxx & int3(0,0,0);
+  r1.yzw = (int3)r0.xxx & int3(65535,65535,65535);

Prey/24702f38f517e171-ps.hlsl.chk
-    r1.xyz = (int3)r0.yzw & int3(0,0,0);
-    r1.xyz = cmp((int3)r1.xyz != int3(0,0,0));
+    r1.xyz = (int3)r0.yzw & int3(2139095040,2139095040,2139095040);
+    r1.xyz = cmp((int3)r1.xyz != int3(2139095040,2139095040,2139095040));

The remaining changes were all benign that I could see - 2 digits for the exponent in scientific notation instead of 3 and some whitespace trimmed at the end of some lines.

We will probably want to use this regression testing (perhaps tailored to compare the versions and ignore the known benign changes) to do some more extensive testing across a wider range of games.

DarkStarSword commented 5 years ago

This issue actually appears to be a bug in the master branch that is mysteriously fixed in the vs2017 branch. Assembly:

and r3.xyzw, r2.xyzw, l(0x0000ffff, 0x0000ffff, 0x0000ffff, 0x0000ffff)

Decompiled on master:

r3.xyzw = (int4)r2.xyzw & int4(0,0,0,0);

Decompiled on vs2017:

r3.xyzw = (int4)r2.xyzw & int4(65535,65535,65535,65535);
DarkStarSword commented 5 years ago

Said bug is now fixed in master, preserving the hex literals, and this brings the vs2017 branch into parity with master as far as the HLSL regression tests are concerned.

DarkStarSword commented 5 years ago

New issue uncovered: The HLSL decompiler outputs "inf" on the vs2017 branch instead of "1.#INF". I personally prefer "inf", but we need to use what the compiler can parse. There are likely similar cases for NAN variants.

usuarionuevor commented 5 years ago

VS2019 when

DarkStarSword commented 5 years ago

Not for some time - there are a lot of build failures on vs2019 at present that will need to be resolved (typically these type of fixes can be incorporated into master provided they don't harm building on previous toolchain versions), but vs2019 will need extensive testing to ensure that any other weird bugs that crop up (and there will be) are squashed before we can even begin to consider switching master to it.

That said, if you have a vested interest in making 3DMigoto build on vs2019, please feel free to send pull requests with any fixes.

DarkStarSword commented 4 years ago

With Nintynuts looking to join the project I have finally flipped the switch and made vs2017 master. If there's any fallout we will deal with it as we would any other bug.

It may also worth looking at vs2019.