glslang version may need updating for Mac

[tomgreen66]

Tried compling Vulkan version on Mac and it seemed to require an update to glslang to current master. With current revision of glslang I got errors about unknown type for TBlockStorageClass in Include/Types.h but I couldnt spot why it had issues even though its an enum the Mac C++ compiler should support it. However after updating glslang submodule in the repository it compiled without any issues. Maybe the CMakeLists.txt had lots of changes so maybe had fixes for Mac?

Seem to have a version that works with Vulkan but I get errors about unable to save settings with autosaves errors and character models do not display in the game so not quite playable.

[SRSaunders]

This is a strange one. If I do a fresh clone of Robert's repo using the --recursive option, then I get the errors you mention above when compiling for Vulkan. However, if I clone without --recursive, and then afterwards clone glslang (https://github.com/KhronosGroup/glslang.git) into the extern directory, things work fine. Is there a difference in semantics when --recursive is used vs. separate cloning?

Also - I am not seeing the autosave or character model errors you mention. I will respond on your other thread #591 regarding this.

As an aside, I have never been able to get rid of a glslang compile error: Use of undeclared identifier 'INFINITY'. This happens due to idMath #undefing INFINITY, which cmath (used in glslang) does not like. As a hack, I have had to redefine INFINITY inside SpvTools.h:

// SRS - HACK: restore INFINITY otherwise cmath complains (note: INFINITY #undef in idMath)
#if defined(__APPLE__) && defined( USE_VULKAN )
#define INFINITY    HUGE_VALF
#endif

Has anyone else seen this error, or solved it in a better way?

[tomgreen66]

I assume clone into external will take latest revision while recursive is fixed to revision on glslang specified as submodule.

As for INFINITY issue, I hit that when I recently reinstalled Xcode, I'm on Mojave so tried 11.3.1 and hit that issue. Noticed that the version of command line tools was older and corresponded to Xcode 10.3 so installed that version and it all worked again. It's defined as a macro in the same file it complains about, thought maybe it's a standard issue (comment in file suggests C99).

[tomgreen66]

Aside, the error about autosave was my fault. Had a symlink in my Library folder to point base to steam version. Had deleted that steam version so broken symlink for base in my Library folder meant it couldn't save. Deleted broken symlink and all working again.

[SRSaunders]

You are right about the glslang submodule versions. To test I did the following on the --recursive clone directory:

$ git submodule foreach --recursive git checkout master
Entering 'neo/extern/glslang'
Previous HEAD position was bcf6a243 Move to SPIR-V 1.5 Rev. 3, bump revision, remove a status from README.
Switched to branch 'master'
Your branch is up to date with 'origin/master'.

This seems to confirm that git clone --recursive does not automatically point to master for the submodules. I wonder if this is something for the README, at least for macOS and perhaps other platforms?

Regarding the INFINITY issue, on Mojave I have been using Xcode 11.3.1 but possibly have older command line tools. Did you upgrade the command line tools to 11.3.1, or downgrade Xcode to 10.3 to make it work? When using Xcode 12.5.1 on Big Sur (with aligned command line tools) unfortunately I still have the same problem.

And thanks for confirming that autosave is still working fine.

[SRSaunders]

Dug into the cmath INFINITY issue a bit more and discovered that it's a toolchain issue that first appears in Xcode 11.3 / Apple clang-1100.0.33.16. For earlier macOS toolchains, cmath does not contain a reference to INFINITY, but in clang-1100.0.33.16 and later a new template is defined in cmath that uses INFINITY. Given that idMath #undef's INFINITY, that explains why Xcode 10.3 compiles the glslang code successfully and later versions of Xcode 11+ do not.

I plan to submit a pull request that solves this by redefining INFINITY inside RenderProgs_VK.cpp when glslang is used (vs. shaderc build option) and Apple clang version >= 11.

[tomgreen66]

Not sure whether to make this a new issue about INFINITY but I would be more comfortable if the solution was to rename idMath::INFINITY to idMath::infinity since it seems using uppercase variables inside namespaces can conflict with defines as happened here and cause all sorts of issues. Surprised undef INFINITY was deemed a good idea if its part of the standard (or maybe it wasnt in the C++ standard until C++11 which was much later than the C99 for C). Or remove IdMath::INFINITY with just INFINITY since C++11 is now the excepted standard I think.

[SRSaunders]

Hi Tom - if you have a better solution to the INFINITY problem please feel free to submit a change - I can always retract or Robert can reject that part of my pull request. Renaming idMath::INFINITY to idMath::infinity and removing #undef INFINITY in Math.h would not change any math operations and may be the best approach. Note that idMath::INFINITY = 1e30f and is different than INFINITY as defined in the standard. Just replacing idMath::INFINITY with INFINITY may result in different math outcomes.

Note that my proposed change, although perhaps not ideal, was designed for minimal impact to the RBDoom3BFG code base and to be specific to the macOS environment where the build problem manifests. I didn't want to force a significant testing effort for Windows or Linux at this point in the project's release cycle. While I do think your solution is better than mine, you will have to assess if the renaming change is portable and low risk (affects 26 files).

[tomgreen66]

Hi - just putting together a branch and testing it (having to fix a few other issues and testing without precompiled headers). All seemed to be going well but first time I used the newer Apple compiler and hit the following segfault:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x50)
    frame #0: 0x000000010059df42 RBDoom3BFG.infinity`idCameraAnim::Start(this=0x0000000164d16604) at Camera.cpp:475:6
   472      BecomeActive( TH_THINK );
   473
   474      // if the player has already created the renderview for this frame, have him update it again so that the camera starts this frame
-> 475      if( gameLocal.GetLocalPlayer()->GetRenderView()->time[TIME_GROUP2] == gameLocal.fast.time )
   476      {
   477          gameLocal.GetLocalPlayer()->CalculateRenderView();
   478      }
Target 0: (RBDoom3BFG.infinity) stopped.

Strange. Will take a look at it later - branch can be seen here https://github.com/tomgreen66/DOOM-3-BFG/commits/rbdoom3_infinityfix - also separated out a minor fix to CPU frequency calculation at: https://github.com/tomgreen66/DOOM-3-BFG/commits/rbdoom3_osxcpufreqfix

[SRSaunders]

I tried out the INFINITY renaming change (only) on my main branch and it worked fine with Xcode 11.3.1 on Mojave 10.14.6. It was a good idea and in my case no segfaults. However, I see two missing differences in your Math.h:

========================
idMath::InvSqrt
========================
*/
ID_INLINE float idMath::InvSqrt( float x )
{
    return ( x > FLT_SMALLEST_NON_DENORMAL ) ? sqrtf( 1.0f / x ) : infinity;
}

/*
========================
idMath::InvSqrt16
========================
*/
ID_INLINE float idMath::InvSqrt16( float x )
{
    return ( x > FLT_SMALLEST_NON_DENORMAL ) ? sqrtf( 1.0f / x ) : infinity;
}

Quick comments about your other changes:

1. Do you really need to turn off PCH for macOS? While perhaps not critical for Xcode (with parallel compilation) it will slow down macOS command line builds. Also, is your change to posix_public.h needed to fix PCH on Linux or other platforms? PCH has not been a problem on macOS for Xcode and command line builds across Mojave, Catalina, and Big Sur.

2. I see that you modified cmake-macos-vulkan-debug.sh. Note that I removed this shell script in a previous pull request which was merged into Robert's repo. This script is now defunct and macOS users should be using cmake-xcode-vulkan-debug.sh for Vulkan debug builds. I left the command line build scripts available for production and retail builds only.

3. Changing include_directories(${PKG_X11_XCB_INCLUDE_DIRS}) and include_directories(${X11_XCB_INCLUDE_DIRS}) to include_directories(${X11_XCB_INCLUDE_DIR}) seems to work for macOS Vulkan debug builds but breaks Vulkan release builds (where MoltenVK is used directly). Have you tested this with different build types? Since this change may affect other platforms as well, it likely needs some validation first.

I recommend that you focus this particular thread on the INFINITY (and FLT_EPSILON) fixes as related to glslang, and separate out the other potential changes above as separately tracked issues.

[tomgreen66]

Thanks for the comments.

1. I turned off PCH due to comment about Vulkan and wanted to see if it made a difference. If it really is just a Linux thing we can remove it but I thought I heard somewhere that PCH was not the future anyway. I did need the posix_public.h change to fix a dependency issue. Sounds like I should just ignore the PCH issue for now.
2. Its a shame to require Xcode rather than just just the command-line tools for debugging. I just wanted the debug symbols to be able to put it through a debugger.
3. the PKG_X11_XCB_INCLUDE_DIRS change is due to in the FindXCB_X11.cmake module it uses X11_XCB_INCLUDE_DIR as the variable to store the include directory rather than the plural variable currently used. I assume it works on Linux due to includes being in the standard search path of a compiler but on Mac it isnt. It is needed when I have the X11 includes in /usr/X11R6 on Mac using Xquartz - I was going to raise whether you use x11-xcb from homebrew instead and whether that might cause an issue for me if Xquartz is not supported.

Will do as suggested. Maybe for now we just do your simpler approach but really dont like redefinining of standard C/C++ defines.

[SRSaunders]

I prefer your solution to the INFINITY (and FLT_EPSILON) conflict as well. The root problem stems from old Doom3 code that was never updated for modern toolchains. Now it's just a matter of isolating the changes for it, testing on multiple platforms, and getting it submitted in a pull request. I made my own branch with just this change and have tested it on macOS Mojave, Catalina & Big Sur, Windows 10, and Linux Manjaro. No issues encountered (see https://github.com/SRSaunders/RBDOOM-3-BFG/commit/92cadb8380392fb16d9ea8761101acc0707abd55). If you agree with these changes, I could just append as a new commit to my existing pull request (pending). Whatever's easier - let me know.

Re your other points:

1. I would prefer to leave PCH alone as nothing is broken for macOS. The comment in the CMakeLists.txt file is for Linux based on work done by Robert on his test platforms (that's why CMakeLists says STREQUAL and not MATCHES for the Clang compiler in the PCH section). Also you shouldn't need the posix_public.h change. What is happening in your case?

2. Regarding the macOS command line debug shell scripts, I wanted to reduce the number of .sh files for macOS and also to avoid needing the MVK_CONFIG_FULL_IMAGE_VIEW_SWIZZLE runtime environment variable for command line Vulkan debug builds. That problem is solved in the Xcode debug version. However, nothing stops you from using my old scripts or creating your own for command line debug.

3. The PKG_X11_XCB_INCLUDE_DIRS is an interesting problem and probably deserves a new issue if you want XQuartz to be supported. I didn't want to make it a dependency for macOS (see the README for macOS build instructions). Homebrew's libx11 and libxcb come in automatically via the FFmpeg dependency chain which is needed anyways. The question is does XQuartz interfere with find_package(X11_XCB) and the X11_XCB include dirs. I must admit I never tested that. Perhaps there is a solution that works for both, but this would need additional investigation separate from this thread. Update: Installing XQuartz after Homebrew's libx11 does not break PKG_X11_XCB_INCLUDE_DIRS at least on macOS Mojave. Were you trying to use XQuartz exclusively? Not sure how you got into this state if you had installed Homebew's FFmpeg as per README instructions. I have a small CMakeLists tweak that may help, but need to know more first. Please advise via a new issue if you want to pursue further.

[tomgreen66]

Okay. It was the missing change to INFINITY which after fixing it everything works as expected. I think I have now tidied up the pull request and submitted #593 to be just the change use of INFINITY and FLT_EPSILON which are both in C/C++ standard so the idMath namespace versions are lowercase as to not to clash. Will open other tickets for the other issues since I think the X11_XCB is just not doing as expected. The posix_public.h is that if we turn of PCH for Mac there are some missing includes - will give more details in separate ticket.

Thanks for all the advice and support. Not sure we ever decided on the glslang issue. Suppose we need to put a pull request to update documentation to tell people to update it for Mac - tricky to test on all platforms on its impact.

[SRSaunders]

Thanks for doing this. Regarding glslang, as part of my original pull request #592 I had submitted a change in the README with information about glslang (need version 11.4.0 or later for macOS). Since this is not yet merged, I changed this pull request to provide only the README information and dropped my original INFINITY fix. Robert can then decide if he wants to update the README or update the glslang version in the RBDOOM-3-BFG repo.

Regarding X11_XCB, I already have a fix that works for both XQuartz and Homebrew x11xcb. It incorporates the issue you caught - that X11_XCB_INCLUDEDIR should not be plural, which is important for both XQuartz and Linux. For Homebrew the PKG X11_XCB_INCLUDE_DIRS variable is important and required due to libx11's multi-directory install structure. Testing with XQuartz also revealed a macOS-specific SDL.h include problem in posix_main.cpp that needed fixing as well. See changes here: https://github.com/SRSaunders/RBDOOM-3-BFG/commit/8f7dbe1243be63f955dab61abdfa13028236f688. If you could test this I would appreciate it. We could address this in the new issue you open.

Finally, I decided to test with and without FFmpeg installed and cleaned up a few compiler warnings with libbinkdec and added new build instructions to the README regarding FFmpeg optionality and X11 (for Vulkan builds). See changes here: https://github.com/SRSaunders/RBDOOM-3-BFG/commit/507973f8cf45b9d4bf4b440bde44d6bac47e9d78.

[SRSaunders]

One more thing. I will be away for a few days, so I wanted to mention something re the macOS PCH issue you identify above (i.e. build errors when PCH disabled for macOS). It looks like this has been dealt with before but disabled for some reason. Have a look at the first code line (line 29) in platform_osx.cpp. The #include "../../idlib/precompiled.h" has been commented out. Reenable it and the issue disappears. I tested it with multiple build types and PCH settings with no issue. Advantage with this possible fix is that it is macOS-specific. Just my 2 cents for the moment...

[tomgreen66]

Tested SRSaunders@8f7dbe1 and seems to work for Xquartz and also installed test Homebrew version to test the pkg-config worked.

The issue with precompiled header is that I get the following:

In file included from /Users/tom/Work/git/myDOOM3-BFG/neo/sys/posix/platform_osx.cpp:30:
/Users/tom/Work/git/myDOOM3-BFG/neo/sys/posix/../posix/posix_public.h:38:8: error:
      unknown type name 'glconfig_t'
extern glconfig_t glConfig;
       ^
/Users/tom/Work/git/myDOOM3-BFG/neo/sys/posix/../posix/posix_public.h:40:23: error:
      unknown type name 'sysEventType_t'
void            Posix_QueEvent( sysEventType_t type, int value, int valu...
                                ^

Which made me place the precompiled.h include in posix_public.h - maybe wrong approach which fixed everything in platform_osx.cpp as a consequence as well. Will open issue for it rather than carry on here.

[SRSaunders]

@tomgreen66 - Please verify pull request #598 fixes your issues with default glslang version referenced by project.

[tomgreen66]

@tomgreen66 - Please verify pull request #598 fixes your issues with default glslang version referenced by project.

Yep - seems to fix the compile issue so we can keep the existing revision for glslang.

[SRSaunders]

@tomgreen66 - Now that you have verified the fix, would you close this glslang issue?

Also, could you close #596 (X11_XCB) and #597 (PCH) once you have verified? Thanks.

[tomgreen66]

All seems to work when glslang fix is applied.