icculus / mojoshader

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

Reorganize profiles into their own files #5

Closed TheSpydog closed 5 years ago

TheSpydog commented 5 years ago

Couldn't get to sleep last night, so I did what any normal person would do -- clean up MojoShader! I ripped out all the profiles and put them in their own files in the profiles/ subdirectory. IMO this has been long overdue, but after having done it, I can totally see why nobody attempted it before. :P

A few things:

  1. This works just fine in all of my tests, but it'd be good to make doubly sure I didn't break anything in the transition.
  2. It might be worth a pass over the new header files to make sure the formatting looks okay. Beauty wasn't a very high priority when working on this initial commit...
  3. We've got to figure out what to do about that mojoshader_internal.h error that I commented out. It's being fussy with the new files (probably due to some preprocessor ordering rule I'm not familiar with), but hopefully it won't be a difficult fix.
  4. Since we're already overhauling the project structure, what are your thoughts on killing off that ARB1 target? TBH I don't even know what it is -- the top google search for "arb1 shader" was MojoShader's website. But I do know that removing it would simplify some things and a fix a small weird bit in the current implementation.
flibitijibibo commented 5 years ago
  1. I’ll most likely start with a side-by-side comparison along with a binary size comparison. I’m hoping it’ll be exactly the same without debug symbols...
  2. Any spots that are all new should be noted, just so we know what was actually added compared to the original.
  3. Probably just need to define that value before including stuff in the profile c files.
  4. I thought about this a lot, but only because the main file was so huge. ARB1 refers to ARB_vertex_program and ARB_fragment program, along with some NV shader extensions. Most (if not all) of this predates GLSL! This is how you can tell MojoShader was written in 2008... back then you needed these for good hardware support, but that’s not really true anymore. But I know Ryan likes his old hardware support, so I dunno if I want to mess with it. At least now it’s not as annoying to maintain.
TheSpydog commented 5 years ago
  1. From my testing, the binary size (even when stripped) is quite a bit bigger than the previous version. I'm wondering if it's due to moving a lot of static inline functions out of mojoshader.c and into the mojoshader_profile_common.h where they're getting copied and pasted around several files.
  2. The profile header files are the only major addition, but they were copied directly from their corresponding source files (with small changes to macros and such). Beyond that, I don't think there was anything significant added...
  3. Yup, that was it.
  4. Makes sense!
flibitijibibo commented 5 years ago

Very well, I’ll try to review the refactor tomorrow.

We’ll have to figure out the best path for dealing with the static functions. May have to macro, may have to throw them in common.c. That may have to be a separate commit though, since this changes enough as it is...

TheSpydog commented 5 years ago

All right, all the formerly static inline functions from mojoshader_profile.h are now non-static, non-inline functions housed in mojoshader_profile_common.c. I've additionally tried to organize things a bit better to match the original declaration order of mojoshader.c, although there are still some notable changes and/or omissions:

Several functions are only ever used in mojoshader.c, so they're still defined as static functions in that file:

A couple functions turned out to only be used by the ARB1 profile, so I moved them into mojoshader_profile_arb1.c:

And finally, get_used_register() was literally never used anywhere, so I just removed it.

Also, I merged the PREDECLARE_PROFILE macro calls into the existing #if SUPPORT_PROFILE_* stuff, per your comment.

TheSpydog commented 5 years ago

All right, with the help of Bloaty McBloatface and #pragma GCC visibility, I've managed to reduce the size difference between this version and the old version to a mere 56 bytes (when stripped and compiled with -DCOMPILER_SUPPORT=OFF).

Other notable change: usagestrs[] has been moved from mojoshader_profile.h to mojoshader_profile_d3d.c since only the D3D profile made use of this array.

TheSpydog commented 5 years ago

What's left to do here before this can be merged? Are we waiting on the SPIR-V emitter so we can refactor that into the new format too?

flibitijibibo commented 5 years ago

I'll do one last review of this today and if everything looks good we can make a Mercurial patch to push to upstream. EDIT: Have to push this to tomorrow, sorry!

flibitijibibo commented 5 years ago

Passed this through all of my compilers and they seem to be happy with everything. The very last thing we get to do also happens to be the most tedious: We'll need to fix parameter lists for functions that used to be static but aren't anymore. One example is isscalar, which looked like this...

https://github.com/FNA-XNA/MojoShader/pull/5/files#diff-d496920d66adab7260e5d2670f334605L766

... but now looks like this:

https://github.com/FNA-XNA/MojoShader/pull/5/files#diff-f6bd4f8f8bd83197cca2564b822858d9R357

We just have to line everything up again. Once that's done we should be good to go on the Hg version of this patch.

TheSpydog commented 5 years ago

All right, I think I got 'em all. There could be a few that snuck by me, but we'll fix them as we see them.

Here's the hg patch (uploaded as a .txt file since GitHub doesn't like .patch files): separateprofiles.txt

flibitijibibo commented 5 years ago

Merged into upstream:

https://hg.icculus.org/icculus/mojoshader/rev/b8ece252a201 https://github.com/FNA-XNA/MojoShader/commit/cd04fa3f1296f9aa99008b2310b5eabc3558a0d5