search

Tom94 / tev

High dynamic range (HDR) image viewer for graphics people
BSD 3-Clause "New" or "Revised" License
1.02k stars 86 forks source link

C++20 usage & other improvements #134

Closed Tom94 closed 2 years ago

Tom94 commented 2 years ago

By using C++20 coroutines, it becomes much easier to effectively utilize tev's threadpool, leading to 2-3x faster parallel loading of large numbers of images.

Also starts using C++20 features in a few minor other places

Recent other improvements to tev were developed on top of this branch. These include

This PR will stay unmerged until C++20 support became more widespread.

Tom94 commented 2 years ago

GitHub workflows finally support C++20 on all operating systems, so CI is finally going through! :)

Still, this PR will remain unmerged until C++20-compatible compilers become more widespread. You can track this branch rather than master to follow along with tev's development till then.

Tom94 commented 2 years ago

Merging this now. In case you would like to compile tev and have no way to install a C++20-capable compiler, you can check out the tag cpp17. (https://github.com/Tom94/tev/tree/cpp17)

diiigle commented 2 years ago

Disclaimer: Fortunately I'm not a production person and not entirely concerned with backwards compatibility issues. But I still care enough to bring this up here.

All in all, its a bit of a pickle switching to such a 'new' standard. I guess to the fast-moving research community (which this software is probably mostly targeting) this shouldn't pose a problem, but there might be others who can't upgrade that easily. I'm thinking about the rendering people in production, those who might be bound to the VFX reference platform.

Unfortunately it has to this date still no (full) cpp20 compatible compiler. GCC 9.3.1 doesn't seem to support coroutines with this version yet, but lets the CMAKE_CXX_STANDARD_REQUIRED check pass, because it already supports some features of c++20. (aswf has docker images for the platform in case you want to add it into your CI workflow)

I understand you don't want to maintain a pure c++17 branch in addition to the cpp20 (master now) branch. But leaving c++17 users without some features but most importantly bugfixes might also be problematic.

Possibly a solution to this problem would be to provide Linux binaries. I know this is a pain in itself which is why you haven't done so in the past, but I did a little research recently, and it seems like with AppImage or Flatpak its getting quite good nowadays. One of the two can also package system libraries (glibc) such that you become truly independent of the executing host system (forgot which one 😺). I think that preparing the packaging infrastructure would also attract some package maintainers of various Linux distributions, because it'll make their job much easier. CMake (INSTALL) should be of great help with preparing the right folder structure already, then it's only about figuring out the parameters for the packaging tool.

Tom94 commented 2 years ago

I would love to provide Linux binaries, actually! Let me open an issue that tracks this.

(RE switching to C++20 this early: normally I would completely avoid it for the very same reasons that you quote. Hard agree with all of them. The reason I still pulled the trigger is that coroutines allow very clean code to extract much better parallelism when loading many images, yielding upward of 2x faster startup times. This was too much for me to ignore -- without wanting to turn the existing code into callback hell or spawning hundreds of threads to achieve the same behavior.)