Feature/add uniform buffers #3
Conversation
|
Warning Rate limit exceeded@kimkulling has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughReplaced a Bash shader compile script with a new Python Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/renderer/RHIVulkan.cpp (1)
355-390: Logical inconsistency increateLogicalDevicewhen graphics and present queues differThe code builds
queueCreateInfosfor all unique queue families but then forcesqueueCreateInfoCountback to1:createInfo.queueCreateInfoCount = static_cast<uint32_t>(queueCreateInfos.size()); createInfo.pQueueCreateInfos = queueCreateInfos.data(); createInfo.queueCreateInfoCount = 1; // Overwrites the countIf
graphicsFamily != presentFamily, only the first queue-family create-info is passed tovkCreateDevice, yet lines 65-66 callvkGetDeviceQueuefor both families:vkGetDeviceQueue(device, qfIndices.presentFamily.value(), 0, &presentQueue); vkGetDeviceQueue(device, qfIndices.graphicsFamily.value(), 0, &graphicsQueue);This will fail or exhibit undefined behavior on the second family if it wasn't created.
Remove the single-queue path and use
queueCreateInfosdirectly:- VkDeviceQueueCreateInfo queueCreateInfo{}; - queueCreateInfo.sType = VK_STRUCTURE_TYPE_DEVICE_QUEUE_CREATE_INFO; - queueCreateInfo.queueFamilyIndex = queueFamilyIndices.graphicsFamily.value(); - queueCreateInfo.queueCount = 1; - float queuePrioritys[2] = { 1.f, 1.f }; - queueCreateInfo.pNext = nullptr; - queueCreateInfo.pQueuePriorities = &queuePrioritys[0]; - VkPhysicalDeviceFeatures deviceFeatures{}; VkDeviceCreateInfo createInfo{}; createInfo.sType = VK_STRUCTURE_TYPE_DEVICE_CREATE_INFO; createInfo.pNext = nullptr; - createInfo.pQueueCreateInfos = &queueCreateInfo; - - createInfo.queueCreateInfoCount = static_cast<uint32_t>(queueCreateInfos.size()); - createInfo.pQueueCreateInfos = queueCreateInfos.data(); - - createInfo.queueCreateInfoCount = 1; + createInfo.queueCreateInfoCount = static_cast<uint32_t>(queueCreateInfos.size()); + createInfo.pQueueCreateInfos = queueCreateInfos.data();src/runtime/application/app.cpp (1)
119-127: Fix empty null-check inApp::shutdown()with early returnThe
if (mSdlWindow == nullptr) {}block is empty and confusing. This should be converted to an early return to make the intent explicit and prevent operating on an uninitialized window:void App::shutdown() { - if (mSdlWindow == nullptr) { - - } - SDL_DestroyWindow(mSdlWindow); + if (mSdlWindow == nullptr) { + return; + } + + SDL_DestroyWindow(mSdlWindow); mSdlWindow = nullptr; mState = ModuleState::Shutdown; SDL_Quit(); }This clarifies the guard condition and improves code readability.
🧹 Nitpick comments (3)
src/runtime/core/genericfilemanager.cpp (1)
9-22: File open/create logic is straightforward; consider binary mode for non-text assetsThe reader/writer functions are simple and correct: they null-check
name, attemptfopen, and wrap theFILE*in aFileArchive. If this manager is used for binary assets (textures, shaders, etc.) on Windows, you may want"rb"/"wb+"instead of"r"/"w+"to avoid newline translation.Also applies to: 24-37
src/runtime/renderer/RHIVulkan.cpp (1)
175-181: More informative logging inreadFile; consider also enriching the exception textLogging
"Failed to open file <name>."viacore::logMessageis an improvement over the previous static message. You might also want thestd::runtime_errorto include the filename for callers that don’t have access to the logs:- core::logMessage(core::LogType::Error, errorMsg.c_str()); - throw std::runtime_error("failed to open file!"); + core::logMessage(core::LogType::Error, errorMsg.c_str()); + throw std::runtime_error(errorMsg);Not required, but it can simplify debugging when exceptions bubble up.
scripts/compile_shader.py (1)
42-51: Make shader path handling robust and propagate compile failures
compile_shader(args.shader + shader, shader_out)assumesargs.shaderends with a path separator;--shader ./shaderswould produce./shadersdefault.vert. It’s safer to useos.path.join/Pathand to handle compilation errors:- shader_files = [f for f in listdir(args.shader) if isfile(join(args.shader, f))] + shader_files = [f for f in listdir(args.shader) if isfile(join(args.shader, f))] print("shader files = " + str(shader_files)) for shader in shader_files: if shader in shader_names: - path = Path(shader) - shader_out = path.suffix[1:len(path.suffix)] + ".spv" - compile_shader(args.shader + shader, shader_out) - - copy_shader(shader_out, "../bin/debug/shaders") + shader_path = join(args.shader, shader) + path = Path(shader) + shader_out = path.suffix.lstrip(".") + ".spv" + success = compile_shader(shader_path, shader_out) + if not success: + # Fail fast so calling scripts/CI can detect the problem + raise SystemExit(f"Failed to compile shader: {shader_path}") + + copy_shader(shader_out, "../bin/debug/shaders")You might also want to base the destination directory on the script’s location (e.g.,
Path(__file__).resolve().parent / ".." / "bin" / "debug" / "shaders") to make it independent of the current working directory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
scripts/compile_shader.py(1 hunks)scripts/compile_shader.sh(0 hunks)src/runtime/application/app.cpp(1 hunks)src/runtime/core/filearchive.h(0 hunks)src/runtime/core/genericfilemanager.cpp(1 hunks)src/runtime/core/genericfilemanager.h(1 hunks)src/runtime/core/ifilemanager.h(1 hunks)src/runtime/renderer/RHIVulkan.cpp(5 hunks)
💤 Files with no reviewable changes (2)
- src/runtime/core/filearchive.h
- scripts/compile_shader.sh
🧰 Additional context used
🧬 Code graph analysis (4)
src/runtime/core/ifilemanager.h (2)
src/runtime/core/filearchive.h (4)
FileArchive(9-46)FileArchive(34-36)FileArchive(38-40)FileArchive(42-44)src/runtime/core/genericfilemanager.cpp (10)
createFileReader(9-22)createFileReader(9-9)createFileWriter(24-37)createFileWriter(24-24)close(39-45)close(39-39)exist(47-53)exist(47-47)getArchiveStat(55-81)getArchiveStat(55-55)
src/runtime/core/genericfilemanager.cpp (1)
src/runtime/renderer/RHIVulkan.cpp (1)
buffer(184-184)
src/runtime/renderer/RHIVulkan.cpp (1)
src/runtime/core/segfault.h (1)
logMessage(58-73)
src/runtime/core/genericfilemanager.h (2)
src/runtime/core/ifilemanager.h (1)
SEGFAULT_EXPORT(15-26)src/runtime/core/genericfilemanager.cpp (10)
createFileReader(9-22)createFileReader(9-9)createFileWriter(24-37)createFileWriter(24-24)close(39-45)close(39-39)exist(47-53)exist(47-47)getArchiveStat(55-81)getArchiveStat(55-55)
🪛 Ruff (0.14.8)
scripts/compile_shader.py
20-20: subprocess call: check for execution of untrusted input
(S603)
21-21: Unpacked variable output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🔇 Additional comments (6)
src/runtime/application/app.cpp (1)
7-15: Formatting-only change between includes and namespace is fineThe added blank line improves readability and has no behavioral impact.
src/runtime/core/genericfilemanager.h (1)
7-16: Header formatting aligns withIFileManagerinterfaceOnly indentation/whitespace changed; the
GenericFileManagerAPI stays consistent withIFileManager. No behavioral impact.src/runtime/core/ifilemanager.h (1)
9-26: Interface and types unchanged; formatting onlyThe forward declaration,
FileStatstruct, andIFileManagervirtual API are unchanged semantically; only indentation/spacing was adjusted.src/runtime/core/genericfilemanager.cpp (1)
47-81: Existence/stat helpers look goodThe
existandgetArchiveStatimplementations correctly null-checknameand usestat/_stat64to determine presence and file size. Behavior is unchanged aside from formatting.src/runtime/renderer/RHIVulkan.cpp (2)
22-25: Value-initializingVertex::posandVertex::coloris a safe improvementZero-initializing the members avoids any chance of using an uninitialized
Vertex. The rest of the vertex setup remains unchanged.
329-347: Index initialization change infindQueueFamiliesis purely stylisticSwitching to brace initialization (
int i{ 0 };) and pre-increment (++i;) is idiomatic and has no behavioral impact on the queue-family search logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/compile_shader.py (1)
10-27: Fix subprocess handling: stderr is never captured, return code is ignored, and error message is brokenCurrent logic has multiple problems:
Popen(..., stdout=PIPE)withoutstderr=PIPEguaranteeserrorfromcommunicate()is alwaysNone, so failures are reported as success.- You never check
process_handle.returncode.- The error message string is not interpolated and
outputis unused apart from optional printing.This means
glslcfailures can silently look like successful compiles, undermining the goal of “more verbose” errors.A more robust approach is to use
subprocess.runwith explicit stdout/stderr capture and return-code checking, and to return a boolean so callers can react:-def compile_shader(shadername, shader_out, verbose): - if len(shadername) == 0: - return - - cmd = [] - cmd.append("glslc") - cmd.append(shadername) - cmd.append("-o") - cmd.append(shader_out) - process_handle = subprocess.Popen(cmd, stdout=subprocess.PIPE) - output, error = process_handle.communicate() - if error is None: - print("Shader " + shadername + " compiled.") - if verbose and output is not None: - if len(output) != 0: - print(str(output)) - else: - print("Error {error} while compilation.", error) +def compile_shader(shadername: str, shader_out: str, verbose: bool) -> bool: + if not shadername: + print("No shader specified") + return False + + cmd = ["glslc", shadername, "-o", shader_out] + if verbose: + print("Running:", " ".join(cmd)) + + try: + result = subprocess.run( + cmd, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + check=False, + ) + except OSError as exc: + print(f"Failed to invoke glslc: {exc}") + return False + + if result.returncode == 0: + print(f"Shader {shadername} compiled.") + if verbose and result.stdout: + print(result.stdout) + return True + + print(f"Error while compiling {shadername} (exit code {result.returncode}):") + if result.stderr: + print(result.stderr) + return FalseIn Python's subprocess module, confirm that `subprocess.run(..., stdout=PIPE, stderr=PIPE, text=True)` returns a CompletedProcess with `.stdout` and `.stderr` as decoded strings and `.returncode` holding the exit status.
🧹 Nitpick comments (1)
scripts/compile_shader.py (1)
29-35: copy_shader is fine for a dev script; optional robustness improvementsFunctionally this is OK for a local build helper. If you want slightly more robust behavior and clearer intent, you could lean on
pathlibandexist_ok:-def copy_shader(source, dest): - if not os.path.exists(dest): - print("Create folder " + dest) - os.makedirs(dest) - print("source " + source) - shutil.copy(source, dest) +def copy_shader(source: str, dest: str) -> None: + dest_path = Path(dest) + if not dest_path.exists(): + print(f"Create folder {dest_path}") + dest_path.mkdir(parents=True, exist_ok=True) + print(f"Copy {source} -> {dest_path}") + shutil.copy(source, dest_path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/compile_shader.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
scripts/compile_shader.py
19-19: subprocess call: check for execution of untrusted input
(S603)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/runtime/renderer/RHIVulkan.cpp (1)
176-180: Use the detailed error message in the thrown exception.The enhanced error message on Line 179 includes the filename, which is helpful. However, Line 180 throws a generic exception message. Callers catching the exception won't know which file failed to open.
Apply this diff to propagate the detailed message to the exception:
if (!file.is_open()) { std::string errorMsg = "Failed to open file "; errorMsg += filename; errorMsg += "."; core::logMessage(core::LogType::Error, errorMsg.c_str()); - throw std::runtime_error("failed to open file!"); + throw std::runtime_error(errorMsg); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/runtime/application/app.cpp(3 hunks)src/runtime/renderer/RHIVulkan.cpp(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/application/app.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
src/runtime/renderer/RHIVulkan.cpp (1)
src/runtime/core/segfault.h (1)
logMessage(58-73)
🔇 Additional comments (1)
src/runtime/renderer/RHIVulkan.cpp (1)
23-24: LGTM! Value initialization prevents undefined behavior.The brace initialization ensures
posandcolorare zero-initialized, which is good defensive practice for struct members.
|
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.