Linux and GCC support #3

Zakhrov · 2019-07-24T09:26:21Z

I have cleaned up the Vulkan backend to get it to compile on Linux with GCC 9.1 and replaced the DirectXMath library with GLM. The code compiles successfully and creates the static libraries but I haven't been able to run the glTFSample project

YvanDaSilva · 2019-07-24T10:57:54Z

I don't have time to try it on Windows again, did you try it ?

I just had a quick look at the changes and at the code quality of this repo in general.
Has this sdk been reviewed by someone before ?
Lots of random spaces, lack of name consistency, src contains a different license file and other "basic" workflow stuff that shouldn't be commit like that.

I would suggest that a cleanup occur before this PR is applied, so the change is a bit clearer, and maybe travis is also added so we can ensure Cross platform compatibility at least at build time.
Maybe lets get some info about other gpuopen repos like:
https://github.com/GPUOpen-LibrariesAndSDKs/Anvil
https://github.com/GPUOpen-LibrariesAndSDKs/VulkanMemoryAllocator

luntik2012 · 2019-07-24T14:34:52Z

please, add build $ cmake .. -DGFX_API=VK/DX12 to build instructions in readme.md

Zakhrov · 2019-07-24T15:03:14Z

please, add build $ cmake .. -DGFX_API=VK/DX12 to build instructions in readme.md,

Done in the latest commit on my fork

src/VK/CMakeLists.txt

Mixaill · 2019-07-28T20:02:20Z

@Zakhrov I've updated a bit your branch with further fixes. Now Cauldron compiles on Windows and Linux, but there is a big amount of work to get it into the working state.

https://github.com/Mixaill/Cauldron/tree/linux-fixes

get rid of SAL.h

Zakhrov · 2019-07-30T08:57:17Z

@Zakhrov I've updated a bit your branch with further fixes. Now Cauldron compiles on Windows and Linux, but there is a big amount of work to get it into the working state.

https://github.com/Mixaill/Cauldron/tree/linux-fixes

I still had problems resolving <DirectXMath.h> It seems to be working now that I passed in a relative include path. I still have to see if the glTFSample compiles

aguaviva · 2019-07-30T12:28:00Z

Maintainer here, just to set some expectations, the plan is to not support linux officially (maybe for now) but I still pay attention to all your comments and suggestions, so from now on I will try to be more GCC friendly :)

Zakhrov · 2019-07-30T12:36:41Z

Maintainer here, just to set some expectations, the plan is to not support linux officially (maybe for now) but I still pay attention to all your comments and suggestions, so from now on I will try to be more GCC friendly :)

OK thanks for letting us know. Will close the PR for now

get rid of SAL.h

Hi-Angel · 2021-07-15T18:40:12Z

For the sake of cross-references: this fixes #1

…Cauldron into GPUOpen-LibrariesAndSDKs-master

Zakhrov · 2021-07-16T10:32:41Z

I've updated the code to Cauldron V1.4.1 and fixed the build errors with GCC. There is still a lot of work to be done before its actually usable though.

TODO

Create Linux specific Vulkan surfaces using VkXlibSurfaceCreateInfoKHR and VkWaylandSurfaceCreateInfoKHR
Port sample glTF projects to Linux

Some doubts

Can we move the Vulkan components from DirectXMath to GLM? I'm right now using a patched fork of DirectXMath that compiles without the sal.h headers that are Windows specific. I don't think that would be feasible in the long run.
It looks like it uses the DirectX shader compiler for compiling shaders on both DX12 and Vulkan. Can we use the default SPV shader compiler along with GLSL vertex and fragment shaders on Vulkan?

Mixaill · 2021-07-16T11:53:25Z

Can we move the Vulkan components from DirectXMath to GLM? I'm right now using a patched fork of DirectXMath that compiles without the sal.h headers that are Windows specific. I don't think that would be feasible in the long run.

You should just use the vanilla DirectXMath and MIT-licensed SAL from https://github.com/dotnet/corert/blob/master/src/Native/inc/unix/sal.h
At the first look there are only few usages of DirectXMath (Vec3/Vec4 and several constants), the other math was converted to the Sony vectormath library. I think it should be easy to eliminate DirectXMath usage for non-D3D code at least.
GLM is not needed at all.

It looks like it uses the DirectX shader compiler for compiling shaders on both DX12 and Vulkan. Can we use the default SPV shader compiler along with GLSL vertex and fragment shaders on Vulkan?

We can switch from the prebuilt binaries to the github version https://github.com/Microsoft/DirectXShaderCompiler

TODO

Mouse/Keyboard handling for the ImGui should be implemented too. Looks like it is worth to outsource Mouse/Keyboard and Window handling to something like SDL2.

Micket

Disclaimer: I'm not affiliated with AMD in any way, I just wanted to give some feedback to hope that this could move forward.
I have some specific comments, but, overall, I believe a PR like this is completely unmergeable. It's not even review-able. I'm going to echo @YvanDaSilva here, but it's just waaaaay to big, with waaaay to many changes unrelated to the specific purpose.

Whitespace changes: Either leave as is or fix in it's own PR.
nullptr modernization: Either leave as is or fix in it's own PR.
UINT8 -> uint8_t etc.: Either leave (just introduce define flags for them on linux if necessary) or replace in it's own PR.
Fix case errors with filenames (without linux changes) as much as possible in separate PR
and so on (probably splitting up the remaining
then the rest of the interesting fixes can be added gradually, preferably in a few distinct PRs working ones way up to a fully working build.

I would create a new branch, cherry pick the changes from here and open up PRs comparable to the steps above (or according to whatever format AMD prefers) to break this down
Then, possibly rebase what is left of this PR on that.

Again: I don't speak for anyone else but myself here.

Micket · 2021-07-16T20:19:05Z

src/common/base/ShaderCompiler.cpp

+                    strcpy(includeName, pRootDir);
+                    strncat(includeName, pName, pch - pName);


Unsafe functions here is a bad idea.

Micket · 2021-07-16T20:22:48Z

src/common/stdafx.h

+#include <algorithm>
+#include <mutex>
+#else
+// ... handle this for other compilers


Most of this duplicated code is standard and must be the same across each platform, no need to group them all under each ifdefs.

Micket · 2021-07-16T20:32:38Z

src/VK/PostProc/Bloom.cpp

@@ -359,7 +359,7 @@ namespace CAULDRON_VK
            for (int i = 1; i < m_mipCount; i++)
            {
                char buf[32];
-                sprintf_s<32>(buf, "weight %i", i);
+                sprintf(buf, "weight %i", i);


Micket · 2021-07-16T20:35:41Z

src/VK/PostProc/SkyDome.cpp

-#include "Base/UploadHeap.h"
-#include "Base/Helper.h"
-#include "Base/Texture.h"
+#include "base/DynamicBufferRing.h"


Given the many many many changes like this. Wouldn't it be better to rename the directory?

Micket · 2021-07-16T20:37:44Z

src/VK/base/Device.cpp

+VkXlibSurfaceCreateInfoKHR createInfo = {};
+createInfo.sType = VK_STRUCTURE_TYPE_XLIB_SURFACE_CREATE_INFO_KHR;
+createInfo.pNext = NULL;
+createInfo.dpy = NULL;
+res = vkCreateXlibSurfaceKHR(m_instance, &createInfo, NULL, &m_surface);


Indentation

Micket · 2021-07-16T20:50:07Z

src/common/CMakeLists.txt

+if(MSVC)
+  add_compile_options(/MP)
+else()
+add_compile_options(-M)


Micket · 2021-07-16T20:55:05Z

src/common/CMakeLists.txt

+  set(shader_compiler_src
+      "base/ShaderCompiler.cpp"
+      "base/ShaderCompiler.h"
+      "base/ShaderCompilerCache.cpp"
+      "base/ShaderCompilerCache.h"
+      "base/DXCHelper.cpp"
+      "base/DXCHelper.h"
+  )
+else()
+  set(shader_compiler_src
+      "base/ShaderCompiler.cpp"
+      "base/ShaderCompiler.h"
+      "base/ShaderCompilerCache.cpp"
+      "base/ShaderCompilerCache.h"
+  )


Might be nicer to just have the platform specific parts separated out sharing the common sources

if(MSVC) list(APPEND shader_compiler_src "base/DXCHelper.cpp" "base/DXCHelper.h" ) endif()

Micket · 2021-07-16T20:57:28Z

src/common/GLTF/GltfHelpers.cpp

@@ -146,6 +146,5 @@ math::Vector4 GetElementVector(json::object_t &root, const char *path, math::Vec
        return GetVector(root[path].get<json::array_t>());
    }
    else
-        return default;
+        return default_value;


This can't be right, the function arguments haven't changed, only the return statements have gotten the replacement default -> default_value

Micket · 2021-07-16T20:59:29Z

src/common/Misc/Camera.cpp


-    if (keyDown['W'])
+#ifdef _WIN32
+	if (keyDown['W'])


Indentation mismatch

Micket · 2021-07-16T21:02:25Z

src/common/Misc/Misc.cpp

@@ -121,8 +134,6 @@ void Trace(const char* pFormat, ...)
 //
 bool ReadFile(const char *name, char **data, size_t *size, bool isbinary)
 {
-    FILE *file;
-
    //Open file
    if (fopen_s(&file, name, isbinary ? "rb" : "r") != 0)


This looks unintentional? file will be undefined below here, this can't be right.

simsilver · 2022-07-04T06:39:11Z

CMakeLists.txt

+set(CMAKE_GENERATOR_PLATFORM x64)
+add_compile_options(/MP)
+else()
+add_compile_options(-M)


this hide all compile error

Fixes for Linux build success

416fb19

more fixes

de2f755

Update README with Linux build instructions

7ef67e5

janisozaur reviewed Jul 25, 2019

View reviewed changes

src/VK/CMakeLists.txt Outdated Show resolved Hide resolved

Mixaill added 14 commits July 28, 2019 19:31

fix DirectXMath under MSVC

caf5ac3

fix compilation under MSVC

c8ce6aa

other: add .gitignore

4d4b149

replace MSVC _countof() macro with std::size()

9b4efed

libs: add dxgiformat.h header for non-windows platforms

1be7d5f

Added submodule libs/DirectXMath

00099ed

fix DirectXMath includes

61e3bc2

replace NULL with nullptr

ef568c6

disable windows-specific things

cdf63ff

fix includes and data types

f7f2f49

update nlohmann-json

a70eba6

fix compilation

ffea323

src/common: fix build script (disable -M flag)

bc3e486

fix Framework_Common compilation under Linux

06f2a5b

Mixaill and others added 4 commits July 29, 2019 02:08

libs/DirectXMath: improved GCC support

2157e26

get rid of SAL.h

Merge branch 'linux-fixes' of https://github.com/Mixaill/Cauldron

12b4504

remove DirectXMath

a68358d

Linux fixes

01d3ada

Zakhrov closed this Jul 30, 2019

Mixaill and others added 17 commits September 30, 2019 16:02

libs: add dxgiformat.h header for non-windows platforms

4ff4551

Added submodule libs/DirectXMath

c49f0bb

fix DirectXMath includes

acd1960

replace NULL with nullptr

5bbdd57

disable windows-specific things

905ff61

fix includes and data types

3e88791

update nlohmann-json

6343a09

fix compilation

6406121

src/common: fix build script (disable -M flag)

59a3de2

fix DirectXMath under MSVC

6d47a8e

fix compilation under MSVC

73d1484

fix Framework_Common compilation under Linux

161ad59

libs/DirectXMath: improved GCC support

c0ea032

get rid of SAL.h

remove DirectXMath

a89e412

Linux fixes

e99aef2

Add build to gitignore"

afe1005

Merge branch 'master' of https://github.com/Zakhrov/Cauldron

54d27ae

Hi-Angel mentioned this pull request Jul 15, 2021

Linux support GPUOpen-Effects/FidelityFX-FSR#1

Open

Zakhrov added 3 commits July 16, 2021 09:44

Merge branch 'master' of https://github.com/GPUOpen-LibrariesAndSDKs/…

7914446

…Cauldron into GPUOpen-LibrariesAndSDKs-master

Merge branch 'GPUOpen-LibrariesAndSDKs-master'

356b474

Fix 1.4.1 build errors on Linux.

6c50024

Zakhrov reopened this Jul 16, 2021

rys self-assigned this Jul 16, 2021

rys added enhancement New feature or request platform Platform support labels Jul 16, 2021

Micket reviewed Jul 16, 2021

View reviewed changes

simsilver reviewed Jul 4, 2022

View reviewed changes

CMakeLists.txt

set(CMAKE_GENERATOR_PLATFORM x64)

add_compile_options(/MP)

else()

add_compile_options(-M)

Copy link

simsilver Jul 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this hide all compile error

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linux and GCC support #3

Linux and GCC support #3

Zakhrov commented Jul 24, 2019

YvanDaSilva commented Jul 24, 2019

luntik2012 commented Jul 24, 2019 •

edited

Loading

Zakhrov commented Jul 24, 2019

Mixaill commented Jul 28, 2019

Zakhrov commented Jul 30, 2019 •

edited

Loading

aguaviva commented Jul 30, 2019

Zakhrov commented Jul 30, 2019

Hi-Angel commented Jul 15, 2021

Zakhrov commented Jul 16, 2021

Mixaill commented Jul 16, 2021 •

edited

Loading

Micket left a comment

Micket Jul 16, 2021

Micket Jul 16, 2021

Micket Jul 16, 2021

Micket Jul 16, 2021

Micket Jul 16, 2021

Micket Jul 16, 2021

Micket Jul 16, 2021

Micket Jul 16, 2021

Micket Jul 16, 2021

Micket Jul 16, 2021

simsilver Jul 4, 2022

		strcpy(includeName, pRootDir);
		strncat(includeName, pName, pch - pName);

Linux and GCC support #3

Are you sure you want to change the base?

Linux and GCC support #3

Conversation

Zakhrov commented Jul 24, 2019

YvanDaSilva commented Jul 24, 2019

luntik2012 commented Jul 24, 2019 • edited Loading

Zakhrov commented Jul 24, 2019

Mixaill commented Jul 28, 2019

Zakhrov commented Jul 30, 2019 • edited Loading

aguaviva commented Jul 30, 2019

Zakhrov commented Jul 30, 2019

Hi-Angel commented Jul 15, 2021

Zakhrov commented Jul 16, 2021

TODO

Some doubts

Mixaill commented Jul 16, 2021 • edited Loading

Micket left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luntik2012 commented Jul 24, 2019 •

edited

Loading

Zakhrov commented Jul 30, 2019 •

edited

Loading

Mixaill commented Jul 16, 2021 •

edited

Loading