-
Notifications
You must be signed in to change notification settings - Fork 54
Visual Studio/Ninja Multi-Config Build workflow branch from 109a4f4 #298
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
base: main
Are you sure you want to change the base?
Visual Studio/Ninja Multi-Config Build workflow branch from 109a4f4 #298
Conversation
…d shader compilation
…chapters in Visual Studio generator and Ninja Multi-Config generator
…mments for clarity
…ization by removing dependency on any previous tasks (executable build task or shaders targets). The post-build task will garntee only the copy-if-different of shader/texture/modele files to proper location in case of Visual Studio or Ninja Multi-Config.
|
This is the same intent of PR#165. But I resolve many issues were in that previous PR. I already closed that PR#165 and all purposes in that PR are here, but they apply on latest state of source code in main branch. Please review and give me your feedback and comment if exist. |
gpx1000
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi sorry reviewing took so long the first time. My queue gets quite long and I get unintentionally back logged. I'll try to keep iterating quickly for you.
Thanks for the effort on this. I'll test this across all platforms and ensure it works for Windows, Linux and Android (for the Android chapters). Additionally, we need to think about how this might impact the newly released Game Engine. I don't know if simply adding it as a subproject is the right path forward; but it is certainly a design decision that might work.
The run directory might be a bit confusing to students. I'll post more of a review after I test on all platforms. For now, this is just eyeballing responses. The CMake looks right; so it should work.
Maybe recommend investigating using CPack instead of copying assets over. That might be a more elegant solution and what we use to get the game engine, and the ML tutorial (when that comes) to work with this.
CMakeLists.txt
Outdated
|
|
||
| project (VulkanTutorialRoot) | ||
| # This is to allow the CMakeLists.txt files in attachments/ to be accessed from root level | ||
| # Use command: cmake -S . -B build to configure from root level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't you just be able to cmake -S attachments -B build from the root level and achieve the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi gpx1000
Yes, you can do that too. The idea here is to be able to cmake from root and from attachments directory or any decent directory if it is having CMakeLists.txt. But if you select to cmake from root it will be going into massive configuration stage for all projects under the root. But in cmake build stage I think I will go with selecting the target more than cmake all targets for time saving. Anyway, my recommendation will not break anything except just the notes I provided below.
Here is commands from the root directory
cmake -S attachments -B attachments/build/msvc -DCMAKE_TOOLCHAIN_FILE="$env:VCPKG_ROOT/scripts/buildsystems/vcpkg.cmake" -DVCPKG_MANIFEST_DIR="./"
OR from attachments directory cmake -S . -B build/msvc -DCMAKE_TOOLCHAIN_FILE="$env:VCPKG_ROOT/scripts/buildsystems/vcpkg.cmake" -DVCPKG_MANIFEST_DIR="../"
Also if you want to use root CMakeLists.txt
cmake -S . -B build/msvc -DCMAKE_TOOLCHAIN_FILE="$env:VCPKG_ROOT/scripts/buildsystems/vcpkg.cmake"
CMakeLists.txt
Outdated
| # Need to be carefull using CMAKE_SOURCE_DIR is points to the root level CMakeLists.txt | ||
| # Use CMAKE_CURRENT_SOURCE_DIR to point to the current directory instead | ||
| # For Simple Vulkan Engine project please uncomment the following line | ||
| # add_subdirectory(attachments/simple_engine) No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... I didn't test having both projects setup like this. One that builds the main tutorial and the other that builds the engine. I can't think why it wouldn't work. However, the "working directory" for when you "run" the engine or the tutorial might feel a bit odd.
I see below that the idea is to copy the assets to the directory local to the build folder. That does lead me to believe that it would make the tutorial chapters able to be run from the build directory. I don't think we sanitized the code in the tutorial to allow running from any directory; so I think the build process would be:
cmake -S . -B build
make
cd build
./run.exe
I think for the game engine, that pattern might not be pleasant as the map and assets are quite large. We do provide a script to download the project. Maybe a pattern to enable the game engine to work with this is to include the packaging phase of CMake. Thus, this constraint would have a side benefit of demonstrating how to create an install for a non-trivial Vulkan project. That might feel a bit beyond the scope for the game engine so will ask for feedback on if that's a wise idea.
We'll have to think that through for other Tutorials currently in the works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I did not cmake the engine from the root directory or engine directory. But my setup work with main tutorial at the attachments directory smoothly. For time being I do not suggest doing cmake from root directory for engine because I do not know what is there and what it will be break. But the suggestion in comment is valid and could be implanted very easily any time if you will be carefully following the suggestion I put in the comment. Meanwhile I'm trying to make live easy for tutorial part.
If I don't answer your comment correctly let me know in comment what is exactly your question.
…ture synchronization with directory handling instead of files handling.
…chronization labels and change copy command to copy_directory_if_different
|
Hi gpx1000 |
|
Hi, I'm not near computer right now but check out copy_if_newer. Checking
the timestamp is much faster than the content. Also look at file(copy as
that will copy during the configure stage instead of the build stage.
Configure is only done once, built many times potentially.
Also take a look at using relative links on non Windows platforms to avoid
the copy all together. The win we're seeking is to avoid a long build time
on other platforms for game engine.
…On Sat, Jan 31, 2026, 7:54 AM Euclid Skyline ***@***.***> wrote:
*euclid-skyline* left a comment (KhronosGroup/Vulkan-Tutorial#298)
<#298 (comment)>
Hi gpx1000
I just found an issue with function sync_directory(). It did not work at
first build stage when there is not build directory in Visual Studio and
Ninja Multi-Config. So, I refactor that function and use
"copy_directory_if_different" instead of the file version with little
adjustment and changes. Now everything is work as supposed to be and ready
for testing.
—
Reply to this email directly, view it on GitHub
<#298 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5IAY4PTKKAA6KOCWBEB6L4JTF2XAVCNFSM6AAAAACTOI276WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTQMRYG42DMNZRHA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
I've been thinking about this problem and want you to test something for me. See if this works for you: Do that instead of copying any of the files over. See if it also works in Release mode. Here's my reasoning. If that will work, then we can put that into a if Windows command in your new top level CMakeLists.txt file. That should work for Windows Visual Studio and hopefully VS Code. It probably wouldn't hurt Android Built in Windows. But I'm thinking for Linux and OSX when we get support for that we can simply do this: We can't do that in Windows as Windows requires extra permissions to create symbolic links. However, that's a nice solution idea in the sense that there's never a need to copy any of the files anywhere and the current build setup would work on all platforms without any changes other than the new top level CMakeLists.txt file. The only real problem with that is we'd want to change the working directory and the link for the other Tutorials as they will have different paths; so will need to be a little clever with that. Maybe a function that detects the build target and a custom_command that updates if it's in a white list. What do you think of that idea? |
|
Hi gpx1000, Let me clarify this My method deterministic clear and to the point. Also, the copy happened once and second time just file comparison with copy_directory_if_different. If you want me to use copy_directory_if_newer you have to upgrade the cmake to 4.2. Also, My method do not require symbolic linking in Linux or macOS. I preferer simplicity and easy to understand but up to you If you are welling me to tested. Just answer the questions above to understand how to apply it correctly. |
…ulti-Config generators
|
Hi gpx1000, let you know the last update now I successfully being able to compile the shaders directly to proper directory for multi-configuration build-system such as Visual Studio and Ninja. So now no copy for shaders. |
|
I made you a PR on your branch with what I had in mind. It looks like symlinks in Linux wont work the way I had thought out of the box. We'd need to carefully manage the loading code and make changes in the source itself I think is the reasonable path forward. Simple working directory isn't the best solution on a platform where a lot of people might build and run from the command line without an IDE. |
|
I will review your PR even though I do not have an interest in working with the idea being able to run from IDE but not from OS. Also using symbolic links to connect the executable of each chapter to their required resources (shaders/models/textures) will create unnecessary steps and complicate the development environment on leaners. At the end this a Vulkan tutorial and the purpose here should let leaner feel at home and start leaning Vulkan immediately without complication of development tools. This is my main purpose I'm trying to achieve by submitting my PR. My PR will not break anything exist Single Configuration build-systems leaners will work as it was before, and multi-configuration build-systems leaners will make the live easier for them. Now my PR compile shaders and put result of compilation (no copy) in proper location for multi-configuration build-systems. Also, the time to copy the other two assets textures and models are very negligible almost 2 to 3 seconds. These files are very small and no need to be warried about copying files. I hope you review and test my PR and give it chance. |
|
I did look at your PR. I do like the changes you made to the initial version. However, I'm not as concerned about the shaders as I am about the assets. Yes, on more complicated builds, the shader build times are not as trivial as they are here. Your solution still copies all of the assets, the models and textures. It also doesn't do anything for the Game engine tutorial which has much larger asset requirements than the base line tutorial. That's the fundamental problem. If we were to accept that the top level directory is the main build and run directory, then we need to do something in order to allow for the gigabytes of assets that could need to be transferred. If we want to simply say that the top level directory is the build and run directory for only the base line tutorial, then your current solution will work. However, that's probably a bit confusing as to why the structure changes between the tutorials. Do you have thoughts for how to address these shortcomings you'd prefer? If the goal is to load the tutorial from a top level CMakeLists.txt, we can certainly achieve that; but we're going to need to be clever in order to find a solution that works for everything in a manner that's easy for people to read/understand. |
|
Your concerns are valid. But I'm still not interested of idea working on IDE but not working on OS or introducing the symbolic links as solution. So, I will remove CMakeLists.txt from root directory to make thing work as usual. Currently the build time for complete Vulkan Tutorial took 15 min in release mode. this will add up for simple engine if someone decide to compile both projects. When I started this PR, I was not known about Simple Engine exist and added to Vulkan Tutorial. Just allow me update .gitignore little bit to ignore vcpkg.json in attachments directory so I do not need to use -DVCPKG_MANIFEST_DIR option when I cmake since the command line is already long. |
|
I'm perfectly fine with this PR as you changed it and we can merge as soon as @SaschaWillems or someone aside from myself approves it. However, you did do work and waited for me for a while, and did raise an interesting convenience utility that the Tutorial is missing. It never occurred to me, but maybe we should refactor the tutorial's folder structure so something closer to your solution could work. That would mean this would be the new folder structure: Next, we could set the "working directory" as a CMake Option which would point to the right place depending upon where the CMake is invoked from. Or even simpler, mandate that the normal idea is to build from the root and not encourage people to build from the tutorial directory. I think if we did a variable option to CMake, then treat it as a macro define in the tutorials themselves we could have a very elegant solution where nothing needs to be copied, don't need to make any links and the code itself is just using an absolute path so you could run the application from literally anywhere on all of the platforms (aside from Android; which has different rules). If you're willing, I'd be happy to work with you on coming up with a refacotring solution that would work across all future tutorials and this one. I'd need to review this idea with the rest of the Tutorial maintainers team to see if there's any reason not to do something like this idea. Anyway, I'm giving you my approval for the PR in its' current form. |
This contribution allows to build from project root directory without switching to attachments folder. Also refactor the shaders target functions in attachments directory CMakeLists.txt to fix MSBuild warning messages (MSB8065/MSB8064). Also provide post-build tasks to move the shaders/models/textures to correct executable directory for Visual Studio generator or Ninja Multi-Config generator