Simple game engine #119
Simple game engine #119
Conversation
…y commit to start getting proofreading while I still iterate on getting the engine to work. Engine still needs assets. Engine still needs Android project created. Engine currently crashes on running. Add `SimpleEngine` with Entity-Component-System (ECS) implementation - Introduced the `SimpleEngine` project featuring a modular ECS-based architecture. - Implemented core files: `Entity`, `Component`, and `TransformComponent` for managing entities and transformations. - Added CMake setup with Vulkan, GLFW, GLM, TinyGLTF, and KTX dependencies for rendering support. - Integrated shader compilation workflow using `slangc`. - Included initial scene setup with basic camera and cube entities in `main.cpp`.
…mmendations - Consolidated sections to address vendor-agnostic optimizations for mobile GPUs. - Replaced Huawei-specific details with broader guidance applicable to various mobile GPU architectures (Mali, Adreno, PowerVR, etc.). - Improved Vulkan extension examples and added checks for multiple vendor IDs for better compatibility. - Updated best practices and performance tuning sections to emphasize device diversity and testing requirements.
…n details - Simplified and reorganized GPU optimization lists for readability. - Expanded explanations for `VK_KHR_dynamic_rendering_local_read` and `VK_EXT_shader_tile_image` to highlight memory bandwidth reduction benefits. - Improved consistency in vendor-specific and tile-based optimization recommendations. - Added detailed examples and practical use cases for extension advantages in mobile rendering pipelines.
- Expanded tutorial with a comprehensive explanation of rendergraphs, including resource management, dependency tracking, and synchronization benefits. - Added a sample `Rendergraph` class implementation with methods for resource and pass management, including compile and execute functions. - Included practical examples such as deferred renderer setup and Vulkan synchronization best practices using semaphores, fences, and pipeline barriers. - Enhanced Vulkan tutorial content with a focus on simplifying complex rendering workflows and optimizing synchronization.
- Inserted new diagrams for layered architecture, component-based architecture, data-oriented design, and service locator pattern in the engine tutorial. - Adjusted and reordered content in corresponding sections for improved clarity and structure. - Updated references to foundational concepts, ensuring consistency across sections.
- Included a detailed clarification about windowing libraries and their purpose. - Provided examples of popular windowing libraries like GLFW, SDL, Qt, and SFML. - Enhanced understanding of platform-specific abstraction benefits for developers.
…i needs some help. it also looks like cleanup logic and resizing needs work.
…the semaphore. resizing still doesn't work. Next step is to get the parts of the engine to demonstrate things.
…the semaphore. resizing still doesn't work. Next step is to get the parts of the engine to demonstrate things.
…U processing mode.
…nk that's due to material only looking at the vespa. This includes camera controls.
|
While it does compile and run with above adjustments, it doesn't work properly. This is how it looks for me on Win11 + with NV RTX 4070: 
Looks like it can't load any of the images of that model. Looking at the folder where the bistro scene gets downloaded, I can't see a single KTX2 file? |
|
That explains it. looks like the KTX2 file being missing and it's using the hand coded default files. That's the root cause of the no colors. The audio, select default ping and hit play. You should hear something. I don't think I checked in the wav I've been using for debug as I don't think that's a good one to use. Validation is clean here. give me a sample of them. frame rate here is around 16fps. The intention is to have culling be part of the samples version of the game engine but that wasn't introduced here for this one so while I want to do things like improve the loading speed of the model, I'm trying to hold off and keep it as simple as possible yet be something we can build off of for the samples version. I chose optimization as a topic for the Samples version when it comes to getting it to work in Android / iOS. |
|
Grab the latest of the bistro and you should have textures. |
|
@SaschaWillems if you want, the changes you did for Windows, please make a PR against my branch here and I'll merge in your changes. That way we can keep a history and enable any further help you might be willing to give. |
|
Textures now work. I guess the validation errors were caused by them missing, though I di get descriptor freeing related errors when resizing. But lighting looks off when starting the app: 
The basic lighting setting looks better, but the sun position slider doesn't do anything. IMO optimizations are a must for this. It's doing ~12fps for me in fullscreen mode. I don't think we should leave it at that. |
|
For reference, here is the Bistro scene in one of my renderers: 
That renderer doesn't cull either, but you can see some visual differences like normal mapping, discard for alpha-masked objects, etc. |
|
For what it's worth, the model you're using is slightly different. You're using a version of the bistro that's a lot simpler in that it doesn't come from the NVIDIA one that opens up the bistro and lets you go inside. There's also other differences. But I think a key one is the blender specified lumans value which does make the scene too bright during the noon day. Yes, there's still plenty of bugs in the rendering that I'm still trying to work out. But I'm still not entirely sure how much of it is due to rendering bugs from the punctual light source (sun) vs not. And the time of day slider is more for debugging the PBR pipeline. I'm still struggling with a few things in it. The phong version doesn't have the slider support because I haven't found a need to validate it there yet, but maybe it makes sense to keep the slider as it is pretty neat and add support to the phong path. |
Main issue was not treating Spec-Gloss extension correctly.
However, still have less than optimal treatment of the glassware: |
|
|
||
| // Call the resize callback if set | ||
| if (platform->resizeCallback) { | ||
| platform->resizeCallback(platform->width, platform->height); |
There was a problem hiding this comment.
The code for APP_CMD_INIT_WINDOW and APP_CMD_WINDOW_RESIZED are identical?
|
|
||
| bool AndroidPlatform::HasWindowResized() { | ||
| bool resized = windowResized; | ||
| windowResized = false; |
There was a problem hiding this comment.
Is it correct that a query (HasWindowResized) is not const and potentially modifies the queried state?
|
|
||
| void AndroidPlatform::SetWindowTitle(const std::string& title) { | ||
| // No-op on Android - mobile apps don't have window titles | ||
| (void)title; // Suppress unused parameter warning |
There was a problem hiding this comment.
Use [[maybe_unused]] instead?
| env->DeleteLocalRef(serviceStr); | ||
|
|
||
| // Check Vulkan support | ||
| // In a real implementation, this would check for Vulkan support and available extensions |
There was a problem hiding this comment.
And in this implementation, you don't do that because of?
| * @brief Get the Vulkan RAII device. | ||
| * @return The Vulkan RAII device. | ||
| */ | ||
| const vk::raii::Device& GetRaiiDevice() const { return device; } |
There was a problem hiding this comment.
Is it worth to have both, GetDevice() and GetRAIIDevice()?
Wouldn't getting the vk::raii::Device const & be sufficient, as you get to the corresponding vk::Device that easy?
| * @param commandBuffer The command buffer to submit. | ||
| * @param fence The fence to signal when the operation completes. | ||
| */ | ||
| void SubmitToComputeQueue(vk::CommandBuffer commandBuffer, vk::Fence fence) const { |
There was a problem hiding this comment.
To keep the API consistent, you could pass in a vk::raii::CommandBuffer const & and a vk::raii::Fence const & here.
| * @param oldLayout The old layout. | ||
| * @param newLayout The new layout. | ||
| */ | ||
| void TransitionImageLayout(vk::Image image, vk::Format format, vk::ImageLayout oldLayout, vk::ImageLayout newLayout) { |
There was a problem hiding this comment.
Pass in a vk::raii::Image const & here?
There was a problem hiding this comment.
In this file, some functions get/return a vk::Handle, some get/return a vk::raii::Handle.
Maybe it would be more consistent to always use a vk::raii::Handle.
| std::vector<vk::DescriptorSet> descriptorSetsToBindRaw; | ||
| descriptorSetsToBindRaw.reserve(1); | ||
| descriptorSetsToBindRaw.push_back(*computeDescriptorSets[0]); | ||
| commandBufferRaii.bindDescriptorSets(vk::PipelineBindPoint::eCompute, *computePipelineLayout, 0, descriptorSetsToBindRaw, {}); |
There was a problem hiding this comment.
No need to copy stuff into a one-element vector.
You could just call
commandBufferRaii.bindDescriptorSets(vk::PipelineBindPoint::eCompute, *computePipelineLayout, 0, *computeDescriptorSets[0], {});
| // Extract raw command buffer for submission and release RAII ownership | ||
| // This prevents premature destruction while preserving the recorded commands | ||
| vk::CommandBuffer rawCommandBuffer = *commandBufferRaii; | ||
| commandBufferRaii.release(); // Release RAII ownership to prevent destruction |
There was a problem hiding this comment.
Where will the rawCommandBuffer be destroyed, then?
…lkan queue handling. Bistro.gltf loads all textures, and creates physics geometry in under 44 seconds. Seeing 55 fps as well.
|
Looking better now, but the image is very noisy. I'm not sure where these come from, but I can see a lot of artifacts like these, changing when moving around: 
And it looks like there is no mip mapping, 
Update: This seems to be caused by over-saturation. Changing lighting so the affected surfaces get darker, the artifacts go away. |
|
A code related question, that Andreas might've already asked (hard to see, github hides older comments): What is the idea behind converting (Vulkan-hpp) exceptions into bools? See e.g. Since this is exclusively using Vulkan-hpp and C++, why not stick to exceptions. So in above case, try, catch, log inside the function and raise the exception up to the calling code. |
| if (LoadKTX2FileToRGBA(filePath, data, w, h, c) && renderer->LoadTextureFromMemory(textureId, data.data(), w, h, c)) { | ||
| material->albedoTexturePath = textureId; | ||
| } | ||
| renderer->LoadTextureAsync(filePath); |
There was a problem hiding this comment.
No need to LoadKTX2FileToRGBA anymore?
| std::vector<uint8_t> data; int w=0,h=0,c=0; | ||
| std::string filePath = baseTexturePath + image.uri; | ||
| if (LoadKTX2FileToRGBA(filePath, data, w, h, c) && renderer->LoadTextureFromMemory(textureId, data.data(), w, h, c)) { | ||
| if (LoadKTX2FileToRGBA(filePath, data, w, h, c)) { |
There was a problem hiding this comment.
Here, you sequentially LoadKTX2FileToRGBA and then copy the loaded data in-memory in LoadTextureFromMemoryAsync. Both is done in the main thread, before anything asynchronously is done.
Maybe you should LoadKTX2FileToRGBA asynchronously as well?
... it seems, in the code below, you're already doing that.
| CleanupMarkedBodies(); | ||
| } | ||
|
|
||
| void PhysicsSystem::EnqueueRigidBodyCreation(Entity* entity, |
There was a problem hiding this comment.
Maybe add some comment, that entity is supposed to be valid. Here, it's kept in the Engine::entityMap.
| // Texture aliasing: map canonical IDs to actual loaded keys (e.g., file paths) to avoid duplicates | ||
| inline void RegisterTextureAlias(const std::string& aliasId, const std::string& targetId) { | ||
| std::unique_lock<std::shared_mutex> lock(textureResourcesMutex); | ||
| if (aliasId.empty() || targetId.empty()) return; |
There was a problem hiding this comment.
No need to lock when aliasID or targetID is empty
| if (aliasId.empty() || targetId.empty()) return; | ||
| // Resolve targetId without re-locking by walking the alias map directly | ||
| std::string resolved = targetId; | ||
| for (int i = 0; i < 8; ++i) { |
| resolved = it->second; | ||
| } | ||
| if (aliasId == resolved) { | ||
| textureAliases.erase(aliasId); |
There was a problem hiding this comment.
In a function named RegisterTextureAlias, I would not expect something to be erased.
What does that mean?
| inline std::string ResolveTextureId(const std::string& id) const { | ||
| std::shared_lock<std::shared_mutex> lock(textureResourcesMutex); | ||
| std::string cur = id; | ||
| for (int i = 0; i < 8; ++i) { // prevent pathological cycles |
There was a problem hiding this comment.
What "pathological cycles" do you want to prevent here?
| // Initialize background thread pool for async tasks (textures, etc.) AFTER all Vulkan resources are ready | ||
| try { | ||
| // Size the thread pool based on hardware concurrency, clamped to a sensible range | ||
| unsigned int hw = std::max(2u, std::min(8u, std::thread::hardware_concurrency() ? std::thread::hardware_concurrency() : 4u)); |
There was a problem hiding this comment.
std::max( ..., std::min(...)) -> std::clamp?
| return true; | ||
| } | ||
|
|
||
| void Renderer::ensureThreadLocalVulkanInit() const { |
There was a problem hiding this comment.
Assuming, you're using vk::raii-objects only, no vk-objects, you don't need to initialize the VULKAN_HPP_DEFAULT_DISPATCHER at all. The vk::raii-objects hold their own (device-specific) dispatcher.
| .semaphoreType = vk::SemaphoreType::eTimeline, | ||
| .initialValue = 0 | ||
| }; | ||
| vk::SemaphoreCreateInfo timelineCreateInfo{ .pNext = &typeInfo }; |
There was a problem hiding this comment.
Maybe:
vk::StructureChain<vk::SemaphoreCreateInfo, vk::SemaphoreTypeCreateInfo> timelineChain( {}, { .semaphoreType = vk::SemaphoreType::eTimeline } );
| #include <ranges> | ||
| #include <thread> | ||
|
|
||
| VULKAN_HPP_DEFAULT_DISPATCH_LOADER_DYNAMIC_STORAGE; // In a .cpp file |
There was a problem hiding this comment.
With vk::raii, this should not be needed.
| #include <vulkan/vk_platform.h> | ||
|
|
||
| // Debug callback for vk::raii | ||
| static VKAPI_ATTR VkBool32 VKAPI_CALL debugCallbackVkRaii( |
There was a problem hiding this comment.
Maybe return a vk::Bool32 here.
|
|
||
| // Create the lighting pipeline | ||
| if (!createLightingPipeline()) { | ||
| std::cerr << "Failed to create lighting pipeline" << std::endl; |
There was a problem hiding this comment.
Why do you have a specific error message on some steps, but not on others?
| resources.pbrDescriptorSets.clear(); | ||
| resources.uniformBuffers.clear(); | ||
| resources.uniformBufferAllocations.clear(); | ||
| resources.uniformBuffersMapped.clear(); |
There was a problem hiding this comment.
Why do you explicitly clear all those resources? Aren't they cleared automatically?
| std::cout << "Adding optional extension: " << optionalExt << std::endl; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe replace the inner loop with something like
if ( std::ranges::any_of( availableExtensions,
[&optionalExt]( auto const & availableExt ) { return strcmp( availableExt.extensionName, optionalExt ) == 0; } ) )
{
deviceExtensions.push_back( optionalExt );
std::cout << "Adding optional extension: " << optionalExt << std::endl;
}
| // Initialize member variable for proper lifetime management | ||
| pbrPipelineRenderingCreateInfo = vk::PipelineRenderingCreateInfo{ | ||
| .sType = vk::StructureType::ePipelineRenderingCreateInfo, | ||
| .pNext = nullptr, |
There was a problem hiding this comment.
.sType again, and .pNext = nullptr
| depthStencilOpaque.depthWriteEnable = VK_TRUE; | ||
|
|
||
| vk::GraphicsPipelineCreateInfo opaquePipelineInfo{ | ||
| .sType = vk::StructureType::eGraphicsPipelineCreateInfo, |
| .sType = vk::StructureType::eGraphicsPipelineCreateInfo, | ||
| .pNext = &pbrPipelineRenderingCreateInfo, | ||
| .flags = vk::PipelineCreateFlags{}, | ||
| .stageCount = 2, |
There was a problem hiding this comment.
.stageCount = static_cast<uint32_t>(shaderStages.size(); ?
| depthStencilBlended.depthWriteEnable = VK_FALSE; | ||
|
|
||
| vk::GraphicsPipelineCreateInfo blendedPipelineInfo{ | ||
| .sType = vk::StructureType::eGraphicsPipelineCreateInfo, |
| } | ||
|
|
||
| // Create a lighting pipeline | ||
| bool Renderer::createLightingPipeline() { |
There was a problem hiding this comment.
Maybe add some comments, highlighting the differences to createGraphicsPipeline?
|
|
||
| // Find queue families | ||
| QueueFamilyIndices indices = findQueueFamilies(physicalDevice); | ||
| uint32_t queueFamilyIndices[] = {indices.graphicsFamily.value(), indices.presentFamily.value()}; |
There was a problem hiding this comment.
std::array<uint32_t,2>?
| // Set sharing mode | ||
| if (indices.graphicsFamily != indices.presentFamily) { | ||
| createInfo.imageSharingMode = vk::SharingMode::eConcurrent; | ||
| createInfo.queueFamilyIndexCount = 2; |
There was a problem hiding this comment.
-> static_cast<uint32_t>(queueFamilyIndices.size())
| .baseArrayLayer = 0, | ||
| .layerCount = 1 | ||
| } | ||
| }; |
There was a problem hiding this comment.
Maybe move the createInfo out of this loop, and just set .image in the loop?
| vk::SemaphoreCreateInfo semaphoreInfo{}; | ||
| vk::FenceCreateInfo fenceInfo{ | ||
| .flags = vk::FenceCreateFlagBits::eSignaled | ||
| }; |
There was a problem hiding this comment.
I would move the fenceInfo closer to the loop where it's used.
| inFlightFences.emplace_back(device, fenceInfo); | ||
| } | ||
|
|
||
| // Ensure uploads timeline semaphore exists (created early in createLogicalDevice) |
There was a problem hiding this comment.
"Ensure..." means, some check or assertion, or what?
| if (modelLoader && entity->GetName().find("_Material_") != std::string::npos) { | ||
| std::string entityName = entity->GetName(); | ||
| size_t tagPos = entityName.find("_Material_"); | ||
| if (tagPos != std::string::npos) { |
There was a problem hiding this comment.
Checking twice for _Material_ ?
| // Extract material name from entity name for any GLTF model entities | ||
| std::string entityName = entity->GetName(); | ||
| size_t tagPos = entityName.find("_Material_"); | ||
| if (tagPos != std::string::npos) { |
There was a problem hiding this comment.
Again checking twice for _Material_ ?
| if (modelLoader && entity->GetName().find("_Material_") != std::string::npos) { | ||
| std::string entityName = entity->GetName(); | ||
| size_t tagPos = entityName.find("_Material_"); | ||
| if (tagPos != std::string::npos) { |
There was a problem hiding this comment.
And a third time this checking sequence on _Material_. Maybe put everything in those three blocks into one?
| uint32_t instanceCount = static_cast<uint32_t>(std::max(1u, static_cast<uint32_t>(meshComponent->GetInstanceCount()))); | ||
| commandBuffers[currentFrame].drawIndexed(meshIt->second.indexCount, instanceCount, 0, 0, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
The opaque and the blended pass are at least very similar and lengthy. Maybe it's worth to use some helper function, instead?
| framebufferResized = true; | ||
| } | ||
|
|
||
| if (result.first == vk::Result::eErrorOutOfDateKHR || result.first == vk::Result::eSuboptimalKHR || framebufferResized) { |
There was a problem hiding this comment.
Again: eErrorOutOfDataKHR won't be provided via result.first, but by an exception.
|
|
||
| [source,cpp] | ||
| ---- | ||
| class MobileOptimizedEngine { |
There was a problem hiding this comment.
Can the simple engine be built for Android as it is?
There was a problem hiding this comment.
No not yet. That's going to come from the samples version.
| #include <cstring> | ||
| #include <functional> | ||
|
|
||
| // stb_image dependency removed; all GLTF textures are uploaded via memory path from ModelLoader. |
There was a problem hiding this comment.
Is this comment worth to have?
After all, this file is added right here, so nobody knows about its potential past.
| ); | ||
|
|
||
| depthImage = std::move(depthImg); | ||
| depthImageAllocation = std::move(depthImgAllocation); |
There was a problem hiding this comment.
Why do you introduce those local variables depthImg and depthImgAllocation?
You could directly use std::tie( depthImage, depthImageAllocation ) = ...
| } | ||
| } | ||
|
|
||
| // Resolve on-disk path (may differ from logical ID) |
There was a problem hiding this comment.
Does this comment belong to line 69?
| } | ||
|
|
||
| // Resolve on-disk path (may differ from logical ID) | ||
| std::string resolvedPath = textureId; |
There was a problem hiding this comment.
I don't get the difference between resolvedPath and textureId.
| if (it2 != textureResources.end()) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
With this check here, is the check at the top of the function needed?
| } | ||
| } | ||
| descriptor_path_resolved: ; | ||
| } |
There was a problem hiding this comment.
Three identical fallback-pathes... can this be organized differently to maybe have just one?
| std::cerr << "Failed to create descriptor sets: " << e.what() << std::endl; | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Wow, nearly 400 lines of code for a single function!
Maybe it's worth to split that into smaller chunks that are easier to digest?
| if (!baseColor.empty()) { | ||
| texturePath = baseColor; | ||
| } | ||
| } |
There was a problem hiding this comment.
Just assign texturePath = meshComponent->GetBaseColorTexturePath()?
If it's also empty, it doesn't change anything.
|
|
||
| // Align allocation size to nonCoherentAtomSize (64 bytes) to prevent validation errors | ||
| // VUID-VkMappedMemoryRange-size-01390 requires memory flush sizes to be multiples of nonCoherentAtomSize | ||
| const vk::DeviceSize nonCoherentAtomSize = 64; // Typical value, should query from device properties |
There was a problem hiding this comment.
If you should query this, why aren't you querying it?
| // Only update PBR descriptor sets (they have light buffer bindings) | ||
| if (!resources.pbrDescriptorSets.empty()) { | ||
| for (size_t i = 0; i < resources.pbrDescriptorSets.size() && i < lightStorageBuffers.size(); ++i) { | ||
| if (i < lightStorageBuffers.size() && *lightStorageBuffers[i].buffer) { |
There was a problem hiding this comment.
You already checked for i < lightStorageBuffers.size() one line above.
| } else if (tiling == vk::ImageTiling::eOptimal && (props.optimalTilingFeatures & features) == features) { | ||
| return format; | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe:
auto formatIt = std::ranges::find_if( candidates,
[&physicalDevice, &tiling, &features]( vk::Format const & format )
{
vk::FormatProperties props = physicalDevice.getFormatProperties( format );
return ( tiling == vk::ImageTiling::eLinear && ( props.linearTilingFeatures & features ) == features ) ||
( tiling == vk::ImageTiling::eOptimal && ( props.optimalTilingFeatures & features ) == features );
} );
if ( formatIt != candidates.end() )
{
return *formatIt;
}
| } catch (const std::exception& e) { | ||
| std::cerr << "Failed to find supported depth format, falling back to D32_SFLOAT: " << e.what() << std::endl; | ||
| // Fallback to D32_SFLOAT which is widely supported | ||
| return vk::Format::eD32Sfloat; |
There was a problem hiding this comment.
This would mean, eD32Sfloat without support of eDepthStencilAttachment.
Is that reasonable?
| } | ||
|
|
||
| // If not found, return first available format | ||
| return availableFormats[0]; |
There was a problem hiding this comment.
Maybe add an assert( !availableFormats.empty() );
| } | ||
|
|
||
| // If not found, return FIFO mode (guaranteed to be available) | ||
| return vk::PresentModeKHR::eFifo; |
There was a problem hiding this comment.
Maybe:
auto modeIt = std::ranges::find( availablePresentModes, vk::PresentModeKHR::eMailbox );
return modeIt != availablePresentModes.end() ? *modeIt : vk::PresentModeKHR::eFiFo;
|
|
||
| void Resource::Unload() { | ||
| loaded = false; | ||
| } |
There was a problem hiding this comment.
Are the Load and Unload functions complete?
| return; | ||
| } | ||
| // Ensure loading flag is cleared on any exit from this function | ||
| struct LoadingGuard { Renderer* r; ~LoadingGuard(){ if (r) r->SetLoading(false); } } loadingGuard{renderer}; |
There was a problem hiding this comment.
No need to if (r), as that case was caught above.
| light.position = glm::vec3(worldPos); | ||
|
|
||
| // Also transform the light direction (for directional lights) | ||
| glm::mat3 normalMatrix = glm::mat3(glm::transpose(glm::inverse(transformMatrix))); |
There was a problem hiding this comment.
Maybe move the calculation of the normalMatrix out of the loop.
| return; | ||
| } | ||
|
|
||
| int entitiesCreated = 0; |
There was a problem hiding this comment.
What is this entitiesCreated for?
| glm::vec3 scale = {1.0f, 1.0f, 1.0f}; | ||
|
|
||
| glm::mat4 modelMatrix = glm::mat4(1.0f); | ||
| bool matrixDirty = true; |
There was a problem hiding this comment.
Wouldn't glm::mat4(1.0f) fit is exactly to the position/rotation/scale above?
Then it's not dirty.
| * @param eulerAngles The rotation to apply in radians. | ||
| */ | ||
| void Rotate(const glm::vec3& eulerAngles) { | ||
| rotation += eulerAngles; |
There was a problem hiding this comment.
Euler angles can't be concatenated this way.
|
|
||
| // Enable required features | ||
| auto features = physicalDevice.getFeatures2(); | ||
| features.features.samplerAnisotropy = vk::True; |
There was a problem hiding this comment.
You should not get the supported features and use that struct to enable all supported features.
Instead you should use a local features struct, set the required (and supported, as should have been tested in pickPhysicalDevice) features.
| // Enable Vulkan 1.3 features | ||
| vk::PhysicalDeviceVulkan13Features vulkan13Features; | ||
| vulkan13Features.dynamicRendering = vk::True; | ||
| vulkan13Features.synchronization2 = vk::True; |
| vk::PhysicalDeviceVulkan13Features vulkan13Features; | ||
| vulkan13Features.dynamicRendering = vk::True; | ||
| vulkan13Features.synchronization2 = vk::True; | ||
| features.pNext = &vulkan13Features; |
There was a problem hiding this comment.
Maybe:
vk::StructureChain<vk::PhysicalDeviceFeatures2, vk::PhysicalDeviceVulkan13Features> featureChain = {
{.features = {.samplerAnisotropy = true }}, // vk::PhysicalDeviceFeatures2
{.synchronization2 = true, .dynamicRendering = true } // vk::PhysicalDeviceVulkan13Features
};
| * *Scalar Multiplication*: Used for scaling movements and directions | ||
| - Example: Slowing down camera movement by multiplying velocity by a factor < 1 | ||
|
|
||
| * *Dot Product*: Calculates the cosine of the angle between vectors (when normalized) |
There was a problem hiding this comment.
Maybe between vectors (when normalized) -> between normalized vectors
|
|
||
| bool rayIntersectsSphere(const Ray& ray, const Sphere& sphere, float& t) { | ||
| // Vector from ray origin to sphere center | ||
| glm::vec3 oc = ray.origin - sphere.center; |
There was a problem hiding this comment.
The vector from ray origin to sphere center would be sphere.center - ray.origin!
| if (firstMouse) { | ||
| lastX = xpos; | ||
| lastY = ypos; | ||
| firstMouse = false; |
There was a problem hiding this comment.
I assume, you want to start a new rotation from time to time... Where would firstMouse be reset to true?
| if (pitch > 89.0f) | ||
| pitch = 89.0f; | ||
| if (pitch < -89.0f) | ||
| pitch = -89.0f; |
There was a problem hiding this comment.
Maybe: pitch = std::clamp( pitch, -89.0f, 89.0f );
| private: | ||
| // Camera position and orientation | ||
| glm::vec3 position; | ||
| glm::vec3 front; |
There was a problem hiding this comment.
I assume front is the view direction? Or the target position?
Seems to be a little unusual name.
| glm::mat4 getViewMatrix() const; | ||
|
|
||
| // Get projection matrix | ||
| glm::mat4 getProjectionMatrix(float aspectRatio, float nearPlane = 0.1f, float farPlane = 100.0f) const; |
There was a problem hiding this comment.
Why are aspectRatio, nearPlane and farPlane not part of the Camera?
| if (pitch > 89.0f) | ||
| pitch = 89.0f; | ||
| if (pitch < -89.0f) | ||
| pitch = -89.0f; |
There was a problem hiding this comment.
use std::clamp, again?
| // Space and Control provide intuitive up/down movement | ||
| if (glfwGetKey(window, GLFW_KEY_SPACE) == GLFW_PRESS) | ||
| camera.processKeyboard(CameraMovement::UP, deltaTime); // Move up along camera's up vector | ||
| if (glfwGetKey(window, GLFW_KEY_LEFT_CONTROL) == GLFW_PRESS) |
There was a problem hiding this comment.
I'm a bit surprised about this mapping.
While the other five keys are actual character keys, CONTROL is a modifier key.
Is that actually an established convention?
There was a problem hiding this comment.
Another camera_implementation, just more detailed than 03_camera_implementation.adoc?
There was a problem hiding this comment.
Another transformation_matrices, with very few differences to the 03_transfomration_matrices.adoc?
| .size = bufferSize, | ||
| .usage = vk::BufferUsageFlagBits::eUniformBuffer, | ||
| .sharingMode = vk::SharingMode::eExclusive | ||
| }; |
There was a problem hiding this comment.
The bufferInfo could be moved out of the loop.
| * link:01_introduction.adoc[Introduction] | ||
| * link:02_math_foundations.adoc[Mathematical Foundations] | ||
| * link:03_camera_implementation.adoc[Camera Implementation] | ||
| * link:04_transformation_matrices.adoc[Transformation Matrices] |
There was a problem hiding this comment.
You probably wanted to link 03_transformation_matrices.adoc and 04_camera_implementation.adoc, and remove 03_camera_implementation.adoc and 04_transformation_matrices.adoc?
| // Only perform cleanup if resource is currently loaded | ||
| if (IsLoaded()) { | ||
| // Step 3a: Obtain device handle for resource destruction | ||
| vk::Device device = GetDevice(); |
There was a problem hiding this comment.
When you're using functions from a vk::Device, you need to have initialized the defaultDispatcher. If you're using functions from the vk::raii::Device, you don't need that, as that object holds its own dispatcher.
|
|
||
| The metadata caching strategy stores frequently accessed information locally to avoid expensive GPU queries during rendering. These counts are essential for draw calls, where the GPU needs to know exactly how many vertices to process and how many triangles to render, making local storage much more efficient than querying the GPU buffers repeatedly. | ||
|
|
||
| === Mesh Resource — Then: GPU Resource Cleanup and Memory Reclamation |
| // Only proceed with cleanup if resources are currently loaded | ||
| if (IsLoaded()) { | ||
| // Phase 3a: Obtain device handle for resource destruction | ||
| vk::Device device = GetDevice(); |
There was a problem hiding this comment.
As above: it's better to use the vk::raii::Device.
| The buffer creation methods represent some of the most performance-critical code in the mesh resource system, as inefficient GPU memory management can significantly impact rendering performance. Production implementations typically use staging buffers for data upload, implement memory pooling to reduce allocation overhead, and carefully select memory types based on GPU architecture characteristics. | ||
|
|
||
| The device access pattern illustrates a common architectural challenge in resource management systems: balancing convenience with loose coupling. While direct access to global singletons can simplify implementation, production systems typically use dependency injection or service locator patterns to maintain testability and flexibility while providing access to core engine services. | ||
|
|
There was a problem hiding this comment.
Some introduction and formatting commands on Shader Resource Implementation missing?
There was a problem hiding this comment.
Maybe you should pair each vk::DeviceMemory here with some vk::DeviceSize offset, to emphasize the need of some device memory management?
| Modern rendering techniques often require multiple passes, each with a specific purpose. A render pass manager helps organize these passes and their dependencies. | ||
|
|
||
| In this tutorial, we use Vulkan's dynamic rendering feature with vk::raii instead of traditional render passes. Dynamic rendering simplifies the rendering process by allowing us to begin and end rendering operations with a single command, without explicitly creating VkRenderPass and VkFramebuffer objects. The vk::raii namespace provides Resource Acquisition Is Initialization (RAII) wrappers for Vulkan objects, which helps with resource management and makes the code cleaner. Additionally, our engine uses C++20 modules for better code organization, faster compilation times, and improved encapsulation. | ||
|
|
There was a problem hiding this comment.
Wouldn't mentioning C++20 modules belong somewhere into the 01_Overview.adoc or 02_Development_environment.adoc?
And likewise the mentioning of vk::raii?
| class Rendergraph { | ||
| private: | ||
| // Resource description and management structure | ||
| // Represents any GPU resource used during rendering (textures, render targets, buffers) |
There was a problem hiding this comment.
It seems this Resource is very image/texture related. Not "any GPU resource", no render target or buffer.
Maybe should be renamed to ImageResource, or so?
| }; | ||
|
|
||
| // Core data storage for the rendergraph system | ||
| std::unordered_map<std::string, Resource> resources; // All resources referenced in the graph |
There was a problem hiding this comment.
Is the key in resources supposed to be identical to the corresponding Resource::name? Shouldn't Resource::name be removed then?
| // Process input dependencies - this pass must wait for producers | ||
| for (const auto& input : pass.inputs) { | ||
| auto it = resourceWriters.find(input); | ||
| if (it != resourceWriters.end()) { |
There was a problem hiding this comment.
What does it mean, if there's no resourceWriter for this input? Is that an error in the rendergraph?
| } | ||
|
|
||
| // Register output production - subsequent passes may depend on these | ||
| for (const auto& output : pass.outputs) { |
There was a problem hiding this comment.
Registering the pass.outputs here requires all passes are added in order. That is, pass i is not allowed to have outputs from pass j, i < j, as input.
Maybe you should first loop over the passes and gather the outputs into the resourceWriters, and then loop over the passes again to determine the dependencies.
| } | ||
|
|
||
| // Resource access interface for retrieving compiled resources | ||
| Resource* GetResource(const std::string& name) { |
There was a problem hiding this comment.
Shouldn't GetResource be a const function, returning a const pointer?
| vk::DependencyFlagBits::eByRegion, // Region-local dependency | ||
| 0, nullptr, 0, nullptr, 1, &barrier // Image barrier only | ||
| ); | ||
| } |
There was a problem hiding this comment.
Would it also work to fill a std::vector<vk::ImageMemoryBarrier> and call commandBuffer.pipelineBarrier just once? Which one would be more performant?
| [source,cpp] | ||
| ---- | ||
| private: | ||
| uint32_t FindMemoryType(uint32_t typeFilter, vk::MemoryPropertyFlags properties) { |
There was a problem hiding this comment.
How does this source snippet fit to the text above?
|
|
||
| auto it = renderPasses.find(name); | ||
| if (it != renderPasses.end()) { | ||
| return dynamic_cast<T*>(it->second.get()); |
There was a problem hiding this comment.
Again: how does the user know that all of its arguments provided for a new RenderPass are ignored?
|
|
||
| [source,cpp] | ||
| ---- | ||
| // Forward declarations |
There was a problem hiding this comment.
How does this code snippet fit to the text above?
| // Set up dynamic rendering info | ||
| renderingInfo.colorAttachmentCount = 1; | ||
| vk::Format formats[] = { colorFormat }; | ||
| renderingInfo.pColorAttachmentFormats = formats; |
There was a problem hiding this comment.
This looks like a dangling pointer, when the constructor is left.
Why don't you just set
renderingInfo.pColorAttachmentFormats = &colorFormat;
|
|
||
| ImGuiVulkanUtil::~ImGuiVulkanUtil() { | ||
| // Wait for device to finish operations before destroying resources | ||
| if (device) { |
There was a problem hiding this comment.
Can it ever happen that device == nullptr?
|
|
||
| [source,cpp] | ||
| ---- | ||
| void ImGuiVulkanUtil::init(float width, float height) { |
There was a problem hiding this comment.
Why do you separate initialization from construction?
|
|
||
| === Resource Initialization: Font Data Extraction and Memory Calculation | ||
|
|
||
| First extract font atlas data from ImGui and calculates the memory requirements for GPU storage. |
There was a problem hiding this comment.
typo: calculates -> calculate
| indexBuffer.unmap(); | ||
| } | ||
|
|
||
| ==== Begin a rendering scope |
There was a problem hiding this comment.
Code end sign is missing
There was a problem hiding this comment.
Especially in this file, but I've seen it in others as well, you often have code like
vk::SamplerCreateInfo samplerInfo{};
samplerInfo.magFilter = vk::Filter::eLinear; // Smooth scaling when magnified
samplerInfo.minFilter = vk::Filter::eLinear; // Smooth scaling when minified
This is totally valid code, but
- You don't need the empty braced initialization
vk::SamplerCreateInfo samplerInfo{}. Other than the pure C-structs, the vk-structs default-initializes all their members. - Instead of setting the members of a struct one-by-one, I would prefer to use designated initializers almost everywhere.
In some files, you also use this style:
vk::SamplerCreateInfo samplerInfo{};
samplerInfo.setMagFilter(vk::Filter::eLinear); // Smooth scaling when magnified
samplerInfo.setMinFilter(vk::Filter::eLinear); // Smooth scaling when minified
which I would also consider to be inferior to the designated initializers. There are just very few cases, where you get something out of those setter functions. Maybe you should (temporarily) compile with VULKAN_HPP_NO_SETTERS set to catch those cases.
|
|
||
| This is the simplest approach and works well for most applications. With dynamic rendering, the code becomes even cleaner: | ||
|
|
||
| === Command Buffer Initialization |
There was a problem hiding this comment.
Should this and the following section ('til the "Multiple Command Buffers Approach" section) be on level 5, instead of level 3?
| static std::function<void(ImGuiIO&)> inputCallback; | ||
|
|
||
| // Initialization state | ||
| static bool initialized; |
There was a problem hiding this comment.
Maybe use static inline initialization:
inline static vk::raii::Instance * instance = nullptr;
etc.
| return commandBuffer; | ||
| } | ||
|
|
||
| void ImGuiUtil::endSingleTimeCommands(vk::raii::CommandBuffer& commandBuffer) { |
There was a problem hiding this comment.
Are beginSingleTimeCommands and endSingleTimeCommands really specific to ImGuiUtil? Or could they be just some free functions?
| }; | ||
|
|
||
| queue->submit(submitInfo); | ||
| queue->waitIdle(); |
There was a problem hiding this comment.
This needs at least a big and loud comment that you never ever should call queue->waitIdle() (or even device->waitIdle()) in production code.
Other synchronization methods like the fence should be used instead.
|
We might need a different way of capturing all that feedback. With all those discussions, this PR has become so large, that githubh is often having issues loading this for me :/ |
|
I love the feedback and I'm trying my best to keep up. But I agree. I'm getting pretty far behind and it'll take me a bit to even catch up to where everyone else is. Github is having problems with the size of this PR + comments. I'll try to get everything caught up in the next few days. The game engine itself still has some rendering bugs I'm trying to work through as well. |
And I thought, it's just my WLAN that's too slow! |
|
|
||
| This chapter serves as the foundation for understanding how light interacts with different materials in a physically accurate way. The concepts you'll learn here will be applied in later chapters, including the Loading_Models chapter where we'll use this knowledge to render glTF models with PBR materials. | ||
|
|
||
| Throughout our engine implementation, we'll be using vk::raii dynamic rendering and C++20 modules. The vk::raii namespace provides Resource Acquisition Is Initialization (RAII) wrappers for Vulkan objects, which helps with resource management and makes the code cleaner. Dynamic rendering simplifies the rendering process by eliminating the need for explicit render passes and framebuffers. C++20 modules improve code organization, compilation times, and encapsulation compared to traditional header files. |
There was a problem hiding this comment.
I think, I've asked this before: does this paragraph fit into this chapter?
Either way, you need to format C++20 somehow, otherwise the ++ doesn't show up.
| ---- | ||
|
|
||
| Where: | ||
| * albedo is the base color of the surface |
There was a problem hiding this comment.
The bullet points here are not rendered as a list.
There was a problem hiding this comment.
Afair I did already raise that (impossible to find ^^). There needs to be an empty line between the `Where´ and the list for it to be rendererd correctly. Also affects other spots afair.
| ---- | ||
|
|
||
| Where: | ||
| * D is the Normal Distribution Function (NDF) |
There was a problem hiding this comment.
Again, those bullet points are not rendered as a list.
|
|
||
| In this chapter, we'll introduce Physically Based Rendering (PBR) and other lighting models. The concepts we cover here will be applied in later chapters, such as the Loading_Models chapter where we'll use glTF, which uses PBR with the metallic-roughness workflow for its material system. By understanding the theory behind different lighting models, including PBR, we can better leverage the material properties provided by glTF models and extend our rendering capabilities. | ||
|
|
||
| Throughout our engine implementation, we'll be using vk::raii dynamic rendering and C++20 modules. The vk::raii namespace provides Resource Acquisition Is Initialization (RAII) wrappers for Vulkan objects, which helps with resource management and makes the code cleaner. Dynamic rendering simplifies the rendering process by eliminating the need for explicit render passes and framebuffers. C++20 modules improve code organization, compilation times, and encapsulation compared to traditional header files. |
There was a problem hiding this comment.
This very same paragraph was at various other locations already.
Note the missing ++ of C++20 in the rendered text.
| * *Diffuse*: Light scattered in all directions (using Lambert's cosine law) | ||
| * *Specular*: Shiny highlights (using a power function of the reflection vector and view vector) | ||
|
|
||
| * *Advantages*: Reasonably realistic for many materials, intuitive parameters |
There was a problem hiding this comment.
You need to separate those two lists. They're rendered as one list.
| * *Disadvantages*: Not physically accurate, can look artificial under certain lighting conditions | ||
| * *When to use*: For simple real-time applications where PBR is too expensive | ||
|
|
||
| For more information on the Phong lighting model, see the link:https://en.wikipedia.org/wiki/Phong_reflection_model[Wikipedia article]. |
There was a problem hiding this comment.
Maybe you should emphasize the difference between Gouraud interpolation (calculate color at the vertices and interpolate them) and Phong interpolation (interpolate vertices and normals and calculate color on each pixel).
And in addition to that, Phong introduced the more complex reflection model, which could also be used with the Gouraud interpolation.
|
|
||
| // === LIGHTING SETUP === | ||
| // Calculate view direction (camera to fragment) | ||
| float3 V = normalize(ubo.camPos.xyz - input.WorldPos); |
There was a problem hiding this comment.
V is the vector from fragment to camera, not from camera to fragment.
|
|
||
| // Calculate specular BRDF | ||
| float3 numerator = D * G * F; | ||
| float denominator = 4.0 * NdotV * NdotL + 0.0001; // Prevent division by zero |
There was a problem hiding this comment.
To prevent division by zero, wouldn't some max be the better choice?
float denominator = max( 4.0 * NdotV * NdotL, 0.0001 );
| * Reinhard with exposure: Multiply color by an exposure before compressing to shift middle gray. | ||
| - Exposure parameter (ubo.exposure): Conceptually shifts scene brightness so midtones sit well under your chosen tone mapper. Even if the snippet shows a fixed operator, you can pre-scale color by exposure to support dynamic auto-exposure. | ||
| - Gamma correction (ubo.gamma): Displays are non-linear (approx 2.2). Lighting must happen in linear space, then we apply pow(color, 1/gamma) right before writing to the sRGB framebuffer. Skipping this causes washed-out or too-dark images. | ||
| - Pipeline note: Prefer sRGB formats for color attachments when presenting. If writing to an sRGB swapchain image, do gamma in shader OR use sRGB formats so hardware handles it — not both. Do exactly one. |
There was a problem hiding this comment.
I think, I don't get this sentence. What exactly is the difference between the two cases described here? Both are dealing with sRGB?
|
|
||
| // Copy the uniform buffer object to the device memory using vk::raii | ||
| // With vk::raii, we can use the mapped memory directly | ||
| memcpy(uniformBuffers[currentFrame].mapped, &ubo, sizeof(ubo)); |
There was a problem hiding this comment.
Is it vk::raii-specific, that you can use the mapped memory directly?
Or is it because you keep the memory mapped in our Buffer wrapper class?
| 1) Extend the renderer with PBR pipeline objects and a material push-constant block | ||
| 2) Create the PBR pipeline (layout, shaders, blending, formats) alongside the main pipeline | ||
| 3) Record draws: bind PBR pipeline, bind geometry, and push per-material constants per mesh | ||
| 4) Clean up via RAII (no special teardown required) |
There was a problem hiding this comment.
This list is rendered in just one line.
|
|
||
| == Updating the Cleanup | ||
|
|
||
| We also need to update the cleanup process to destroy our PBR pipeline: |
There was a problem hiding this comment.
Well, we don't need to update the cleanup process.
| @@ -0,0 +1,375 @@ | |||
| ::pp: {plus}{plus} | |||
| - *Categorization*: Assets are grouped by type (models, textures, shaders, config) | ||
| - *Hierarchy*: Assets are organized in a nested structure (e.g., models > characters > player) | ||
| - *Discoverability*: Common assets are placed in dedicated folders (e.g., common textures, core shaders) making them easy to find | ||
| - *Scalability*: The structure accommodates different quality levels and platform-specific assets (e.g., high-resolution textures, mobile shaders, quality presets) |
There was a problem hiding this comment.
This list isn't rendered as a list.
|
|
||
| = Loading Models: Asset Pipeline Concepts | ||
| :doctype: book | ||
| :sectnums: |
There was a problem hiding this comment.
This is the first file with sectnums.
Do you want to add that to the other files as well?
| return node; | ||
| } | ||
| } | ||
| return nullptr; |
There was a problem hiding this comment.
Maybe:
auto nodeIt = std::ranges::find_if( linearNodes, [&name]( auto const & node ){ return node.name == name; } );
return ( nodeIt != linearNodes.end() ) ? *nodeIt : nullptr;
| } | ||
|
|
||
| void updateAnimation(uint32_t index, float deltaTime) { | ||
| if (animations.empty() || index >= animations.size()) { |
There was a problem hiding this comment.
Is index >= animations.size() a valid check? Or would it just hide some programmer's error?
In that case, it probably should be replaced by an assertion.
And, by the way: is a public function that updates a specific animation reasonable? Or should it be private and a public function updates all animations.
| } | ||
|
|
||
| for (auto& channel : animation.channels) { | ||
| AnimationSampler& sampler = animation.samplers[channel.samplerIndex]; |
There was a problem hiding this comment.
Maybe add an
assert( channel.samplerIndex < animation.samplers.size() );
| Animation& animation = animations[index]; | ||
| animation.currentTime += deltaTime; | ||
| if (animation.currentTime > animation.end) { | ||
| animation.currentTime = animation.start; |
There was a problem hiding this comment.
The animation always gets to the start. No matter how much deltaTime might have advanced beyond animation.end.
|
|
||
| // Find the current key frame | ||
| for (size_t i = 0; i < sampler.inputs.size() - 1; i++) { | ||
| if (animation.currentTime >= sampler.inputs[i] && animation.currentTime <= sampler.inputs[i + 1]) { |
There was a problem hiding this comment.
Maybe:
auto keyFrameIt = std::ranges::lower_bound( sampler.inputs, animation.currentTime );
assert( keyFrameIt != sampler.inputs.end() );
size_t keyFrameIndex = std::distance( sampler.inputs.begin(), keyFrameIt );
This is the next big tutorial effort. I added the conclusion to the base line tutorial, and this is the "next thing." The game engine presented here is one example of what the tutorial describes and not a 1-1 mapping of the source code to the implementation in the attachments folder.
Future work will add new features to the tutorial in the Vulkan Samples project. This is meant to give the course to describe basics of the game engine and Samples is meant to describe features as they are implemented. This can be used as a template for a starting off point for any project as it is designed to be self contained.
Core features in this tutorial/engine:
Several features are left out intentionally as a method of what we'll implement and feature in the Samples including parallel loading of the assets. Bistro takes a few moments to load as it stands here.