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

Fix examples to compile in MSVC #45

Merged
merged 2 commits into from
Jun 6, 2022
Merged

Conversation

Admer456
Copy link
Contributor

@Admer456 Admer456 commented May 5, 2022

I recently cloned Vookoo to try it out and learn a little bit of Vulkan, and I had issues compiling it in Visual Studio 2022 / MSVC v143.

The first issue was that the examples were using designated initialisers; a C++20 feature, but their CMakeLists configured them to use C++11.
After fixing that, there were a couple of errors with using designated initialisers themselves (error C7562: designated initialization can only be used to initialize aggregate class types).
Finally, the multi-threaded example was doing the following:

  // define number of threads
  const uint32_t Nthreads = std::thread::hardware_concurrency();
  std::cout << "Nthreads = " << Nthreads << std::endl;

  // allocate array of threads to be reused from frame to frame
  std::thread v[Nthreads];

Arrays like these are not allowed in MSVC, so I've changed it to std::vector<std::thread> v( Nthreads );. After successful compilation, I tested all the examples on my main PC with an RTX 3060 and they work without any runtime errors, except for tripping some Vulkan validation layers (primarily about this one), but my assumption is that this is "normal".

@lhog
Copy link
Collaborator

lhog commented May 10, 2022

  // define number of threads
  const uint32_t Nthreads = std::thread::hardware_concurrency();
  std::cout << "Nthreads = " << Nthreads << std::endl;

  // allocate array of threads to be reused from frame to frame
  std::thread v[Nthreads];

Doesn't look correct indeed, worth fixing for sure.

Designated initializers used to work on prev generations of MSVC certainly, but you're right they are cpp20 class semantics.
I see you bumped up C++ standard version to 20, yet removed designated initializers. Why is that?

@Admer456
Copy link
Contributor Author

Now that I think of it, it must be an accidental leftover. When I initially tried building the examples, as mentioned, they were in C++11, which didn't support designated initialisers. That was the reason I changed it to C++20.

Then I encountered a different error about certain classes not being aggregate types. Apparently, if a class has user-defined constructors, it may not be initialised with designated initialisers. That was the reason I ultimately removed them from the examples.

After that, I guess I simply forgot to change it back to C++11. I'll correct this tomorrow.

@Admer456
Copy link
Contributor Author

I have just checked and setting it back to C++11 will result in errors.
In helloTriangle.cpp, designated initialisers are used here:

  // This is our triangles.
  const std::vector<Vertex> vertices = {
    {.pos={ 0.5f,  0.5f}, .colour={0.0f, 1.0f, 0.0f}},
    {.pos={-0.5f,  0.5f}, .colour={0.0f, 0.0f, 1.0f}},
    {.pos={ 0.5f, -0.5f}, .colour={1.0f, 0.0f, 0.0f}},

    {.pos={ 0.5f, -0.5f}, .colour={1.0f, 0.0f, 0.0f}},
    {.pos={-0.5f,  0.5f}, .colour={0.0f, 0.0f, 1.0f}},
    {.pos={-0.5f, -0.5f}, .colour={0.0f, 0.0f, 0.0f}},
  };

Which results in: error C7555: use of designated initializers requires at least '/std:c++latest'

With C++11 on, I've tried building the same in MSVC v142 (2019) and I am getting the same error, meanwhile v141 (2017) did not recognise this style of initialisation, resulting in error C2059: syntax error: '.' and a dozen others.
I haven't tested with v140 (2015) and earlier, but I am guessing it'd be similar. Only v142 and above support C++20.

Just in case, I've also tried building this on Linux in GCC 11. Originally, before my changes, it'd report designated initializers cannot be used with a non-aggregate type 'vku::Window'. With my changes, in both C++11 and C++20, it works.

In short, modern MSVC seems rather strict about the use of designated initialisers. Considering all of the above, I think it is alright to keep the C++ standard at 20.
Alternatively, C++11 could be kept if designated initialisers are removed from every example.

@lhog
Copy link
Collaborator

lhog commented May 16, 2022

In short, modern MSVC seems rather strict about the use of designated initialisers. Considering all of the above, I think it is alright to keep the C++ standard at 20.

I think so too.

@lhog
Copy link
Collaborator

lhog commented Jun 2, 2022

Is this ready for merge? Not around the PC to test it.

@Admer456
Copy link
Contributor Author

Admer456 commented Jun 2, 2022

It should be. I'll test again tomorrow to make sure.

@Admer456
Copy link
Contributor Author

Admer456 commented Jun 5, 2022

Apologies for the delay. College can sometimes be a time sink.

I've tested it again, on Windows 10, compiled in x64 under MSVC v142 and v143.

Under MSVC v142, 13-crystalLogo crashes on GIMP_TEXT1_RUN_LENGTH_DECODE, but that is unrelated to Vookoo and my changes. Under MSVC v143, this crash does not occur. Maybe it's some sort of bug in MSVC.

Another, albeit non-critical thing I've noticed is that in Debug mode, in 12-renderToCubemapByMultiview, the background colour of each cubemap face is exactly as it is defined in code, i.e.:
image
Meanwhile in Release mode, it's black:
image
This could be a separate issue though, as it seems to be unrelated to my changes.

Other than that, I've observed no issues. This PR is ready to be merged.

@lhog lhog merged commit c2c5bed into andy-thomason:master Jun 6, 2022
@pocdn
Copy link
Contributor

pocdn commented Aug 1, 2022

In short, modern MSVC seems rather strict about the use of designated initialisers. Considering all of the above, I think it is alright to keep the C++ standard at 20.

I think so too.

Since vookoo is now at cpp 20, could we re-enable all the designated initialisers. The above comment seems to say that MSVC (and gcc/c++20 on my windows/linux machines) all work fine with the prior initializers like

vku::Window window{
.instance = fw.instance(),
.device = device,
.physicalDevice = fw.physicalDevice(),
.graphicsQueueFamilyIndex = fw.graphicsQueueFamilyIndex(),
.window = glfwwindow
};

compared to the present usage as
vku::Window window{
fw.instance(),
device,
fw.physicalDevice(),
fw.graphicsQueueFamilyIndex(),
glfwwindow
};

@lhog
Copy link
Collaborator

lhog commented Aug 2, 2022

Oh well idk. We kinda switched, yet it's still possible to get back to c++17, which is still more commonly used than c++20.

@Admer456
Copy link
Contributor Author

Admer456 commented Aug 2, 2022

Note that even with designated initialisers enabled, I was getting the following error in MSVC:
error C7562: designated initialization can only be used to initialize aggregate class types
And the equivalent sort of thing in GCC.

According to cppreference, an aggregate class type is one which, among other things, doesn't have a user-defined constructor, something that is done in vku::Window for example.

Refer to 3) and 4) on this page: https://en.cppreference.com/w/cpp/language/aggregate_initialization, and the definition of an aggregate. It seems like it would require major changes to Vookoo if you wish to support designated initialisers.

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 this pull request may close these issues.

None yet

3 participants