CookiePLMonster / SilentPatch

SilentPatch for GTA III, Vice City, and San Andreas
MIT License
384 stars 15 forks source link

III/VC cross-compile fixes #95

Open TheComputerGuy96 opened 2 weeks ago

TheComputerGuy96 commented 2 weeks ago

This PR might be too large when compared to the ModUtils one (so I can split it if needed)

Compiling with MSVC isn't tested either (maybe I should try installing VS on Wine to avoid this testing void?; GitHub Actions would also help)

Note: Adding a different build system is required to actually use this work (#31 mentions Premake which might be okay here but Meson is an option too and it's used by some popular PE libraries like DXVK) so I'm currently using some hacky Makefiles (I might push those to a branch later)

CookiePLMonster commented 2 weeks ago

Before I start reviewing - is that not missing the inline assembly changes for San Andreas? That said, I would actually prefer the assembly changes and the rest split into two PRs.

Also as mentioned in #31, Actions will not happen because of the RW SDK dependency.

Some of those changes will also likely have to go under a macro, because SP needs to support v141_xp from VS2017 that has problems with static inline templated method pointers.

TheComputerGuy96 commented 2 weeks ago

Before I start reviewing - is that not missing the inline assembly changes for San Andreas? That said, I would actually prefer the assembly changes and the rest split into two PRs.

That's intentional because San Andreas has even bigger inline assembly changes (and this PR is probably big enough) and I'm currently having weird segfaults in that game (so support will be delayed for a future PR)

Actions will not happen because of the RW SDK dependency

So even adding an external RW SDK source in the Actions .yml file is as bad as vendoring the headers directly, right? If so then I guess REing the definitions required by the headers might be the only way

Also RW 3.7 SDK seems to work okay for all of the GTA SilentPatches (so I'm not sure why a precise version is needed)

Some of those changes will also likely have to go under a macro, because SP needs to support v141_xp from VS2017 that has problems with static inline templated method pointers.

I can work around that if the older compiler requires it

CookiePLMonster commented 2 weeks ago

So even adding an external RW SDK source in the Actions .yml file is as bad as vendoring the headers directly, right? If so then I guess REing the definitions required by the headers might be the only way

Probably - it's a risk I choose not to take.

Also RW 3.7 SDK seems to work okay for all of the GTA SilentPatches (so I'm not sure why a precise version is needed)

At some point I remember it was problematic with one of the fixes, but maybe that was in another project. I vaguely recall that the SA fixes involving the anim interpolation broke with 3.7.

I can work around that if the older compiler requires it

I'll have to check if it's necessary on all templated static inlines, or only the method pointers. but something like #define STATIC_INLINE static inline and #define STATIC_INLINE static might be an okay solution.

TheComputerGuy96 commented 1 week ago

Some of the MSVC inline assembly statements do weird things (like doing a double pointer deference on a regular single-star pointer)

Is there any real reason for this?

CookiePLMonster commented 1 week ago

Do you have an example? Are you sure it's not a *&?

TheComputerGuy96 commented 1 week ago

Do you have an example? Are you sure it's not a *&?

This is one example: https://github.com/CookiePLMonster/SilentPatch/blob/dev/SilentPatchIII/SilentPatchIII.cpp#L199 (InstantHitsFiredByPlayer is defined as an int*)

CookiePLMonster commented 1 week ago

OK I think that may have been me being careless with the notation - the first mov is supposed to load the value in InstantHitsFiredByPlayer, then inc dereferences it.

TheComputerGuy96 commented 1 week ago

Anyway I just redid the MSVC inline assembly statements to be more logical (but they're of course untested)

Also there's some llvm-mingw/Clang fixes (now there's just some stubborn call instructions that won't work with the i constraint for some reason; I might make a bug report for that later)

CookiePLMonster commented 1 week ago

Thanks! I'll have to verify those assembly statements later. I just hope they don't start mov'ing an address of the variable, but for that I think it'd have to be mov eax, offset X?

CookiePLMonster commented 1 week ago

I checked the assembly changes and indeed, both mov eax, VAR and mov eax, [VAR] dereference it, so your changes are correct: image

mov'ing the address of VAR requires mov eax, offset VAR, which makes sense.

What do you think about me (or you) submitting those MSVC assembly changes separately so then your PR can become a bit smaller and easier to review? I could submit this change as a PR too for you to review.

TheComputerGuy96 commented 1 week ago

What do you think about me (or you) submitting those MSVC assembly changes separately so then your PR can become a bit smaller and easier to review?

I'll do that fairly soon

TheComputerGuy96 commented 1 week ago

Ironically up-to-date MSVC on Wine requires some of these cross-compile fixes too (despite not being GCC/Clang of course):

Z:\mnt\storagebeast\Development\General\SilentPatch\SilentPatch\Common.cpp(37,18): error C2065: 'RwIm2DRenderLine': undeclared identifier [Z:\mnt\storagebeast\Development\General\SilentPatch\SilentPatchIII\SilentPatchIII.vcxproj]
Z:\mnt\storagebeast\Development\General\SilentPatch\SilentPatch\RWGTA.cpp(55,8): error C2143: syntax error: missing ')' before '*' [Z:\mnt\storagebeast\Development\General\SilentPatch\SilentPatchIII\SilentPatchIII.vcxproj]
CookiePLMonster commented 1 week ago

Yeah, it's a combination of two things:

  1. VS2017 not being entirely C++17 conformant (static inline).
  2. Old Windows SDK 7.0 headers not being very strict with gating off the features (in this case, condition variables) behind a WINNT version check.
CookiePLMonster commented 3 days ago

I merged #113 because it's safe and testable, but I am going to hold this one off until the next hotfix is released.

TheComputerGuy96 commented 3 hours ago

Also the generated .rc files are fully UTF-16 (LE?) encoded which windres doesn't support at all (I'm currently converting those files to UTF-8 at compile-time which complicates the compile process a bit)