Skip to content

Commit 3b61167

Browse files
committed
fix CHLSLCompiler - move RequiredArguments into private section and expose public getter. Update sources accordingly, explain in the comment why you cannot keep such data symbols in public section. Add missing L"-HV", L"202x" arguments to RequiredArguments
1 parent 8f9b129 commit 3b61167

File tree

3 files changed

+52
-22
lines changed

3 files changed

+52
-22
lines changed

include/nbl/asset/utils/CHLSLCompiler.h

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -54,18 +54,11 @@ class NBL_API2 CHLSLCompiler final : public IShaderCompiler
5454
std::string preprocessShader(std::string&& code, IShader::E_SHADER_STAGE& stage, const SPreprocessorOptions& preprocessOptions, std::vector<std::string>& dxc_compile_flags_override, std::vector<CCache::SEntry::SPreprocessingDependency>* dependencies = nullptr) const;
5555

5656
void insertIntoStart(std::string& code, std::ostringstream&& ins) const override;
57-
constexpr static inline const wchar_t* RequiredArguments[] = { // TODO: and if dxcOptions is span of std::string then why w_chars there? https://en.cppreference.com/w/cpp/string/basic_string
58-
L"-spirv",
59-
L"-Zpr",
60-
L"-enable-16bit-types",
61-
L"-fvk-use-scalar-layout",
62-
L"-Wno-c++11-extensions",
63-
L"-Wno-c++1z-extensions",
64-
L"-Wno-c++14-extensions",
65-
L"-Wno-gnu-static-float-init",
66-
L"-fspv-target-env=vulkan1.3"
67-
};
68-
constexpr static inline uint32_t RequiredArgumentCount = sizeof(RequiredArguments) / sizeof(RequiredArguments[0]);
57+
58+
static constexpr auto getRequiredArguments() //! returns required arguments for the compiler's backend
59+
{
60+
return std::span(RequiredArguments);
61+
}
6962

7063
protected:
7164
// This can't be a unique_ptr due to it being an undefined type
@@ -81,6 +74,23 @@ class NBL_API2 CHLSLCompiler final : public IShaderCompiler
8174
ret.setCommonData(options);
8275
return ret;
8376
}
77+
78+
private:
79+
// we cannot have PUBLIC data symbol in header we do export - endpoint application will fail on linker with delayed DLL loading mechanism (thats why we trick it with private member hidden from the export + provide exported getter)
80+
// https://learn.microsoft.com/en-us/previous-versions/w59k653y(v=vs.100)?redirectedfrom=MSDN
81+
constexpr static inline auto RequiredArguments = std::to_array<const wchar_t*> // TODO: and if dxcOptions is span of std::string then why w_chars there? https://en.cppreference.com/w/cpp/string/basic_string
82+
({
83+
L"-spirv",
84+
L"-Zpr",
85+
L"-enable-16bit-types",
86+
L"-fvk-use-scalar-layout",
87+
L"-Wno-c++11-extensions",
88+
L"-Wno-c++1z-extensions",
89+
L"-Wno-c++14-extensions",
90+
L"-Wno-gnu-static-float-init",
91+
L"-fspv-target-env=vulkan1.3",
92+
L"-HV", L"202x"
93+
});
8494
};
8595

8696
}

src/nbl/asset/utils/CHLSLCompiler.cpp

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -211,12 +211,15 @@ static void add_required_arguments_if_not_present(std::vector<std::wstring>& arg
211211
auto set = std::unordered_set<std::wstring>();
212212
for (int i = 0; i < arguments.size(); i++)
213213
set.insert(arguments[i]);
214-
for (int j = 0; j < CHLSLCompiler::RequiredArgumentCount; j++)
214+
215+
const auto required = CHLSLCompiler::getRequiredArguments();
216+
217+
for (int j = 0; j < required.size(); j++)
215218
{
216-
bool missing = set.find(CHLSLCompiler::RequiredArguments[j]) == set.end();
219+
bool missing = set.find(required[j]) == set.end();
217220
if (missing) {
218-
logger.log("Compile flag error: Required compile flag not found %ls. This flag will be force enabled, as it is required by Nabla.", system::ILogger::ELL_WARNING, CHLSLCompiler::RequiredArguments[j]);
219-
arguments.push_back(CHLSLCompiler::RequiredArguments[j]);
221+
logger.log("Compile flag error: Required compile flag not found %ls. This flag will be force enabled, as it is required by Nabla.", system::ILogger::ELL_WARNING, required[j]);
222+
arguments.push_back(required[j]);
220223
}
221224
}
222225
}
@@ -397,9 +400,10 @@ core::smart_refctd_ptr<ICPUShader> CHLSLCompiler::compileToSPIRV_impl(const std:
397400
populate_arguments_with_type_conversion(arguments, hlslOptions.dxcOptions, logger);
398401
}
399402
else { // lastly default arguments
403+
const auto required = CHLSLCompiler::getRequiredArguments();
400404
arguments = {};
401-
for (size_t i = 0; i < RequiredArgumentCount; i++)
402-
arguments.push_back(RequiredArguments[i]);
405+
for (size_t i = 0; i < required.size(); i++)
406+
arguments.push_back(required[i]);
403407
arguments.push_back(L"-HV");
404408
arguments.push_back(L"202x");
405409
// TODO: add this to `CHLSLCompiler::SOptions` and handle it properly in `dxc_compile_flags.empty()`
@@ -479,5 +483,4 @@ void CHLSLCompiler::insertIntoStart(std::string& code, std::ostringstream&& ins)
479483
code.insert(0u, ins.str());
480484
}
481485

482-
483486
#endif

src/nbl/ext/ImGui/ImGui.cpp

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
#include <ranges>
44
#include <vector>
55
#include <utility>
6+
#include <locale>
7+
#include <codecvt>
68

79
#include "nbl/system/CStdoutLogger.h"
810
#include "nbl/ext/ImGui/ImGui.h"
@@ -181,19 +183,34 @@ namespace nbl::ext::imgui
181183

182184
auto compileToSPIRV = [&]() -> smart_refctd_ptr<ICPUShader>
183185
{
184-
#define NBL_DEFAULT_OPTIONS "-spirv", "-Zpr", "-enable-16bit-types", "-fvk-use-scalar-layout", "-Wno-c++11-extensions", "-Wno-c++1z-extensions", "-Wno-c++14-extensions", "-Wno-gnu-static-float-init", "-fspv-target-env=vulkan1.3", "-HV", "202x" /* default required params, just to not throw warnings */
186+
auto toOptions = []<uint32_t N>(const std::array<std::string_view, N>& in) // options must be alive till compileToSPIRV ends
187+
{
188+
const auto required = CHLSLCompiler::getRequiredArguments();
189+
std::array<std::string, required.size() + N> options;
190+
191+
std::wstring_convert<std::codecvt_utf8<wchar_t>> converter;
192+
for (uint32_t i = 0; i < required.size(); ++i)
193+
options[i] = converter.to_bytes(required[i]); // meh
194+
195+
uint32_t offset = required.size();
196+
for (const auto& opt : in)
197+
options[offset++] = std::string(opt);
198+
199+
return options;
200+
};
201+
185202
const std::string_view code (reinterpret_cast<const char*>(shader->getContent()->getPointer()), shader->getContent()->getSize());
186203

187204
if constexpr (stage == IShader::E_SHADER_STAGE::ESS_VERTEX)
188205
{
189-
const auto VERTEX_COMPILE_OPTIONS = std::to_array<std::string>({NBL_DEFAULT_OPTIONS, "-T", "vs_6_7", "-E", "VSMain", "-O3"});
206+
const auto VERTEX_COMPILE_OPTIONS = toOptions(std::to_array<std::string_view>({ "-T", "vs_6_7", "-E", "VSMain", "-O3" }));
190207
options.dxcOptions = VERTEX_COMPILE_OPTIONS;
191208

192209
return compiler->compileToSPIRV(code.data(), options); // we good here - no code patching
193210
}
194211
else if (stage == IShader::E_SHADER_STAGE::ESS_FRAGMENT)
195212
{
196-
const auto FRAGMENT_COMPILE_OPTIONS = std::to_array<std::string>({NBL_DEFAULT_OPTIONS, "-T", "ps_6_7", "-E", "PSMain", "-O3"});
213+
const auto FRAGMENT_COMPILE_OPTIONS = toOptions(std::to_array<std::string_view>({ "-T", "ps_6_7", "-E", "PSMain", "-O3" }));
197214
options.dxcOptions = FRAGMENT_COMPILE_OPTIONS;
198215

199216
std::stringstream stream;

0 commit comments

Comments
 (0)