icculus / mojoshader

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

spirv: Fix PointCoord input using vec4 instead of vec2 #39

Closed krolli closed 3 years ago

krolli commented 3 years ago

This fixes Vulkan validation error about PointCoord input using vec4 instead of vec2 type by expanding pixel shader patching. To avoid having to modify result types of all accesses to t0 register (and possibly many more problems with shuffles and such), a private variable is initialized with contents of input variable at the beginning of generated shader, so only this initialization code needs to be patched.

Unfortunately, to keep patching simple, types must be declared for both cases, even if they never get used. As far as I can tell, spirv validator is fine with having unused types declared.

I don't have access to anything that uses point coords, so I've only checked that resulting binaries (both unpatched and patched in hacky way) pass spirv validator without issues. Someone should probably try it for real before merging.

cc @thatcosmonaut

thatcosmonaut commented 3 years ago

Tested this with Flotilla, which is the only game we are aware of that uses point coords, and it worked perfectly. Validation errors are gone. Looks good to me.

flibitijibibo commented 3 years ago

Wrote up a patch on top, just need to make sure this isn't any less broken and I'll merge the whole thing together:

From ec3ebed2e67c412e4cd344846b475468ebcf1215 Mon Sep 17 00:00:00 2001
From: Ethan Lee <flibitijibibo@gmail.com>
Date: Wed, 11 Nov 2020 10:12:59 -0500
Subject: [PATCH] Support reversing the pointcoord linker patch

---
 mojoshader_common.c                 | 4 +++-
 mojoshader_internal.h               | 5 ++++-
 profiles/mojoshader_profile_spirv.c | 2 ++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/mojoshader_common.c b/mojoshader_common.c
index 4270c53..a425cd5 100644
--- a/mojoshader_common.c
+++ b/mojoshader_common.c
@@ -1142,8 +1142,10 @@ void MOJOSHADER_spirv_link_attributes(const MOJOSHADER_parseData *vertex,
         } // if
         else
         {
-            // texcoord0Loc should already have attr_loc from the above work!
+            ((uint32 *) pixel->output)[pTable->pointcoord_var_offset + 1] = pTable->tid_pvec4i;
+            ((uint32 *) pixel->output)[pTable->pointcoord_load_offset + 1] = pTable->tid_vec4;
             ((uint32 *) pixel->output)[texcoord0Loc - 1] = SpvDecorationLocation;
+            // texcoord0Loc should already have attr_loc from the above work!
         } // else
     } // if
 } // MOJOSHADER_spirv_link_attributes
diff --git a/mojoshader_internal.h b/mojoshader_internal.h
index d303347..16f4ce4 100644
--- a/mojoshader_internal.h
+++ b/mojoshader_internal.h
@@ -754,11 +754,14 @@ typedef struct SpirvPatchTable
     int32 location_count;

     // TEXCOORD0 is patched to PointCoord if VS outputs PointSize.
-    // in `helpers`, [OpDecorate|id|Location|0xDEADBEEF] -> [OpDecorate|id|BuiltIn|PointCoord], offset derived from attrib_offsets[TEXCOORD][0].
+    // In `helpers`: [OpDecorate|id|Location|0xDEADBEEF] -> [OpDecorate|id|BuiltIn|PointCoord]
+    // Offset derived from attrib_offsets[TEXCOORD][0].
     uint32 pointcoord_var_offset; // in `mainline_intro`, [OpVariable|tid|id|StorageClass], patch tid to pvec2i
     uint32 pointcoord_load_offset; // in `mainline_top`, [OpLoad|tid|id|src_id], patch tid to vec2
     uint32 tid_pvec2i;
     uint32 tid_vec2;
+    uint32 tid_pvec4i;
+    uint32 tid_vec4;

     // Patches for linking vertex output/pixel input
     uint32 attrib_offsets[MOJOSHADER_USAGE_TOTAL][16];
diff --git a/profiles/mojoshader_profile_spirv.c b/profiles/mojoshader_profile_spirv.c
index 55f1daf..0d4aa08 100644
--- a/profiles/mojoshader_profile_spirv.c
+++ b/profiles/mojoshader_profile_spirv.c
@@ -2306,6 +2306,8 @@ void emit_SPIRV_attribute(Context *ctx, RegisterType regtype, int regnum,

                         table->tid_pvec2i = tid_pvec2i;
                         table->tid_vec2 = tid_vec2;
+                        table->tid_pvec4i = tid_pvec4i;
+                        table->tid_vec4 = tid_vec4;

                         push_output(ctx, &ctx->mainline_intro);
                         ctx->spirv.id_var_texcoord0_private = r->spirv.iddecl;
-- 
2.26.2
flibitijibibo commented 3 years ago

Squashed and manually applied:

https://hg.icculus.org/icculus/mojoshader/rev/ff4eb6d9c9c2