search

icculus / mojoshader

Use Direct3D shaders with other 3D rendering APIs.
https://icculus.org/mojoshader/
zlib License
152 stars 37 forks source link

SPIR-V Testing Thread #15

Closed flibitijibibo closed 4 years ago

flibitijibibo commented 5 years ago

Post SPIR-V test results here. Use this archive to test. Some games may be super old and will not work even with stock FNA releases, don't expect much from games that haven't been updated in 4+ years. Repo containing SPIR-V changes is here.

Environment variables of interest:

Template/Example Report:

OS: Fedora 30 x86_64 Graphics: NVIDIA GTX 770, version 430.26 Games Tested:

Breakages (with logs):

Platinum:

Gold:

Known not to work:

Untested:

flibitijibibo commented 5 years ago

I forget where I saw it, but I believe it goes all the way up to index 9 for SM3, hence this unpleasant variable:

https://github.com/FNA-XNA/MojoShader/blob/fna/mojoshader_opengl.c#L104

But yeah, this will add another step to the link process for sure :/

krolli commented 5 years ago

Infrastructure and patching step are already there, but so far I thought we might be able to ditch it with UBOs. This won't work for attributes though. I wanted to get rid of it mostly because it forces the same shader to be compiled multiple times because of these mappings. :(

krolli commented 5 years ago

Hmm, I think that vertex_attrib_loc length is something else. Also, it might be wrong. :) It looks like the number of input registers (which has little to do with semantics). As far as I can tell those numbers are: 16 vertex inputs (v#) for vs_3_0 12 vertex outputs (o#) for vs_3_0 10 pixel inputs (v#) for ps_3_0 https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx9-graphics-reference-asm-vs-registers-vs-3-0#input-registers https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx9-graphics-reference-asm-ps-registers-ps-3-0#input-register-types

However, this doesn't say anything about semantics. Those come from HLSL reference and for linking vertex shader outputs to pixel shader inputs, we have to look at pixel shader input semantics: https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-semantics#pixel-shader-semantics And now that I look at it, its interesting that they don't mention NORMAL as pixel shader input semantic, yet Murder Miners use that for connecting vertex shader output to pixel shader input. I remember reading somewhere that in DX10 or DX11, you can use any text for naming semantics between VS and PS, but I don't know how that would map to DXBC. Anyway, indices for semantics can apparently go up to n, but I couldn't find any info on what n is guaranteed to be.

n is an optional integer between 0 and the number of resources supported.

krolli commented 5 years ago

Pushed a bunch of fixes:

Point size thing should help Flotilla, while NRM and POW have fixed specular reflections on Simply Chess when I tested it. Unfortunately, I only have Psychonauts shader for testing point size and the way it is declared is different in newer shader models, so it might actually emit invalid spirv. I wrote something that might work, but I'm afraid register info might be incomplete on attributes everywhere except during attribute emit.

TheSpydog commented 5 years ago

With those fixes, Apotheon now displays correctly! Simply Chess also saw an improvement on my intel machine, although it's still totally texture-less:

latest_chess
flibitijibibo commented 5 years ago

With the latest revision it looks like Flotilla's still busted - thankfully the shader is very simple (before the FNA update the game rendered each PointSize individually, wrote this to allow for proper batching):

float4x4 WorldViewProj;

float4 VSBasicNoFog(
    float4 Position : SV_Position,
    inout float4 DiffuseColor : COLOR0,
    inout float PointSize : PSIZE0
) : SV_Position {
    return mul(Position, WorldViewProj);
}

float4 PSBasicNoFog(float4 Diffuse : COLOR0) : SV_Target0
{
    return Diffuse;
}

Technique PointEffect
{
    Pass
    {
        VertexShader = compile vs_3_0 VSBasicNoFog();
        PixelShader  = compile ps_3_0 PSBasicNoFog();
    }
}
krolli commented 5 years ago

@flibitijibibo It was what I was afraid of. Validation threw up on it. Luckily, GLSL runs into same problem, so I switched to using same check as GLSL does and now it seems to be fine. I also found similar vector/scalar mismatch in fog thanks to this, so I fixed that too (though fog probably won't work without pixel shader support anyway). Changes are pushed so you can try them and see if it helps.

@TheSpydog Interesting. I forgot Simply Chess had some extra problem on intel. I don't think textures are an issue here. This looks more like contents of one of the render targets, but in a capture I am looking it, it seems to only be written during GBuffer fill pass, but never read afterwards. Have you checked RenderDoc capture?

Murder Miners seem to be having some problem with cube map. Will take a look at it, I haven't checked how cube maps were implemented so far. Is it correct that character also looks different in spirv capture? He seems to be using different texture ...

flibitijibibo commented 5 years ago

Seems the validator's still unhappy for whatever reason... here's the effect binary just to be sure:

pointeffect.fxb.zip

flibitijibibo commented 5 years ago

fnaSPIRV.tar.bz2 has been updated. Additionally I've updated the OP with the complete list of FNA games to test. I'm pretty sure most of the FNA library is on a version recent enough to be using MojoShader instead of the old MGFX stuff, but if not let me know and I'll try to get a new build out. I know for sure Gateways and AVNT are on an old build, but that should be it...

TheSpydog commented 5 years ago

I can confirm that Dust: AET and TowerFall Ascension work without issue on Intel Windows.

@krolli Oh, good catch on the Murder Miners character texture! He does indeed look different in spirv. (Seems to be using only the helmet texture?)

krolli commented 5 years ago

@flibitijibibo Pushed a fix for point size semantic being used on input. Hopefully that helps Flotilla. I didn't notice it marked point size as inout so I only tested on point size output.

@TheSpydog Assuming we're seeing the same thing, I think there is some procedural coloring of texture based on character preset going on, but it's not part of the captured frame. The input texture mapped to the character looks different between spirv and glsl:

GLSL mm_glsl

SPIRV mm_spirv

Either color change is not applied at all, or only one color is applied / overwrites all others.

flibitijibibo commented 5 years ago

That's a weird one! But in case anyone's wondering, MM does indeed render out the colors of the player model based on a user preference. It's the first screen on a fresh install and should be accessible from the menu after that. It's user-specific, so make sure you're testing multiple colors for your personal setup.

krolli commented 5 years ago

Just found a shader called MinerArmorEffect.fxb in my test suite. Sounds like that might be the one responsible. :)

flibitijibibo commented 5 years ago

Indeed it is! Worth noting that the effect is among those in MM that use 3D textures, so that's a possible starting point.

krolli commented 5 years ago

Yeah, just noticed. Given that the other broken one is using cube map, I think sampler generation code needs some reviewing.

krolli commented 5 years ago

Pushed a fix for volume and cube samplers. Wish everything was that easy to find and fix. It was a funny one though. :)

flibitijibibo commented 5 years ago

Murder Miners got an update this afternoon, may be worth testing one more time. It now also has hardware instancing, so lots and lots of vertex usages now!

flibitijibibo commented 5 years ago

Updated A Virus Named TOM to use the latest FNA, should now be testable.

flibitijibibo commented 5 years ago

Video playback should now work in core contexts as long as you use FNA_GRAPHICS_FORCE_GLDEVICE=ModernGLDevice:

https://github.com/FNA-XNA/FNA/commit/e464a53edeb2075169ab365ce229f40ae7561f78

That should fix one of the more annoying parts of testing this...

krolli commented 5 years ago

Have you guys tried Murder Miners and Flotilla recently? I don't really have those games, so I'm just fixing it blindly, but I think they should be in better shape than they were. Also, I'm at a loss for what might be wrong with Chasm and Simply Chess on Intel and I don't have one to test it on. Could the same issue be also on Linux or is it specific to Windows driver?

Anyway, I've been looking into doing some cleanup and refactoring, but I want to try and make my current way of checking changes produce less noise to result ID renumbering, so there wasn't much activity in spirv branch itself.

flibitijibibo commented 5 years ago

From what I remember Intel Windows and Mesa don't share anything, I'm not even sure if their internal docs are the same... Intel Windows might have to go in the "meh" pile until we can see how their Vulkan support handles the same modules.

I'll do a fresh re-test of everything starting on Monday.

flibitijibibo commented 5 years ago

Did a mass test of my catalog (minus the games that are maintained by the studio), and everything seems good except for Flotilla, which appears to have the same problem as before. If it's useful at all, D9VK implemented points recently:

https://github.com/Joshua-Ashton/d9vk/commit/b756e626be1ce0d88c77856f45692e6d3b80e1df

krolli commented 5 years ago

Hmm, I wonder what might be wrong with Flotilla then. Compiled shader you sent me a while ago seems perfectly fine. Going through the D9VK changes, it all looks pretty much the same, though I would have to see shaders they generate for given binary to be sure. They seem to be handling also some extra rasterizer state related to point size that I think is not exposed in xna. At least, I don't see it handled anywhere in GLSL emitter.

Can you make a capture from both GLSL correct version and broken SPIRV using renderdoc? I might spot something I am missing.

flibitijibibo commented 5 years ago

This got totally buried under some other stuff these past two weeks, my bad - one bit of good news is that we can stop looking at Simply Chess on Intel, as the game has an invalid vertex attrib layout for one of its shaders, so that’s definitely a game bug that we can’t address. Still dunno about Chasm though.

I’ll try to get a Flotilla capture tomorrow, hopefully it’s okay with OpenGL point sprite rendering...

flibitijibibo commented 5 years ago

Managed to get a glsl120 capture and spirv capture, but the spirv capture likely won't load. It seems RenderDoc ignores point lists and captures them as GL_NONE instead of GL_POINTS. But, here they are anyway...

flotillardc.zip

The draw call you care about is glDrawArrays(256).

krolli commented 5 years ago

After our investigation on discord yesterday, I went in and added patching of TEXCOORD0 input of pixel shaders to PointCoord BuiltIn when vertex shader it links to outputs PSIZE. Let me know if it helped Flotilla.

Unfortunately, I couldn't find any official specification of this mapping, and it seems that at least at some point in time, IHVs used to implement this slightly differently (https://stackoverflow.com/questions/12041358/hlsl-point-sprite-texture-coordinates-work-on-ati-not-nvidia).

I also had a quick look at D9VK patch you linked previously, to see how they do the mapping, but I'm not all that familiar with their architecture in general and I couldn't find anything for this. I did notice they take care to implement various rasterizer state that controls point rasterization and judging by effect file in stackoverflow question, it seems like something that can in theory show up in XNA games as well, so it might be worth keeping an eye on this as well.

flibitijibibo commented 5 years ago

The pointcoord changes work for Flotilla, so that’s good enough for me! Basically everything but PointSpriteEnable is something that can be done in the vertex data, and this is all XNA3 stuff anyway so an XNA4 update will eliminate these issues.

I think all that’s left of the tested games is Chasm, I wonder if it’s another bogus attribute like Simply Chess?

flibitijibibo commented 4 years ago

Updated the OP's test archive, after who knows how long. I think Flotilla got the last of the major features, so at this point I think we're down to the final testing process. I'll try to look at some of the untested list, but I dunno how much of it I can actually cover... but really I just care about any remaining FIXMEs in the SPIR-V subsystem. After that's covered, it's time to reformat, play through my usual checklist of feature-heavy games, and get this merged at last.

krolli commented 4 years ago

Just had a quick look at FIXMEs and TODOs still remaining. Most seem to be either present in GLSL emitter, or could be turned into notes now, since they often describe some behavior of the emitter in corner case but it's unclear whether that's a problem or not.

flibitijibibo commented 4 years ago

Sounds good, did a quick review to see if anything stood out. Using the current diff...

Already committed elsewhere: https://bitbucket.org/krolli/mojoshader/branches/compare/spirv%0Ddefault#Lmojoshader_effects.cF1333T1351

Stuff that can be committed separately: https://bitbucket.org/krolli/mojoshader/branches/compare/spirv%0Ddefault#Lmojoshader.cF524 https://bitbucket.org/krolli/mojoshader/branches/compare/spirv%0Ddefault#Lmojoshader.cF1270 https://bitbucket.org/krolli/mojoshader/branches/compare/spirv%0Ddefault#Lmojoshader.cF1833 https://bitbucket.org/krolli/mojoshader/branches/compare/spirv%0Ddefault#Lmojoshader.cF3696T3720 https://bitbucket.org/krolli/mojoshader/branches/compare/spirv%0Ddefault#Lmojoshader_opengl.cF580 https://bitbucket.org/krolli/mojoshader/branches/compare/spirv%0Ddefault#Lmojoshader_opengl.cF711 https://bitbucket.org/krolli/mojoshader/branches/compare/spirv%0Ddefault#Lprofiles/mojoshader_profile_glsl.cF953T953

Actual Comments:

Most of it seems to just be cleanup that we can take in before taking in the SPIR-V stuff. I can import a zipfile full of patches for that list above, then I'll do one last pass on my own stuff before sending this to Ryan to review.

krolli commented 4 years ago

Already committed elsewhere: ...

I think you were looking at wrong branch. There is spirv-rebased that we created after last review and cleanup and that's where that fix came from. spirv branch is mostly just for my own testing but everything important should be in spirv-rebased.

Stuff that can be committed separately: ...

Most of those changes seem to have been there before I started working on it. Anyway, I agree those could be committed separately. Want me to pull them out or will you do it?

https://bitbucket.org/krolli/mojoshader/branches/compare/spirv%0Ddefault#Lmojoshader.cF1833

I'm not sure this is correct. I don't remember the exact problem, but I don't think I fully understood it when I ran into this either. It's just a workaround for parser choking on TEXM3X3. I didn't include it in spirv-rebased for now.

Isn't this already declared...?

Certainly looks like it. I would probably scope the first declaration above to the for loop where it's used, unless some compiler complains.

As I mentioned, spirv-rebased should contain less noise and none of my debug tweaks/hacks, so I'd recommend looking at that one instead.

flibitijibibo commented 4 years ago

For some reason when I did a comparison of rebased on Bitbucket it complained that there were no diffs... going to guess that their Hg support is already starting to rot a bit.

In any case, if you can get all the non-SPIR-V stuff separated into separate .patch files I'll push all of them today. After that we should probably re-rebase and squash.

krolli commented 4 years ago

Do we need to worry about div-by-zero for SPIR-V? I know we had to do this for GLSL/Metal, not sure if the SPIR-V spec says anything about 1/0=INF.

If it had to be solved for GLSL, than I think so. If you're referring to NRM opcode implementation, then that one is literally using GLSL 4.50 InverseSqrt semantics. I wasn't sure at the time whether to use OpSelect, or go through the basic block dance and write some actual control flow. Not having either in there seems pretty dumb though, so I'll fill it in.

flibitijibibo commented 4 years ago

The ones we recently patched up in GLSL are RCP/RSQ, but it wouldn't surprise me if there were other instructions with the issue:

https://github.com/FNA-XNA/MojoShader/commit/135aa1838ed57f41e2818f75a79326786d2d72a2#diff-4e911017b30f49678e6ce07bf820611b

krolli commented 4 years ago

Probably should give the same treatment to those as well. In which game did it show up as a problem? I would expect the same to show up with SPIR-V emitter.

flibitijibibo commented 4 years ago

I believe the only one with the bug still in the wild is Murder Miners, I found this with Shuggy and Reus but unfortunately I fixed those on the client side. If you need a copy ping me on Discord, I should have some gift copies available. Spydog should also have a repro case.

krolli commented 4 years ago

Pushed handling of corner cases for NRM, RCP and RSQ based on what GLSL emitter is doing. I noticed that according to DXBC docs, division by 0 in RCP and RSQ should return infinity, while NRM uses FLT_MAX instead. I decided to be consistent with GLSL emitter here and use FLT_MAX everywhere.

Unfortunately, for various reasons I haven't been able to test it on Murder Miners. If one of you guys can try it and let me know, that would be great. It's an extra commit on spirv-rebased-2 branch, so it should be fairly simple to do before/after comparison and see if the explosion particle effect problem was present before and fixed with these changes.

flibitijibibo commented 4 years ago

Just tested rebased2 and everything seems to work well, including the div-by-zero case in Murder Miners.

At this point I'm happy enough to where this can be merged - it'll be pretty much impossible to test everything, but it definitely seems to work with what I've thrown at it, so go ahead and do a final pass with any last edits, comment updates, notes for scary stuff, etc. and I'll get the final hg patch pushed before the end of December. (I'll also send the final chunk of the bounty before the year's over, just so we have everything in a single calendar year...)

krolli commented 4 years ago

Alright, I'll check it one more time to see if there are some comments worth updating. I definitely don't want to touch code unless we run into any more bugs. There are things I'd like to try to improve for some future update, but they are quite a lot more work that might require retesting everything, so not for this version.

krolli commented 4 years ago

Committed some final cleanup of TODOs, FIXMEs and testparse code (some of my unintended changes slipped through). All should be ready on my end.

flibitijibibo commented 4 years ago

The patches have been merged into upstream! From now on we'll diagnose issues on a case-by-case basis.