Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds Vulkan GPU texture loading/sampling and shader texcoord support, updates descriptor sets and vertex format, adjusts example window size, tweaks application logs/shutdown behavior, initializes a file stream member, standardizes log tags, updates vcpkg manifest/submodules, and marks the shader CLI arg required. Changes
Sequence DiagramsequenceDiagram
participant App
participant RHI as RHI::init()
participant STBI as stbi_load
participant Vulkan as Vulkan API
App->>RHI: init()
RHI->>STBI: stbi_load("textures/SegFault.jpg")
STBI-->>RHI: image data
RHI->>Vulkan: create staging buffer & upload image data
RHI->>Vulkan: create VkImage (textureImage) + allocate memory
RHI->>Vulkan: transitionImageLayout(textureImage, UNDEFINED → TRANSFER_DST)
RHI->>Vulkan: copyBufferToImage(stagingBuffer → textureImage)
RHI->>Vulkan: transitionImageLayout(textureImage, TRANSFER_DST → SHADER_READ_ONLY)
RHI->>Vulkan: create imageView (textureImageView) and create sampler (textureSampler)
RHI->>Vulkan: update descriptor set (binding 1 = image sampler)
RHI-->>App: init complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/core/segfault.h (1)
62-77:⚠️ Potential issue | 🟡 MinorHandle all enum cases in the switch statement.
The pipeline warning indicates that
LogType::InvalidandLogType::Countare not handled in the switch. Add adefaultcase to silence the warning and handle unexpected values gracefully.🛠️ Proposed fix
case LogType::Print: std::cout << msg << std::endl; break; + default: + break; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/core/segfault.h` around lines 62 - 77, The switch in logMessage(LogType type, const char* msg) doesn't handle LogType::Invalid and LogType::Count; add a default case to the switch to handle unexpected enum values (e.g., log as unknown/unexpected or no-op) so the compiler warning is silenced and runtime odd values are handled gracefully; ensure the default provides a clear message or safe behavior consistent with the other cases.src/runtime/core/filearchive.h (1)
18-40:⚠️ Potential issue | 🟠 MajorGuard null stream usage in destructor/read/write.
Line 18 makes
mStreamexplicitly nullable, but Lines 31, 35, and 39 still dereference/use it unconditionally. Ifstream == nullptr(e.g., failedfopen), this can crash.Proposed fix
inline FileArchive::~FileArchive() { - fclose(mStream); + if (mStream != nullptr) { + fclose(mStream); + mStream = nullptr; + } } inline size_t FileArchive::read(uint8_t *buffer, size_t size) { + if (mStream == nullptr || !mCanRead || buffer == nullptr) { + return 0; + } return fread(buffer, 1, size, mStream); } inline size_t FileArchive::write(const uint8_t *buffer, size_t size) { - return fwrite(buffer, size, 1, mStream); + if (mStream == nullptr || !mCanWrite || buffer == nullptr) { + return 0; + } + return fwrite(buffer, 1, size, mStream); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/core/filearchive.h` around lines 18 - 40, FileArchive methods use mStream without null checks: update FileArchive::~FileArchive, FileArchive::read, and FileArchive::write to guard against mStream == nullptr; in the destructor only call fclose(mStream) if mStream is non-null, and in read/write return 0 (or appropriate size_t zero) immediately if mStream is nullptr (and optionally respect mCanRead/mCanWrite by returning 0 if not permitted). Reference the symbols FileArchive::~FileArchive, FileArchive::read, FileArchive::write and the member mStream when making the conditional checks.
🧹 Nitpick comments (1)
src/runtime/renderer/RHIVulkan.cpp (1)
132-187: Keep the new texture helpers aligned with the repo naming rules.The added members and methods use lowerCamelCase (
textureImage,createTextureImage,beginSingleTimeCommands, …) instead of the requiredm+ PascalCase / PascalCase conventions. As per coding guidelines, "Usem+ PascalCase for member variable names (e.g.,mCurrentIndex,mArgument)" and "Use PascalCase for function names (e.g.,getNext(),init())."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/renderer/RHIVulkan.cpp` around lines 132 - 187, The new texture fields and helper methods violate naming rules: rename member variables textureImage, textureImageView, textureSampler, textureImageMemory to mTextureImage, mTextureImageView, mTextureSampler, mTextureImageMemory, and rename functions createTextureImage, createTextureImageView, createTextureSampler, beginSingleTimeCommands, endSingleTimeCommands, transitionImageLayout (both overloads), copyBufferToImage to PascalCase (e.g., CreateTextureImage, CreateTextureImageView, CreateTextureSampler, BeginSingleTimeCommands, EndSingleTimeCommands, TransitionImageLayout, CopyBufferToImage); update all declarations in RHIImpl and every call site to the new identifiers so signatures and references remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/compile_shader.py`:
- Line 51: The CLI argument for '--shader' is declared with both required=True
and a default which conflicts; update the parser.add_argument call for
'--shader' (the line creating the argument in parser.add_argument('--shader',
...)) to keep the backward-compatible default behavior by removing required=True
so the default='./' is used as a fallback, ensuring external callers without the
flag still get the intended path.
In `@src/contrib/vcpkg`:
- Line 1: The vcpkg submodule entry points to a non-existent commit
e7d7451462697d77ef319ddf2da8ff7320a82662 which will break submodule fetch;
update the submodule reference by either changing the commit to a valid commit
from the official Microsoft vcpkg repo or change the submodule URL to the fork
that contains that commit—edit the submodule configuration (e.g., the
.gitmodules entry and the src/contrib/vcpkg submodule pointer) and run git
submodule sync && git submodule update --init --recursive to verify the new
commit/URL resolves.
In `@src/runtime/application/app.cpp`:
- Line 18: The version string in the call to entry.append in
src/runtime/application/app.cpp contains a typo "0.0.l" (lowercase L) — change
that literal to the correct version "0.0.1" in the entry.append("| SegFault
version 0.0.1 inited.\n"); so the logged/printed version is accurate; locate the
entry.append(...) call and update the string accordingly.
- Around line 134-139: The shutdown guard in App::shutdown conflates two
different cases and has a grammar error; change the branching to check mState ==
ModuleState::Shutdown first and return with a corrected message like "App
already shut down." and separately handle mSdlWindow == nullptr (which likely
indicates failed init) by logging an error via logMessage (e.g., "Cannot
shutdown: SDL window not initialized") and returning; update the log text to use
"shut down" instead of "shutdowned" and ensure you reference App::shutdown,
mState, ModuleState::Shutdown, mSdlWindow, and logMessage when making the
changes.
In `@src/runtime/renderer/RHIVulkan.cpp`:
- Around line 10-11: RHIVulkan.cpp now includes STB_IMAGE_IMPLEMENTATION and
<stb_image.h> but the renderer target isn't consuming the stb headers, causing
the compile error; fix by wiring the stb package into the renderer's CMake
target: locate the CMake target that builds src/runtime/renderer/RHIVulkan.cpp
(the renderer target) and either call find_package(stb CONFIG REQUIRED) or
obtain the include dir from vcpkg and add
target_include_directories(<renderer-target> PRIVATE <stb_include_dir>) or
target_link_libraries(<renderer-target> PRIVATE stb::stb) so the compiler can
find stb_image.h, ensuring this change applies before building RHIVulkan.cpp
which relies on STB_IMAGE_IMPLEMENTATION and <stb_image.h>.
- Around line 1133-1161: createTextureSampler() unconditionally enables
anisotropy (samplerInfo.anisotropyEnable = VK_TRUE) while createLogicalDevice()
never enables deviceFeatures.samplerAnisotropy; fix by querying the physical
device features (vkGetPhysicalDeviceFeatures or vkGetPhysicalDeviceProperties as
appropriate) in createLogicalDevice(), set deviceFeatures.samplerAnisotropy =
VK_TRUE when supported and enable it when creating the logical device, and then
in createTextureSampler() gate samplerInfo.anisotropyEnable (and
samplerInfo.maxAnisotropy) on whether that samplerAnisotropy feature was
reported/enabled (store the queried/enabled flag in a member or check the
VkPhysicalDeviceFeatures used to create the device) so anisotropy is only used
when supported.
- Around line 1099-1103: The code calls transitionImageLayout(textureImage,
VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL,
VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL) twice in a row after
copyBufferToImage; remove the duplicate second call so transitionImageLayout is
invoked only once (keeping the initial transition from
UNDEFINED->TRANSFER_DST_OPTIMAL, copyBufferToImage, then the single
TRANSFER_DST_OPTIMAL->SHADER_READ_ONLY_OPTIMAL). Ensure no extra transition
calls remain for textureImage with VK_FORMAT_R8G8B8A8_SRGB that would attempt to
re-transition from TRANSFER_DST_OPTIMAL to SHADER_READ_ONLY_OPTIMAL.
- Around line 1076-1082: The imageSize calculation uses texWidth and texHeight
before verifying stbi_load succeeded; move the line "VkDeviceSize imageSize =
texWidth * texHeight * 4;" to after the null-check of pixels (after the throw)
so it only runs when pixels is non-null, and ensure texWidth/texHeight are
initialized by stbi_load; update the block around stbi_load, pixels, texWidth,
texHeight, imageSize accordingly.
- Around line 1493-1494: Call mImpl->createTextureSampler() immediately after
mImpl->createTextureImageView() during initialization so textureSampler is
actually created; then add a sampler binding
(VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER) to the descriptor set layout
alongside the uniform buffer binding and include a corresponding
VkWriteDescriptorSet that points to the created textureImageView and
textureSampler (i.e., write the combined image sampler into the descriptor set);
finally, ensure the shutdown code that destroys textureSampler (the lines around
the existing vkDestroySampler call) either only runs when textureSampler was
created or simply remains correct because the sampler is now created.
- Around line 1313-1370: In transitionImageLayout() remove the first
vkCmdPipelineBarrier(...) call that passes zero src/dst stage masks (and any
placeholder srcAccessMask/dstAccessMask values) — compute and set
barrier.srcAccessMask, barrier.dstAccessMask, sourceStage and destinationStage
in the existing conditional branches (the ones that set masks for the two
supported transitions) and call vkCmdPipelineBarrier(...) only once after those
branches; keep using beginSingleTimeCommands() and endSingleTimeCommands() as
before and update the barrier struct (image, subresourceRange, etc.) before the
single, valid vkCmdPipelineBarrier call.
---
Outside diff comments:
In `@src/runtime/core/filearchive.h`:
- Around line 18-40: FileArchive methods use mStream without null checks: update
FileArchive::~FileArchive, FileArchive::read, and FileArchive::write to guard
against mStream == nullptr; in the destructor only call fclose(mStream) if
mStream is non-null, and in read/write return 0 (or appropriate size_t zero)
immediately if mStream is nullptr (and optionally respect mCanRead/mCanWrite by
returning 0 if not permitted). Reference the symbols FileArchive::~FileArchive,
FileArchive::read, FileArchive::write and the member mStream when making the
conditional checks.
In `@src/runtime/core/segfault.h`:
- Around line 62-77: The switch in logMessage(LogType type, const char* msg)
doesn't handle LogType::Invalid and LogType::Count; add a default case to the
switch to handle unexpected enum values (e.g., log as unknown/unexpected or
no-op) so the compiler warning is silenced and runtime odd values are handled
gracefully; ensure the default provides a clear message or safe behavior
consistent with the other cases.
---
Nitpick comments:
In `@src/runtime/renderer/RHIVulkan.cpp`:
- Around line 132-187: The new texture fields and helper methods violate naming
rules: rename member variables textureImage, textureImageView, textureSampler,
textureImageMemory to mTextureImage, mTextureImageView, mTextureSampler,
mTextureImageMemory, and rename functions createTextureImage,
createTextureImageView, createTextureSampler, beginSingleTimeCommands,
endSingleTimeCommands, transitionImageLayout (both overloads), copyBufferToImage
to PascalCase (e.g., CreateTextureImage, CreateTextureImageView,
CreateTextureSampler, BeginSingleTimeCommands, EndSingleTimeCommands,
TransitionImageLayout, CopyBufferToImage); update all declarations in RHIImpl
and every call site to the new identifiers so signatures and references remain
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33311300-b03a-4b8f-8e6f-263505823944
⛔ Files ignored due to path filters (1)
assets/textures/SegFault.jpgis excluded by!**/*.jpg
📒 Files selected for processing (8)
scripts/compile_shader.pysrc/contrib/vcpkgsrc/examples/hello_world/hello_world.cppsrc/runtime/application/app.cppsrc/runtime/core/filearchive.hsrc/runtime/core/segfault.hsrc/runtime/renderer/RHIVulkan.cppvcpkg.json
| parser = argparse.ArgumentParser() | ||
| parser.add_argument('--verbose', action='store_true', default=False, help='The full output will be shown') | ||
| parser.add_argument('--shader', type=str, default='./', help='The folder containing the shaders') | ||
| parser.add_argument('--shader', type=str, required=True, default='./', help='The folder containing the shaders') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether repository callsites invoke compile_shader.py without --shader.
# Expected: ideally zero matches in the second query if keeping required=True.
set -euo pipefail
echo "== All compile_shader.py callsites =="
rg -n -C2 'compile_shader\.py' --type-add 'cmake:*.cmake' -tpy -tsh -tyaml -tcmake -ttoml -tmake
echo
echo "== Likely callsites missing --shader on the same line =="
rg -nP -C2 'compile_shader\.py(?!.*--shader)' --type-add 'cmake:*.cmake' -tpy -tsh -tyaml -tcmake -ttoml -tmakeRepository: kimkulling/Segfault
Length of output: 100
--shader is marked required while also given a default (conflicting CLI contract).
At Line 51, required=True makes default='./' effectively unused. Either remove required=True to keep the default as fallback, or keep required=True and remove the default. No callsites in the repository were found that invoke this script, so the fix should maintain intended behavior for external callers.
Suggested diff (keep backward-compatible default behavior)
- parser.add_argument('--shader', type=str, required=True, default='./', help='The folder containing the shaders')
+ parser.add_argument('--shader', type=str, default='./', help='The folder containing the shaders')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parser.add_argument('--shader', type=str, required=True, default='./', help='The folder containing the shaders') | |
| parser.add_argument('--shader', type=str, default='./', help='The folder containing the shaders') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/compile_shader.py` at line 51, The CLI argument for '--shader' is
declared with both required=True and a default which conflicts; update the
parser.add_argument call for '--shader' (the line creating the argument in
parser.add_argument('--shader', ...)) to keep the backward-compatible default
behavior by removing required=True so the default='./' is used as a fallback,
ensuring external callers without the flag still get the intended path.
| @@ -1 +1 @@ | |||
| Subproject commit f75c836a67777a86a2c1116a28b179827f028b66 | |||
| Subproject commit e7d7451462697d77ef319ddf2da8ff7320a82662 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the vcpkg submodule commit exists and gather information about it.
# Check if the commit exists in the vcpkg repository
cd src/contrib/vcpkg 2>/dev/null || echo "Submodule directory not found"
# Verify the commit exists
git cat-file -t e7d7451462697d77ef319ddf2da8ff7320a82662 2>/dev/null || echo "Commit not found in local submodule"
# Get commit details if it exists
git show --no-patch --format="%H%n%an%n%ad%n%s" e7d7451462697d77ef319ddf2da8ff7320a82662 2>/dev/null
# Check the date to see how recent it is
git log --oneline -1 e7d7451462697d77ef319ddf2da8ff7320a82662 2>/dev/nullRepository: kimkulling/Segfault
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Check if .gitmodules exists and contains vcpkg configuration
if [ -f .gitmodules ]; then
echo "=== .gitmodules content ==="
cat .gitmodules
echo ""
fi
# Check git history for the src/contrib/vcpkg file
echo "=== Git log for src/contrib/vcpkg (recent commits) ==="
git log --oneline -n 10 -- src/contrib/vcpkg 2>/dev/null || echo "File not in git history"
# Check current status of src/contrib/vcpkg
echo ""
echo "=== Current content of src/contrib/vcpkg ==="
if [ -f src/contrib/vcpkg ]; then
cat src/contrib/vcpkg
else
echo "File does not exist as regular file"
fi
# Check if it's tracked as a submodule or gitlink
echo ""
echo "=== Git ls-files for src/contrib/vcpkg ==="
git ls-files --stage src/contrib/vcpkg 2>/dev/nullRepository: kimkulling/Segfault
Length of output: 609
🌐 Web query:
vcpkg commit e7d7451462697d77ef319ddf2da8ff7320a82662 github
💡 Result:
No information found about vcpkg commit e7d7451462697d77ef319ddf2da8ff7320a82662 on GitHub or related sources. The commit hash does not appear in the microsoft/vcpkg repository commits, issues, PRs, or other vcpkg documentation. It may be from a fork, private repository, or invalid.
Citations:
- 1: https://github.com/microsoft/vcpkg/commits
- 2: https://github.com/microsoft/vcpkg
- 3: https://github.com/microsoft/vcpkg/commits?after=aa2d37682e3318d93aef87efa7b0e88e81cd3d59+34
- 4: https://github.com/microsoft/vcpkg/commits?before=582a4de14bef91df217f4f49624cf5b2b04bd7ca+280
🏁 Script executed:
#!/bin/bash
# Validate the commit hash format and check git configuration
echo "=== Checking git remote configuration for vcpkg ==="
git config --file .gitmodules --get submodule.src/contrib/vcpkg.url
# Try to list the current branch/history in the submodule reference
echo ""
echo "=== Verify commit hash format ==="
# Check if it's a valid 40-character hex string
COMMIT="e7d7451462697d77ef319ddf2da8ff7320a82662"
if [[ $COMMIT =~ ^[0-9a-f]{40}$ ]]; then
echo "✓ Commit hash format is valid (40 hex characters)"
else
echo "✗ Commit hash format is INVALID"
fi
# Check git log to see when this commit was referenced
echo ""
echo "=== Git log with submodule updates ==="
git log --all --oneline --grep="submodule\|vcpkg" -n 20 2>/dev/null || git log --all --oneline -n 20 -- .gitmodules 2>/dev/null
# Check git show for the merge commit that updated vcpkg
echo ""
echo "=== Details of merge commit cbe81f1 ==="
git show --stat cbe81f1 -- src/contrib/vcpkg 2>/dev/null | head -30Repository: kimkulling/Segfault
Length of output: 573
🏁 Script executed:
#!/bin/bash
# Show the diff for the merge commit that modified vcpkg submodule
echo "=== Git diff for merge commit cbe81f1 ==="
git show cbe81f1 -- src/contrib/vcpkg 2>/dev/null
# Also check the previous commit to see what it was before
echo ""
echo "=== Previous commits on this branch ==="
git log --oneline -n 5 2>/dev/null
# Check if the old commit exists
echo ""
echo "=== Checking if old commit f75c836a67777a86a2c1116a28b179827f028b66 is valid ==="
if [[ "f75c836a67777a86a2c1116a28b179827f028b66" =~ ^[0-9a-f]{40}$ ]]; then
echo "✓ Old commit hash format is valid (40 hex characters)"
else
echo "✗ Old commit hash format is INVALID"
fiRepository: kimkulling/Segfault
Length of output: 739
Critical: vcpkg submodule commit does not exist in the official repository.
The vcpkg submodule is being added with commit e7d7451462697d77ef319ddf2da8ff7320a82662, which does not exist in the official Microsoft vcpkg repository (https://github.com/microsoft/vcpkg.git). This will cause build failures when cloning or fetching submodules.
Either specify a valid commit from the official vcpkg repository, or update the submodule URL to point to the fork where this commit exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/contrib/vcpkg` at line 1, The vcpkg submodule entry points to a
non-existent commit e7d7451462697d77ef319ddf2da8ff7320a82662 which will break
submodule fetch; update the submodule reference by either changing the commit to
a valid commit from the official Microsoft vcpkg repo or change the submodule
URL to the fork that contains that commit—edit the submodule configuration
(e.g., the .gitmodules entry and the src/contrib/vcpkg submodule pointer) and
run git submodule sync && git submodule update --init --recursive to verify the
new commit/URL resolves.
| std::string getStartLog() { | ||
| std::string entry = "===========================================================================\n"; | ||
| entry.append("| SegFault version 0.0.l |\n"); | ||
| entry.append("| SegFault version 0.0.l inited.\n"); |
There was a problem hiding this comment.
Typo in version string: 0.0.l should likely be 0.0.1.
The version string appears to use a lowercase letter 'l' instead of the number '1'.
✏️ Proposed fix
- entry.append("| SegFault version 0.0.l inited.\n");
+ entry.append("| SegFault version 0.0.1 inited.\n");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| entry.append("| SegFault version 0.0.l inited.\n"); | |
| entry.append("| SegFault version 0.0.1 inited.\n"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/application/app.cpp` at line 18, The version string in the call
to entry.append in src/runtime/application/app.cpp contains a typo "0.0.l"
(lowercase L) — change that literal to the correct version "0.0.1" in the
entry.append("| SegFault version 0.0.1 inited.\n"); so the logged/printed
version is accurate; locate the entry.append(...) call and update the string
accordingly.
| void App::shutdown() { | ||
| if (mSdlWindow == nullptr) { | ||
| logMessage(LogType::Error, "Invalid application state, cannot shutdown."); | ||
| return; | ||
| if (mSdlWindow == nullptr || mState == ModuleState::Shutdown) { | ||
| logMessage(LogType::Warn, "App already shutdowned."); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Grammar issue and potential logic concern in shutdown guard.
-
Grammar: "shutdowned" is not a valid word. Consider "already shut down" or "shutdown already complete."
-
Logic concern: The merged condition treats
mSdlWindow == nullptr(which could indicate a failed init) the same asmState == ModuleState::Shutdown(legitimate double-shutdown). Previously, thenullptrcase logged an error. Consider whether these scenarios should be distinguished.
✏️ Proposed fix (grammar only)
void App::shutdown() {
if (mSdlWindow == nullptr || mState == ModuleState::Shutdown) {
- logMessage(LogType::Warn, "App already shutdowned.");
+ logMessage(LogType::Warn, "App already shut down.");
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/application/app.cpp` around lines 134 - 139, The shutdown guard
in App::shutdown conflates two different cases and has a grammar error; change
the branching to check mState == ModuleState::Shutdown first and return with a
corrected message like "App already shut down." and separately handle mSdlWindow
== nullptr (which likely indicates failed init) by logging an error via
logMessage (e.g., "Cannot shutdown: SDL window not initialized") and returning;
update the log text to use "shut down" instead of "shutdowned" and ensure you
reference App::shutdown, mState, ModuleState::Shutdown, mSdlWindow, and
logMessage when making the changes.
| int texWidth, texHeight, texChannels; | ||
| stbi_uc *pixels = stbi_load("textures/SegFault.jpg", &texWidth, &texHeight, &texChannels, STBI_rgb_alpha); | ||
| VkDeviceSize imageSize = texWidth * texHeight * 4; | ||
|
|
||
| if (pixels == nullptr) { | ||
| throw std::runtime_error("failed to load texture image!"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file at the specified lines
head -n 1085 src/runtime/renderer/RHIVulkan.cpp | tail -n 15Repository: kimkulling/Segfault
Length of output: 581
🏁 Script executed:
# Find the RHIImpl::createTextureImage function and see where imageSize is used
sed -n '1074,1150p' src/runtime/renderer/RHIVulkan.cppRepository: kimkulling/Segfault
Length of output: 3682
Move imageSize calculation below the load failure check.
texWidth and texHeight are used in the calculation before stbi_load() is validated. Reading uninitialized variables constitutes undefined behavior. If the load fails, the calculation uses indeterminate values before throwing the exception.
Initialize the variables and move the imageSize calculation to occur only after the null check succeeds:
Suggested fix
- int texWidth, texHeight, texChannels;
- stbi_uc *pixels = stbi_load("textures/SegFault.jpg", &texWidth, &texHeight, &texChannels, STBI_rgb_alpha);
- VkDeviceSize imageSize = texWidth * texHeight * 4;
+ int texWidth = 0;
+ int texHeight = 0;
+ int texChannels = 0;
+ stbi_uc* pixels = stbi_load("textures/SegFault.jpg", &texWidth, &texHeight, &texChannels, STBI_rgb_alpha);
if (pixels == nullptr) {
throw std::runtime_error("failed to load texture image!");
}
+
+ VkDeviceSize imageSize = static_cast<VkDeviceSize>(texWidth) * static_cast<VkDeviceSize>(texHeight) * 4;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int texWidth, texHeight, texChannels; | |
| stbi_uc *pixels = stbi_load("textures/SegFault.jpg", &texWidth, &texHeight, &texChannels, STBI_rgb_alpha); | |
| VkDeviceSize imageSize = texWidth * texHeight * 4; | |
| if (pixels == nullptr) { | |
| throw std::runtime_error("failed to load texture image!"); | |
| } | |
| int texWidth = 0; | |
| int texHeight = 0; | |
| int texChannels = 0; | |
| stbi_uc* pixels = stbi_load("textures/SegFault.jpg", &texWidth, &texHeight, &texChannels, STBI_rgb_alpha); | |
| if (pixels == nullptr) { | |
| throw std::runtime_error("failed to load texture image!"); | |
| } | |
| VkDeviceSize imageSize = static_cast<VkDeviceSize>(texWidth) * static_cast<VkDeviceSize>(texHeight) * 4; |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 1076-1076: Define each identifier in a dedicated statement.
[warning] 1081-1081: Define and throw a dedicated exception instead of using a generic one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/renderer/RHIVulkan.cpp` around lines 1076 - 1082, The imageSize
calculation uses texWidth and texHeight before verifying stbi_load succeeded;
move the line "VkDeviceSize imageSize = texWidth * texHeight * 4;" to after the
null-check of pixels (after the throw) so it only runs when pixels is non-null,
and ensure texWidth/texHeight are initialized by stbi_load; update the block
around stbi_load, pixels, texWidth, texHeight, imageSize accordingly.
| transitionImageLayout(textureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_UNDEFINED, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL); | ||
| copyBufferToImage(stagingBuffer, textureImage, static_cast<uint32_t>(texWidth), static_cast<uint32_t>(texHeight)); | ||
| transitionImageLayout(textureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL); | ||
|
|
||
| transitionImageLayout(textureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
ls -la src/runtime/renderer/RHIVulkan.cpp
wc -l src/runtime/renderer/RHIVulkan.cppRepository: kimkulling/Segfault
Length of output: 187
🏁 Script executed:
# Read the specific lines mentioned in the review (1099-1103) with context
sed -n '1090,1110p' src/runtime/renderer/RHIVulkan.cpp | cat -nRepository: kimkulling/Segfault
Length of output: 1429
🏁 Script executed:
# Also check if there's a transitionImageLayout function to understand its implementation
rg -A 10 "void.*transitionImageLayout" src/runtime/renderer/RHIVulkan.cppRepository: kimkulling/Segfault
Length of output: 1406
Remove the duplicate layout transition.
Lines 1102-1103 contain an identical transition call that incorrectly attempts to transition from VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL to VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL. After the first transition on line 1101, the image is already in VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL, so this duplicate transition violates Vulkan's image layout state machine and will trigger validation errors at runtime.
Suggested fix
transitionImageLayout(textureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_UNDEFINED, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL);
copyBufferToImage(stagingBuffer, textureImage, static_cast<uint32_t>(texWidth), static_cast<uint32_t>(texHeight));
transitionImageLayout(textureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL);
-
- transitionImageLayout(textureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| transitionImageLayout(textureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_UNDEFINED, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL); | |
| copyBufferToImage(stagingBuffer, textureImage, static_cast<uint32_t>(texWidth), static_cast<uint32_t>(texHeight)); | |
| transitionImageLayout(textureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL); | |
| transitionImageLayout(textureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL); | |
| transitionImageLayout(textureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_UNDEFINED, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL); | |
| copyBufferToImage(stagingBuffer, textureImage, static_cast<uint32_t>(texWidth), static_cast<uint32_t>(texHeight)); | |
| transitionImageLayout(textureImage, VK_FORMAT_R8G8B8A8_SRGB, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/renderer/RHIVulkan.cpp` around lines 1099 - 1103, The code calls
transitionImageLayout(textureImage, VK_FORMAT_R8G8B8A8_SRGB,
VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL)
twice in a row after copyBufferToImage; remove the duplicate second call so
transitionImageLayout is invoked only once (keeping the initial transition from
UNDEFINED->TRANSFER_DST_OPTIMAL, copyBufferToImage, then the single
TRANSFER_DST_OPTIMAL->SHADER_READ_ONLY_OPTIMAL). Ensure no extra transition
calls remain for textureImage with VK_FORMAT_R8G8B8A8_SRGB that would attempt to
re-transition from TRANSFER_DST_OPTIMAL to SHADER_READ_ONLY_OPTIMAL.
| void RHIImpl::createTextureSampler() { | ||
| VkSamplerCreateInfo samplerInfo{}; | ||
| samplerInfo.sType = VK_STRUCTURE_TYPE_SAMPLER_CREATE_INFO; | ||
| samplerInfo.magFilter = VK_FILTER_LINEAR; | ||
| samplerInfo.minFilter = VK_FILTER_LINEAR; | ||
|
|
||
| samplerInfo.addressModeU = VK_SAMPLER_ADDRESS_MODE_REPEAT; | ||
| samplerInfo.addressModeV = VK_SAMPLER_ADDRESS_MODE_REPEAT; | ||
| samplerInfo.addressModeW = VK_SAMPLER_ADDRESS_MODE_REPEAT; | ||
|
|
||
| VkPhysicalDeviceProperties properties{}; | ||
| vkGetPhysicalDeviceProperties(physicalDevice, &properties); | ||
| samplerInfo.anisotropyEnable = VK_TRUE; | ||
| samplerInfo.maxAnisotropy = properties.limits.maxSamplerAnisotropy; | ||
|
|
||
| samplerInfo.borderColor = VK_BORDER_COLOR_INT_OPAQUE_BLACK; | ||
| samplerInfo.unnormalizedCoordinates = VK_FALSE; | ||
|
|
||
| samplerInfo.compareEnable = VK_FALSE; | ||
| samplerInfo.compareOp = VK_COMPARE_OP_ALWAYS; | ||
|
|
||
| samplerInfo.mipmapMode = VK_SAMPLER_MIPMAP_MODE_LINEAR; | ||
| samplerInfo.mipLodBias = 0.0f; | ||
| samplerInfo.minLod = 0.0f; | ||
| samplerInfo.maxLod = 0.0f; | ||
|
|
||
| if (vkCreateSampler(device, &samplerInfo, nullptr, &textureSampler) != VK_SUCCESS) { | ||
| throw std::runtime_error("failed to create texture sampler!"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the createLogicalDevice function
fd -t f "RHIVulkan.cpp" --exec wc -l {} \;Repository: kimkulling/Segfault
Length of output: 104
🏁 Script executed:
# Search for createLogicalDevice in the file
rg -n "createLogicalDevice" src/runtime/renderer/RHIVulkan.cppRepository: kimkulling/Segfault
Length of output: 501
🏁 Script executed:
# Search for VkPhysicalDeviceFeatures usage
rg -n "VkPhysicalDeviceFeatures" src/runtime/renderer/RHIVulkan.cpp -A 5 -B 2Repository: kimkulling/Segfault
Length of output: 417
🏁 Script executed:
# Search for samplerAnisotropy usage in the file
rg -n "samplerAnisotropy\|anisotropyEnable" src/runtime/renderer/RHIVulkan.cpp -B 3 -A 3Repository: kimkulling/Segfault
Length of output: 45
🏁 Script executed:
rg -n "anisotropy" src/runtime/renderer/RHIVulkan.cpp -B 2 -A 2Repository: kimkulling/Segfault
Length of output: 329
🏁 Script executed:
# Get the exact lines 1133-1161 to see the full function
sed -n '1133,1161p' src/runtime/renderer/RHIVulkan.cppRepository: kimkulling/Segfault
Length of output: 1338
🏁 Script executed:
# Get more context around createLogicalDevice implementation (lines 370-430)
sed -n '370,430p' src/runtime/renderer/RHIVulkan.cppRepository: kimkulling/Segfault
Length of output: 2980
Gate sampler anisotropy on device feature availability.
In createLogicalDevice(), VkPhysicalDeviceFeatures deviceFeatures is zero-initialized (line 397) and samplerAnisotropy is never enabled. However, createTextureSampler() unconditionally sets samplerInfo.anisotropyEnable = VK_TRUE (line 1145), which violates the Vulkan specification and will fail validation on conformant drivers. Query the device for samplerAnisotropy support, enable it in deviceFeatures.samplerAnisotropy = VK_TRUE during logical device creation, then gate the sampler's anisotropy enablement on this feature being available.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 1160-1160: Define and throw a dedicated exception instead of using a generic one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/renderer/RHIVulkan.cpp` around lines 1133 - 1161,
createTextureSampler() unconditionally enables anisotropy
(samplerInfo.anisotropyEnable = VK_TRUE) while createLogicalDevice() never
enables deviceFeatures.samplerAnisotropy; fix by querying the physical device
features (vkGetPhysicalDeviceFeatures or vkGetPhysicalDeviceProperties as
appropriate) in createLogicalDevice(), set deviceFeatures.samplerAnisotropy =
VK_TRUE when supported and enable it when creating the logical device, and then
in createTextureSampler() gate samplerInfo.anisotropyEnable (and
samplerInfo.maxAnisotropy) on whether that samplerAnisotropy feature was
reported/enabled (store the queried/enabled flag in a member or check the
VkPhysicalDeviceFeatures used to create the device) so anisotropy is only used
when supported.
| void RHIImpl::transitionImageLayout(VkImage image, VkFormat format, VkImageLayout oldLayout, VkImageLayout newLayout) { | ||
| VkCommandBuffer commandBuffer = beginSingleTimeCommands(); | ||
|
|
||
| VkImageMemoryBarrier barrier{}; | ||
| barrier.sType = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER; | ||
| barrier.oldLayout = oldLayout; | ||
| barrier.newLayout = newLayout; | ||
|
|
||
| barrier.srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED; | ||
| barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED; | ||
|
|
||
| barrier.image = image; | ||
| barrier.subresourceRange.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT; | ||
| barrier.subresourceRange.baseMipLevel = 0; | ||
| barrier.subresourceRange.levelCount = 1; | ||
| barrier.subresourceRange.baseArrayLayer = 0; | ||
| barrier.subresourceRange.layerCount = 1; | ||
|
|
||
| barrier.srcAccessMask = 0; // TODO | ||
| barrier.dstAccessMask = 0; // TODO | ||
|
|
||
| vkCmdPipelineBarrier( | ||
| commandBuffer, | ||
| 0 /* TODO */, 0 /* TODO */, | ||
| 0, | ||
| 0, nullptr, | ||
| 0, nullptr, | ||
| 1, &barrier | ||
| ); | ||
|
|
||
| VkPipelineStageFlags sourceStage; | ||
| VkPipelineStageFlags destinationStage; | ||
|
|
||
| if (oldLayout == VK_IMAGE_LAYOUT_UNDEFINED && newLayout == VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL) { | ||
| barrier.srcAccessMask = 0; | ||
| barrier.dstAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; | ||
|
|
||
| sourceStage = VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT; | ||
| destinationStage = VK_PIPELINE_STAGE_TRANSFER_BIT; | ||
| } else if (oldLayout == VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL && newLayout == VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL) { | ||
| barrier.srcAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; | ||
| barrier.dstAccessMask = VK_ACCESS_SHADER_READ_BIT; | ||
|
|
||
| sourceStage = VK_PIPELINE_STAGE_TRANSFER_BIT; | ||
| destinationStage = VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT; | ||
| } else { | ||
| throw std::invalid_argument("unsupported layout transition!"); | ||
| } | ||
|
|
||
| vkCmdPipelineBarrier( | ||
| commandBuffer, | ||
| sourceStage, destinationStage, | ||
| 0, | ||
| 0, nullptr, | ||
| 0, nullptr, | ||
| 1, &barrier | ||
| ); | ||
| endSingleTimeCommands(commandBuffer); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -name "RHIVulkan.cpp" -type fRepository: kimkulling/Segfault
Length of output: 99
🏁 Script executed:
# Get the size of the file to determine how to read it
wc -l src/runtime/renderer/RHIVulkan.cppRepository: kimkulling/Segfault
Length of output: 102
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '1313,1370p' src/runtime/renderer/RHIVulkan.cpp | cat -nRepository: kimkulling/Segfault
Length of output: 2721
🏁 Script executed:
# Search for other uses of vkCmdPipelineBarrier in the codebase to understand if this pattern is used elsewhere
rg "vkCmdPipelineBarrier" src/runtime/renderer/RHIVulkan.cpp -B 3 -A 5Repository: kimkulling/Segfault
Length of output: 561
🌐 Web query:
Vulkan vkCmdPipelineBarrier srcStageMask dstStageMask zero requirement specification
💡 Result:
In Vulkan's vkCmdPipelineBarrier, both srcStageMask and dstStageMask have specific zero requirements defined in the specification's Valid Usage statements. srcStageMask must not be 0 unless the synchronization2 feature is enabled: VUID-vkCmdPipelineBarrier-srcStageMask-03937 states "If the synchronization2 feature is not enabled, srcStageMask must not be 0". Similarly for dstStageMask: VUID-vkCmdPipelineBarrier-dstStageMask-03938 states "If the synchronization2 feature is not enabled, dstStageMask must not be 0". When the synchronization2 feature (introduced in Vulkan 1.3 / VK_KHR_synchronization2) is enabled, zero stage masks are permitted, corresponding to the newer vkCmdPipelineBarrier2 where VK_PIPELINE_STAGE_2_NONE is valid. These rules ensure pipeline barriers define meaningful synchronization scopes in legacy mode without synchronization2.
Citations:
- 1: https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkCmdPipelineBarrier.html
- 2: https://registry.khronos.org/VulkanSC/specs/1.0-extensions/man/html/vkCmdPipelineBarrier.html
Remove the invalid placeholder barrier call with zero stage masks.
The first vkCmdPipelineBarrier() call uses zero for both srcStageMask and dstStageMask, which violates the Vulkan specification (VUID-vkCmdPipelineBarrier-srcStageMask-03937 and VUID-vkCmdPipelineBarrier-dstStageMask-03938). Stage masks must be non-zero unless the synchronization2 feature is enabled, which this codebase does not use. This call will fail validation and produce undefined behavior. Remove it and issue the barrier only after determining the proper stage masks in the conditional blocks below.
Suggested fix
- barrier.srcAccessMask = 0; // TODO
- barrier.dstAccessMask = 0; // TODO
-
- vkCmdPipelineBarrier(
- commandBuffer,
- 0 /* TODO */, 0 /* TODO */,
- 0,
- 0, nullptr,
- 0, nullptr,
- 1, &barrier
- );
-
VkPipelineStageFlags sourceStage;
VkPipelineStageFlags destinationStage;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void RHIImpl::transitionImageLayout(VkImage image, VkFormat format, VkImageLayout oldLayout, VkImageLayout newLayout) { | |
| VkCommandBuffer commandBuffer = beginSingleTimeCommands(); | |
| VkImageMemoryBarrier barrier{}; | |
| barrier.sType = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER; | |
| barrier.oldLayout = oldLayout; | |
| barrier.newLayout = newLayout; | |
| barrier.srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED; | |
| barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED; | |
| barrier.image = image; | |
| barrier.subresourceRange.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT; | |
| barrier.subresourceRange.baseMipLevel = 0; | |
| barrier.subresourceRange.levelCount = 1; | |
| barrier.subresourceRange.baseArrayLayer = 0; | |
| barrier.subresourceRange.layerCount = 1; | |
| barrier.srcAccessMask = 0; // TODO | |
| barrier.dstAccessMask = 0; // TODO | |
| vkCmdPipelineBarrier( | |
| commandBuffer, | |
| 0 /* TODO */, 0 /* TODO */, | |
| 0, | |
| 0, nullptr, | |
| 0, nullptr, | |
| 1, &barrier | |
| ); | |
| VkPipelineStageFlags sourceStage; | |
| VkPipelineStageFlags destinationStage; | |
| if (oldLayout == VK_IMAGE_LAYOUT_UNDEFINED && newLayout == VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL) { | |
| barrier.srcAccessMask = 0; | |
| barrier.dstAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; | |
| sourceStage = VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT; | |
| destinationStage = VK_PIPELINE_STAGE_TRANSFER_BIT; | |
| } else if (oldLayout == VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL && newLayout == VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL) { | |
| barrier.srcAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; | |
| barrier.dstAccessMask = VK_ACCESS_SHADER_READ_BIT; | |
| sourceStage = VK_PIPELINE_STAGE_TRANSFER_BIT; | |
| destinationStage = VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT; | |
| } else { | |
| throw std::invalid_argument("unsupported layout transition!"); | |
| } | |
| vkCmdPipelineBarrier( | |
| commandBuffer, | |
| sourceStage, destinationStage, | |
| 0, | |
| 0, nullptr, | |
| 0, nullptr, | |
| 1, &barrier | |
| ); | |
| endSingleTimeCommands(commandBuffer); | |
| void RHIImpl::transitionImageLayout(VkImage image, VkFormat format, VkImageLayout oldLayout, VkImageLayout newLayout) { | |
| VkCommandBuffer commandBuffer = beginSingleTimeCommands(); | |
| VkImageMemoryBarrier barrier{}; | |
| barrier.sType = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER; | |
| barrier.oldLayout = oldLayout; | |
| barrier.newLayout = newLayout; | |
| barrier.srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED; | |
| barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED; | |
| barrier.image = image; | |
| barrier.subresourceRange.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT; | |
| barrier.subresourceRange.baseMipLevel = 0; | |
| barrier.subresourceRange.levelCount = 1; | |
| barrier.subresourceRange.baseArrayLayer = 0; | |
| barrier.subresourceRange.layerCount = 1; | |
| VkPipelineStageFlags sourceStage; | |
| VkPipelineStageFlags destinationStage; | |
| if (oldLayout == VK_IMAGE_LAYOUT_UNDEFINED && newLayout == VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL) { | |
| barrier.srcAccessMask = 0; | |
| barrier.dstAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; | |
| sourceStage = VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT; | |
| destinationStage = VK_PIPELINE_STAGE_TRANSFER_BIT; | |
| } else if (oldLayout == VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL && newLayout == VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL) { | |
| barrier.srcAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; | |
| barrier.dstAccessMask = VK_ACCESS_SHADER_READ_BIT; | |
| sourceStage = VK_PIPELINE_STAGE_TRANSFER_BIT; | |
| destinationStage = VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT; | |
| } else { | |
| throw std::invalid_argument("unsupported layout transition!"); | |
| } | |
| vkCmdPipelineBarrier( | |
| commandBuffer, | |
| sourceStage, destinationStage, | |
| 0, | |
| 0, nullptr, | |
| 0, nullptr, | |
| 1, &barrier | |
| ); | |
| endSingleTimeCommands(commandBuffer); |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 1331-1331: Complete the task associated to this "TODO" comment.
[warning] 1336-1336: Complete the task associated to this "TODO" comment.
[warning] 1332-1332: Complete the task associated to this "TODO" comment.
[warning] 1336-1336: Complete the task associated to this "TODO" comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/renderer/RHIVulkan.cpp` around lines 1313 - 1370, In
transitionImageLayout() remove the first vkCmdPipelineBarrier(...) call that
passes zero src/dst stage masks (and any placeholder srcAccessMask/dstAccessMask
values) — compute and set barrier.srcAccessMask, barrier.dstAccessMask,
sourceStage and destinationStage in the existing conditional branches (the ones
that set masks for the two supported transitions) and call
vkCmdPipelineBarrier(...) only once after those branches; keep using
beginSingleTimeCommands() and endSingleTimeCommands() as before and update the
barrier struct (image, subresourceRange, etc.) before the single, valid
vkCmdPipelineBarrier call.
| mImpl->createTextureImage(); | ||
| mImpl->createTextureImageView(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the RHIVulkan.cpp file
find . -name "RHIVulkan.cpp" -type fRepository: kimkulling/Segfault
Length of output: 99
🏁 Script executed:
# Get file size to understand if we can read it
wc -l src/runtime/renderer/RHIVulkan.cpp 2>/dev/null || wc -l $(find . -name "RHIVulkan.cpp" -type f 2>/dev/null)Repository: kimkulling/Segfault
Length of output: 102
🏁 Script executed:
# Examine the specific lines mentioned in the review (1493-1494 and 1503-1505)
sed -n '1485,1510p' $(find . -name "RHIVulkan.cpp" -type f)Repository: kimkulling/Segfault
Length of output: 1169
🏁 Script executed:
# Search for createTextureSampler in the file
rg "createTextureSampler" $(find . -name "RHIVulkan.cpp" -type f)Repository: kimkulling/Segfault
Length of output: 142
🏁 Script executed:
# Search for textureSampler variable usage
rg "textureSampler" $(find . -name "RHIVulkan.cpp" -type f) -A 2 -B 2Repository: kimkulling/Segfault
Length of output: 697
🏁 Script executed:
# Search for descriptor set layout and bindings
rg "createDescriptorSetLayout|VkDescriptorSetLayoutBinding" $(find . -name "RHIVulkan.cpp" -type f) -A 5Repository: kimkulling/Segfault
Length of output: 939
🏁 Script executed:
# Search for ALL calls to createTextureSampler() throughout the file
rg "createTextureSampler\(" $(find . -name "RHIVulkan.cpp" -type f)Repository: kimkulling/Segfault
Length of output: 142
🏁 Script executed:
# Get complete init sequence to verify all calls
sed -n '1480,1520p' $(find . -name "RHIVulkan.cpp" -type f)Repository: kimkulling/Segfault
Length of output: 1853
🏁 Script executed:
# Look for descriptor writes/bindings that include sampler
rg "VkWriteDescriptorSet|VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER|imageInfo|samplerBinding" $(find . -name "RHIVulkan.cpp" -type f) -B 3 -A 3Repository: kimkulling/Segfault
Length of output: 1398
🏁 Script executed:
# Double-check that no other init path calls createTextureSampler()
rg "createTextureSampler|mImpl->.*sampler" $(find . -name "RHIVulkan.cpp" -type f) | head -20Repository: kimkulling/Segfault
Length of output: 142
Add missing sampler creation and descriptor binding for texture.
The texture image initialization is incomplete. createTextureSampler() is declared and defined but never called during initialization. Additionally, the descriptor set layout and descriptor writes only bind the uniform buffer—there is no sampler binding. This means the texture image cannot be sampled in shaders. The shutdown code then destroys textureSampler despite it never being created, causing undefined behavior.
Fix this by:
- Calling
mImpl->createTextureSampler()in the initialization sequence (aftercreateTextureImageView()) - Adding a sampler binding to the descriptor set layout
- Writing the sampler to the descriptor set
Also applies to: 1503-1505 (shutdown's sampler destruction)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/renderer/RHIVulkan.cpp` around lines 1493 - 1494, Call
mImpl->createTextureSampler() immediately after mImpl->createTextureImageView()
during initialization so textureSampler is actually created; then add a sampler
binding (VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER) to the descriptor set layout
alongside the uniform buffer binding and include a corresponding
VkWriteDescriptorSet that points to the created textureImageView and
textureSampler (i.e., write the combined image sampler into the descriptor set);
finally, ensure the shutdown code that destroys textureSampler (the lines around
the existing vkDestroySampler call) either only runs when textureSampler was
created or simply remains correct because the sampler is now created.
…ng/Segfault into feature/texture_view
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/runtime/renderer/RHIVulkan.cpp (4)
1095-1100:⚠️ Potential issue | 🔴 CriticalDelay
imageSizecomputation until afterstbi_load()succeeds.
texWidthandtexHeightare indeterminate on the failure path, so the multiply currently runs with garbage values before the null check.🩹 Minimal fix
- int texWidth, texHeight, texChannels; - stbi_uc *pixels = stbi_load("textures/SegFault.jpg", &texWidth, &texHeight, &texChannels, STBI_rgb_alpha); - VkDeviceSize imageSize = texWidth * texHeight * 4; + int texWidth = 0; + int texHeight = 0; + int texChannels = 0; + stbi_uc* pixels = stbi_load("textures/SegFault.jpg", &texWidth, &texHeight, &texChannels, STBI_rgb_alpha); if (pixels == nullptr) { throw std::runtime_error("failed to load texture image!"); } + + VkDeviceSize imageSize = + static_cast<VkDeviceSize>(texWidth) * static_cast<VkDeviceSize>(texHeight) * 4;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/renderer/RHIVulkan.cpp` around lines 1095 - 1100, The code computes VkDeviceSize imageSize before verifying stbi_load succeeded, causing use of uninitialized texWidth/texHeight on failure; move the calculation of imageSize (the multiply of texWidth * texHeight * 4) to after the nullptr check for pixels in RHIVulkan.cpp (the block around stbi_load and the throw), so compute imageSize only when pixels != nullptr and then proceed with the rest of the texture setup (references: variables imageSize, pixels, texWidth, texHeight, and the stbi_load call).
1118-1122:⚠️ Potential issue | 🔴 CriticalRemove the duplicate final layout transition.
After the first
VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL→VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMALtransition, the image is no longer inTRANSFER_DST_OPTIMAL. Replaying the same transition immediately afterward is an invalid state change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/renderer/RHIVulkan.cpp` around lines 1118 - 1122, There is a duplicate final call to transitionImageLayout for textureImage that repeats the VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL -> VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL transition; remove the redundant second transitionImageLayout call so the sequence after copyBufferToImage ends with a single transition to VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL (locate the calls around transitionImageLayout, copyBufferToImage and textureImage).
140-141:⚠️ Potential issue | 🔴 CriticalCreate the texture view/sampler before updating binding 1.
These descriptor writes consume
textureImageViewandtextureSampler, but the init sequence builds descriptor sets before the image view exists,textureSampleris not value-initialized, andcreateTextureSampler()is never called. Since the sets are never rewritten afterward, binding 1 can hold an invalid handle for the whole run, and shutdown later destroys a sampler that was never created. Move descriptor set creation until aftercreateTextureImageView()/createTextureSampler(), or rewrite the sets once those handles are valid.Also applies to: 1276-1299, 1530-1537, 1546-1548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/renderer/RHIVulkan.cpp` around lines 140 - 141, Descriptor sets are being written before textureImageView and textureSampler are created, and textureSampler is not value-initialized or created via createTextureSampler(), so binding 1 can hold an invalid handle and later cause teardown issues; fix by ensuring createTextureImageView() and createTextureSampler() are called (and textureSampler is properly initialized) before calling vkUpdateDescriptorSets for the sets that reference binding 1 (or by re-running the descriptor set writes after those calls). Specifically, move the descriptor set allocation/update code that uses textureImageView/textureSampler to after the calls to createTextureImageView() and createTextureSampler(), or add a follow-up updateDescriptorSets call that updates binding 1 with the valid VkDescriptorImageInfo using textureImageView and textureSampler. Ensure textureSampler is created and checked for success before use and that any old descriptor writes are not relying on uninitialized handles.
1374-1384:⚠️ Potential issue | 🔴 CriticalDelete the placeholder barrier with zero stage masks.
The first
vkCmdPipelineBarrier()runs beforesourceStageanddestinationStageare derived and passes0for both masks. In this API path, that is invalid and will trip validation before the real barrier executes. Keep a single barrier after the access masks and stage flags are selected.🧱 Keep only the final barrier
- barrier.srcAccessMask = 0; // TODO - barrier.dstAccessMask = 0; // TODO - - vkCmdPipelineBarrier( - commandBuffer, - 0 /* TODO */, 0 /* TODO */, - 0, - 0, nullptr, - 0, nullptr, - 1, &barrier - ); - VkPipelineStageFlags sourceStage; VkPipelineStageFlags destinationStage;Also applies to: 1386-1412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/renderer/RHIVulkan.cpp` around lines 1374 - 1384, Remove the premature pipeline barrier call that uses zero stage masks: delete the vkCmdPipelineBarrier invocation that passes 0 for both source and destination stage masks (the call that uses barrier.srcAccessMask = 0 and barrier.dstAccessMask = 0) and ensure only the final vkCmdPipelineBarrier is kept after you derive and set barrier.srcAccessMask, barrier.dstAccessMask, srcStage and dstStage; reference the existing barrier variable and commandBuffer/vkCmdPipelineBarrier to locate and keep the single valid barrier invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/shaders/default.frag`:
- Around line 4-9: The fragment shader currently writes UVs to outColor instead
of sampling the bound combined image sampler; declare a combined image sampler
(sampler2D) matching binding 1 and use texture(sampler, fragTexCoord) to set
outColor. Update the shader to add a uniform sampler2D (bound at layout(binding
= 1)) and replace the outColor assignment in main() (which currently uses
fragTexCoord) with the sampled texel value from that sampler2D.
In `@src/runtime/renderer/RHIVulkan.cpp`:
- Around line 10-11: Add the required MIT license header at the very top of
RHIVulkan.cpp before any preprocessor directives or includes (before `#define`
STB_IMAGE_IMPLEMENTATION and `#include` <stb_image.h>); insert the standard
project MIT header block exactly as used elsewhere in the repo so the file
contains the required license notice for changed C++ sources.
---
Duplicate comments:
In `@src/runtime/renderer/RHIVulkan.cpp`:
- Around line 1095-1100: The code computes VkDeviceSize imageSize before
verifying stbi_load succeeded, causing use of uninitialized texWidth/texHeight
on failure; move the calculation of imageSize (the multiply of texWidth *
texHeight * 4) to after the nullptr check for pixels in RHIVulkan.cpp (the block
around stbi_load and the throw), so compute imageSize only when pixels !=
nullptr and then proceed with the rest of the texture setup (references:
variables imageSize, pixels, texWidth, texHeight, and the stbi_load call).
- Around line 1118-1122: There is a duplicate final call to
transitionImageLayout for textureImage that repeats the
VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL -> VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL
transition; remove the redundant second transitionImageLayout call so the
sequence after copyBufferToImage ends with a single transition to
VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL (locate the calls around
transitionImageLayout, copyBufferToImage and textureImage).
- Around line 140-141: Descriptor sets are being written before textureImageView
and textureSampler are created, and textureSampler is not value-initialized or
created via createTextureSampler(), so binding 1 can hold an invalid handle and
later cause teardown issues; fix by ensuring createTextureImageView() and
createTextureSampler() are called (and textureSampler is properly initialized)
before calling vkUpdateDescriptorSets for the sets that reference binding 1 (or
by re-running the descriptor set writes after those calls). Specifically, move
the descriptor set allocation/update code that uses
textureImageView/textureSampler to after the calls to createTextureImageView()
and createTextureSampler(), or add a follow-up updateDescriptorSets call that
updates binding 1 with the valid VkDescriptorImageInfo using textureImageView
and textureSampler. Ensure textureSampler is created and checked for success
before use and that any old descriptor writes are not relying on uninitialized
handles.
- Around line 1374-1384: Remove the premature pipeline barrier call that uses
zero stage masks: delete the vkCmdPipelineBarrier invocation that passes 0 for
both source and destination stage masks (the call that uses
barrier.srcAccessMask = 0 and barrier.dstAccessMask = 0) and ensure only the
final vkCmdPipelineBarrier is kept after you derive and set
barrier.srcAccessMask, barrier.dstAccessMask, srcStage and dstStage; reference
the existing barrier variable and commandBuffer/vkCmdPipelineBarrier to locate
and keep the single valid barrier invocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e92ba36d-1a0e-45cc-8338-78318f8109bf
📒 Files selected for processing (4)
assets/shaders/default.fragassets/shaders/default.vertsrc/contrib/cppcoresrc/runtime/renderer/RHIVulkan.cpp
| #define STB_IMAGE_IMPLEMENTATION | ||
| #include <stb_image.h> |
There was a problem hiding this comment.
Add the required MIT header above the includes.
This file still starts directly with includes, so the license notice required for changed C++ sources is missing. As per coding guidelines, Include MIT license header in all source files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/renderer/RHIVulkan.cpp` around lines 10 - 11, Add the required
MIT license header at the very top of RHIVulkan.cpp before any preprocessor
directives or includes (before `#define` STB_IMAGE_IMPLEMENTATION and `#include`
<stb_image.h>); insert the standard project MIT header block exactly as used
elsewhere in the repo so the file contains the required license notice for
changed C++ sources.
|
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores