Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare NovelRT for C++ 20 #500

Open
4 of 6 tasks
Pheubel opened this issue Sep 10, 2022 · 2 comments · Fixed by #535 or #537
Open
4 of 6 tasks

Prepare NovelRT for C++ 20 #500

Pheubel opened this issue Sep 10, 2022 · 2 comments · Fixed by #535 or #537

Comments

@Pheubel
Copy link
Contributor

Pheubel commented Sep 10, 2022

Although NovelRT compiles for C++ 17 on both Clang and MSVC, there are some obstacles that prevent the engine from moving forwards to C++ 20.

Status resolved

Fabulist

Fabulist currently fails to compile in C++ 20 due to a change in how std:accumulate() works. Relevant issue: novelrt/Fabulist#14

NovelRT/LoggingService

Due to how C++ 20 behaves, the interaction with spdlog and fmt becomes a bit more rough. The following lines are affected:

template<typename I, typename... IRest> void logInfo(I current, IRest... next) const
{
_logger->info(current, std::forward<IRest>(next)...);
}
template<typename E, typename... ERest> void logError(E current, ERest... next) const
{
_logger->error(current, std::forward<ERest>(next)...);
}
template<typename W, typename... WRest> void logWarning(W current, WRest... next) const
{
_logger->warn(current, std::forward<WRest>(next)...);
}
template<typename D, typename... DRest> void logDebug(D current, DRest... next) const
{
_logger->debug(current, std::forward<DRest>(next)...);
}

One way to get around this issue is by wrapping the argument we want to pass in fmt::runtime(). Here is an example with logInfo():

template<typename I, typename... IRest> void logInfo(I current, IRest... next) const
{
    _logger->info(fmt::runtime(current), std::forward<IRest>(next)...);
}

Another option that can be explored is to have spdlog use C++'s std::format() by setting SPDLOG_USE_STD_FORMAT, it is to be noted that in the current release of spdlog, it is currently marked as experimental.

NovelRT/Graphics/IGraphicsMemoryRegionCollection

Right now something strange is going on within IGraphicsMemoryRegionCollection::DefaultMetadata::Clear(). with both clang and MSVC it currently compiles just fine on C++ 17, however when switching to C++ 20 MSVC will have issues with GetDevice(), it is no longer able to access it as expected. You will find that during building it will say a call of a non-static member function requires an object.

void Clear() final
{
size_t size = GetSize();
_freeRegionCount = 1;
_totalFreeRegionSize = size;
_regions->clear();
GraphicsMemoryRegion<TSelf> region(1, _collection, GetDevice(), false, 0, size);
_regions->emplace_front(region);
auto iterator = _regions->begin();
_freeRegionsBySize->clear();
_freeRegionsBySize->emplace_back(iterator);
assert(Validate());
}

It seems like a compiler bug and we will most likely have to wait until it is fixed unless we want to use the work around mentioned above.

After talk in the discord it seems like a template resolution issue and might be able to be worked around.

JsonCons

JsonCons seems to be causing trouble when compiling with C++ 20. After a bit of talk in Discord, it seems that a good option would be to replace it with nlohmann json, as it is already being used for Fabulist and seems to compile just fine for C++ 20.


With that out of the way, these are the current pain points I've found so far. I am not sure if this list is complete, but after applying the changes for Fabulist and NovelRT/LoggingService, I was able to build the project with C++ 20 with clang on Linux.

@Pheubel
Copy link
Contributor Author

Pheubel commented Nov 20, 2022

spdlog came out with a new release a couple of days ago, with it it should be able to get rid of the compilation error with the logger.

after a quick check, the original solution seems best for now.

@RubyNova
Copy link
Member

I think we need to figure some of this out a little more.

Since this seems like it'd break our steam runtime linux target, I think we need a plan going forward.

There's definitely more high prio stuff we could be doing as well to be fair.

Thoughts are appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants