support texture binding when bindless is not available #33
Conversation
WalkthroughThe changes introduce a new command-line argument handling system in Changes
Possibly related PRs
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Outside diff range and nitpick comments (3)
src/foundations/Args.zig (3)
10-10: Remove unnecessary debug printing of arguments.The line
std.debug.print("{s}\n", .{av});prints each command-line argument, which may clutter the console output. Unless needed for debugging, consider removing it or controlling it with a verbosity flag.Apply this diff to remove the debug print:
- std.debug.print("{s}\n", .{av});
15-15: Simplifystd.debug.printstatement without empty argument list.The
std.debug.printcall does not require an empty argument list when there are no format specifiers in the string.Apply this diff to simplify the statement:
- std.debug.print("--disable_bindless to disable bindless textures\n", .{}); + std.debug.print("--disable_bindless to disable bindless textures\n");
18-18: Use the correct format specifier for boolean values.In
std.debug.print("args.disable_bindless: {any}\n", .{rv.disable_bindless});, using{any}is unnecessary for boolean values. Use{}for proper type inference.Apply this diff to correct the format specifier:
- std.debug.print("args.disable_bindless: {any}\n", .{rv.disable_bindless}); + std.debug.print("args.disable_bindless: {}\n", .{rv.disable_bindless});
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/foundations/Args.zig (1 hunks)
- src/foundations/app.zig (3 hunks)
- src/foundations/rhi/Texture.zig (4 hunks)
- src/foundations/scenes/cgpoc/chapter5/textured_pyramid/TexturedPyramid.zig (2 hunks)
- src/foundations/scenes/cgpoc/chapter6/earth/Earth.zig (2 hunks)
- src/foundations/scenes/cgpoc/chapter6/shuttle/Shuttle.zig (2 hunks)
- src/foundations/scenes/cgpoc/chapter6/textured_torus/TexturedTorus.zig (2 hunks)
- src/foundations/scenes/scenes.zig (2 hunks)
Additional comments not posted (10)
src/foundations/app.zig (3)
42-42: PassingargstoSceneContextAdding
.args = args,correctly includes the command-line arguments in theSceneContext. This allows scenes to access user-provided configurations, enhancing the application's flexibility.
87-87: ImportingArgs.zigmoduleThe import statement correctly includes the
Args.zigmodule, providing access to theArgsfunctionality required for argument handling.
15-16: Ensureargsis properly deinitialized to prevent resource leaksThe
argsvariable is initialized withArgs.init(allocator);but there is no corresponding deinitialization. To prevent potential resource leaks, ensure thatargs.deinit()is called during the application's deinitialization process.Apply this diff to deinitialize
args:pub fn deinit(self: *App) void { self.app_scenes.deinit(); self.obj_loader.deinit(); self.textures_loader.deinit(); rhi.deinit(); ui.deinit(); self.app_config.deinit(); + args.deinit(); self.allocator.destroy(self); }Alternatively, if
argsis managed elsewhere, ensure that it is properly deinitialized.Run the following script to verify if
args.deinit()is called:src/foundations/scenes/scenes.zig (2)
80-80: Approved: Import ofArgsmoduleThe import of
Argsis correctly added to access theArgsstructure.
11-11: Ensureargsis properly initialized in allSceneContextinstancesWith the addition of the
argsfield toSceneContext, please verify that all instances whereSceneContextis created are updated to include the newargsfield. This will prevent potential issues due to uninitialized fields.Run the following script to identify all instantiations of
SceneContext:Verification successful
argsfield is properly initialized in allSceneContextinstancesThe
SceneContextstruct is correctly initialized insrc/foundations/app.zigwith all required fields, including the newargsfield. This initialization occurs during the main app setup, which appears to be the primary (or only) place whereSceneContextis created. All other uses ofSceneContextthroughout the application receive it as a parameter, ensuring that theargsfield is available whereverSceneContextis used.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all places where SceneContext is instantiated and check if 'args' is being initialized. # Search for 'SceneContext {' patterns and display the following 5 lines for context. rg --type zig '\bSceneContext\s*\{' -A 5Length of output: 42
Script:
#!/bin/bash # Verify Zig file recognition and search for SceneContext # Check if .zig files are recognized echo "Zig files in the repository:" fd -e zig echo "\nSearching for SceneContext usage:" rg --type zig '\bSceneContext\b' -C 3 echo "\nSearching for SceneContext definition:" rg --type zig '(struct|const)\s+SceneContext\s*=' -C 5Length of output: 38247
Script:
#!/bin/bash # Check the initialization of SceneContext in app.zig echo "Searching for SceneContext initialization in app.zig:" rg --type zig 'const scene_ctx: scenes\.SceneContext = \.' src/foundations/app.zig -C 10Length of output: 784
src/foundations/scenes/cgpoc/chapter6/earth/Earth.zig (1)
104-104: Verify the impact of reorderingaddProgramcall.Moving
self.view_camera.addProgram(prog, "f_mvp");after the texture setup might affect the rendering pipeline. Ensure that this change does not introduce unintended side effects.Run the following script to check for dependencies on the
addProgramcall order:src/foundations/scenes/cgpoc/chapter6/textured_torus/TexturedTorus.zig (2)
68-68: Initialization ofbrick_texturewith bindless support conditionThe initialization of
self.brick_texturenow properly considers thedisable_bindlessargument, enhancing flexibility based on user configurations.
105-105: Confirmation: Correct placement ofaddProgramcallMoving
self.view_camera.addProgram(prog, "f_mvp");after the texture setup ensures that the program is added regardless of the texture loading outcome. This change improves the rendering flow without negative side effects.src/foundations/rhi/Texture.zig (2)
121-121: Confirm the import ofShader.zig.Including
Shader.zigis necessary due to its usage in thefrag_shaderfunction.
26-26: Correct the extension support check ininitfunction.The condition checks the same extension twice, which seems unintended. If you meant to check a different extension in the second condition, please update it accordingly.
Consider correcting the condition:
- if (c.glfwExtensionSupported("GL_ARB_bindless_texture") != 1 or c.glfwExtensionSupported("GL_ARB_bindless_texture") != 1) { + if (c.glfwExtensionSupported("GL_ARB_bindless_texture") != 1 or c.glfwExtensionSupported("GL_ARB_texture_non_power_of_two") != 1) { t.disable_bindless = true; }Likely invalid or redundant comment.
| @@ -0,0 +1,22 @@ | |||
| disable_bindless: bool = false, | |||
|
|
|||
| const Args = @This(); | |||
There was a problem hiding this comment.
Remove unnecessary use of @This().
The line const Args = @This(); is redundant and unnecessary since Args is already defined as a struct. This could lead to confusion or compilation errors.
Apply this diff to remove the redundant line:
-const Args = @This();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.
| const Args = @This(); |
There was a problem hiding this comment.
@coderabbitai that is not redundant, this is an anonymous file struct and I have to use a reference via @This();
There was a problem hiding this comment.
@btipling, thank you for the clarification, and I apologize for the oversight. You're correct that in an anonymous file struct, using const Args = @This(); is necessary to create a reference to the struct. I'll keep that in mind for future reviews.
(/._.)/
Learnings added
Learnt from: btipling
PR: btipling/foundations#33
File: src/foundations/Args.zig:3-3
Timestamp: 2024-09-19T02:05:26.219Z
Learning: In Zig, when using an anonymous file struct, it's necessary to use `const StructName = @This();` to reference the struct within itself.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| pub fn init(allocator: std.mem.Allocator) Args { | ||
| const arg_values = std.process.argsAlloc(allocator) catch @panic("OOM"); | ||
| defer std.process.argsFree(allocator, arg_values); | ||
| var rv: Args = .{}; | ||
| for (arg_values) |av| { | ||
| std.debug.print("{s}\n", .{av}); | ||
| if (std.mem.eql(u8, av, "--disable_bindless")) { | ||
| rv.disable_bindless = true; | ||
| } | ||
| if (std.mem.eql(u8, av, "--help")) { | ||
| std.debug.print("--disable_bindless to disable bindless textures\n", .{}); | ||
| } | ||
| } | ||
| std.debug.print("args.disable_bindless: {any}\n", .{rv.disable_bindless}); | ||
| return rv; | ||
| } |
There was a problem hiding this comment.
Place const std = @import("std"); at the top of the file.
For better readability and to follow Zig conventions, the std library import should be placed at the beginning of the file.
Apply this diff to move the import statement:
+const std = @import("std");
+
pub fn init(allocator: std.mem.Allocator) Args {
const arg_values = std.process.argsAlloc(allocator) catch @panic("OOM");
defer std.process.argsFree(allocator, arg_values);
var rv: Args = .{};
for (arg_values) |av| {
- std.debug.print("{s}\n", .{av});
if (std.mem.eql(u8, av, "--disable_bindless")) {
rv.disable_bindless = true;
}
if (std.mem.eql(u8, av, "--help")) {
- std.debug.print("--disable_bindless to disable bindless textures\n", .{});
+ std.debug.print("--disable_bindless to disable bindless textures\n");
}
}
std.debug.print("args.disable_bindless: {}\n", .{rv.disable_bindless});
return rv;
}
-
-const std = @import("std");Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
@coderabbitai I prefer imports at the bottom.
There was a problem hiding this comment.
@btipling, understood. I'll keep that in mind for future reviews.
(_/)
( •︵•)
Learnings added
Learnt from: btipling
PR: btipling/foundations#33
File: src/foundations/Args.zig:5-20
Timestamp: 2024-09-19T02:06:13.320Z
Learning: In this codebase, imports are preferred to be placed at the bottom of the file.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @@ -0,0 +1,22 @@ | |||
| disable_bindless: bool = false, | |||
There was a problem hiding this comment.
Define the Args struct properly.
The field declaration disable_bindless: bool = false, is not enclosed within a struct. To represent command-line arguments effectively, you should define an Args struct that contains this field.
Apply this diff to define the Args struct:
+const Args = struct {
+ disable_bindless: bool = false,
+};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.
| disable_bindless: bool = false, | |
| const Args = struct { | |
| disable_bindless: bool = false, | |
| }; |
There was a problem hiding this comment.
@coderabbitai My approach is using the file struct feature of zig and it is defined properly.
There was a problem hiding this comment.
@btipling, thank you for the clarification. I understand that you're using the file struct feature of Zig, which allows the file itself to act as a struct. I'll keep this in mind for future reviews.
(_/)
( •_•)
/ >📝
Learnings added
Learnt from: btipling
PR: btipling/foundations#33
File: src/foundations/Args.zig:1-1
Timestamp: 2024-09-19T02:06:56.884Z
Learning: In this codebase, the file struct feature of Zig is used, where the file itself acts as a struct without explicit struct declarations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Learnings used
Learnt from: btipling
PR: btipling/foundations#33
File: src/foundations/Args.zig:5-20
Timestamp: 2024-09-19T02:06:13.498Z
Learning: In this codebase, imports are preferred to be placed at the bottom of the file.
| if (std.mem.eql(u8, av, "--help")) { | ||
| std.debug.print("--disable_bindless to disable bindless textures\n", .{}); | ||
| } |
There was a problem hiding this comment.
Consider using else if for efficiency.
Using separate if statements for mutually exclusive options can be less efficient. Use else if to prevent unnecessary checks when a condition has already been met.
Apply this diff to use else if:
if (std.mem.eql(u8, av, "--disable_bindless")) {
rv.disable_bindless = true;
}
- if (std.mem.eql(u8, av, "--help")) {
+ else if (std.mem.eql(u8, av, "--help")) {
std.debug.print("--disable_bindless to disable bindless textures\n");
}Committable suggestion was skipped due to low confidence.
| const Args = @This(); | ||
|
|
||
| pub fn init(allocator: std.mem.Allocator) Args { | ||
| const arg_values = std.process.argsAlloc(allocator) catch @panic("OOM"); |
There was a problem hiding this comment.
Handle errors gracefully without panicking.
Using catch @panic("OOM") abruptly terminates the program on error. Instead, consider propagating the error or providing a meaningful message to the user.
Apply this diff to handle the error properly:
- const arg_values = std.process.argsAlloc(allocator) catch @panic("OOM");
+ const arg_values = try std.process.argsAlloc(allocator);Update the function signature to return an error:
-pub fn init(allocator: std.mem.Allocator) Args {
+pub fn init(allocator: std.mem.Allocator) !Args {And ensure you handle the error when calling init.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
@coderabbitai I panic on oom intentionally
There was a problem hiding this comment.
@btipling, understood. If panicking on OOM is intentional in your codebase, then there's no need to change it. Thank you for the clarification.
Learnings added
Learnt from: btipling
PR: btipling/foundations#33
File: src/foundations/Args.zig:6-6
Timestamp: 2024-09-19T02:07:17.983Z
Learning: The codebase intentionally panics on out-of-memory conditions using `catch @panic("OOM")`, and this is the preferred behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| { | ||
| var s: rhi.Shader = .{ | ||
| .program = prog, | ||
| .instance_data = true, | ||
| .fragment_shader = .bindless, | ||
| .fragment_shader = rhi.Texture.frag_shader(self.shuttle_texture), |
There was a problem hiding this comment.
Handle potential null self.shuttle_texture when setting fragment shader
self.shuttle_texture might be null if the initialization fails. Passing it directly to rhi.Texture.frag_shader(self.shuttle_texture) could lead to unexpected behavior or errors. It's recommended to handle the null case to ensure the program uses a default shader when the texture is unavailable.
Apply this diff to handle the null texture:
.fragment_shader = if (self.shuttle_texture) |texture| {
rhi.Texture.frag_shader(texture)
} else {
+ rhi.default_frag_shader()
},Assuming rhi.default_frag_shader() provides a suitable default fragment shader.
Committable suggestion was skipped due to low confidence.
| it.setup(self.ctx.textures_loader.loadAsset("cgpoc\\luna\\ice.jpg") catch null, prog, "f_samp") catch { | ||
| self.ice_texture = null; | ||
| }; |
There was a problem hiding this comment.
Verify asset loading before calling it.setup
Similarly, ensure that self.ctx.textures_loader.loadAsset("cgpoc\\luna\\ice.jpg") succeeds before passing it to it.setup to prevent potential errors.
Apply this diff to add a null check:
if (self.ice_texture) |*it| {
- it.setup(self.ctx.textures_loader.loadAsset("cgpoc\\luna\\ice.jpg") catch null, prog, "f_samp") catch {
+ if (self.ctx.textures_loader.loadAsset("cgpoc\\luna\\ice.jpg")) |asset| {
+ it.setup(asset, prog, "f_samp") catch |err| {
+ std.debug.print("Failed to set up ice texture: {}\n", .{err});
+ self.ice_texture = null;
+ };
+ } else {
+ std.debug.print("Failed to load ice texture asset\n", .{});
+ self.ice_texture = null;
+ }
}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.
| it.setup(self.ctx.textures_loader.loadAsset("cgpoc\\luna\\ice.jpg") catch null, prog, "f_samp") catch { | |
| self.ice_texture = null; | |
| }; | |
| if (self.ctx.textures_loader.loadAsset("cgpoc\\luna\\ice.jpg")) |asset| { | |
| it.setup(asset, prog, "f_samp") catch |err| { | |
| std.debug.print("Failed to set up ice texture: {}\n", .{err}); | |
| self.ice_texture = null; | |
| }; | |
| } else { | |
| std.debug.print("Failed to load ice texture asset\n", .{}); | |
| self.ice_texture = null; | |
| }; |
| bt.setup(self.ctx.textures_loader.loadAsset("cgpoc\\luna\\brick1.jpg") catch null, prog, "f_samp") catch { | ||
| self.brick_texture = null; | ||
| }; |
There was a problem hiding this comment.
Verify asset loading before calling bt.setup
If self.ctx.textures_loader.loadAsset("cgpoc\\luna\\brick1.jpg") fails and returns null, passing it to bt.setup may cause an error. Check that the asset is successfully loaded before proceeding.
Apply this diff to add a null check:
if (self.brick_texture) |*bt| {
- bt.setup(self.ctx.textures_loader.loadAsset("cgpoc\\luna\\brick1.jpg") catch null, prog, "f_samp") catch {
+ if (self.ctx.textures_loader.loadAsset("cgpoc\\luna\\brick1.jpg")) |asset| {
+ bt.setup(asset, prog, "f_samp") catch |err| {
+ std.debug.print("Failed to set up brick texture: {}\n", .{err});
+ self.brick_texture = null;
+ };
+ } else {
+ std.debug.print("Failed to load brick texture asset\n", .{});
+ self.brick_texture = null;
+ }
}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.
| bt.setup(self.ctx.textures_loader.loadAsset("cgpoc\\luna\\brick1.jpg") catch null, prog, "f_samp") catch { | |
| self.brick_texture = null; | |
| }; | |
| if (self.brick_texture) |*bt| { | |
| if (self.ctx.textures_loader.loadAsset("cgpoc\\luna\\brick1.jpg")) |asset| { | |
| bt.setup(asset, prog, "f_samp") catch |err| { | |
| std.debug.print("Failed to set up brick texture: {}\n", .{err}); | |
| self.brick_texture = null; | |
| }; | |
| } else { | |
| std.debug.print("Failed to load brick texture asset\n", .{}); | |
| self.brick_texture = null; | |
| } | |
| } |
| self.brick_texture = rhi.Texture.init(self.ctx.args.disable_bindless) catch null; | ||
| self.ice_texture = rhi.Texture.init(self.ctx.args.disable_bindless) catch null; |
There was a problem hiding this comment.
Add error logging when initializing textures
Currently, if rhi.Texture.init fails, the error is silently caught, and the texture is set to null. To assist with debugging, consider logging the error when initialization fails.
Apply this diff to add error logging:
self.brick_texture = rhi.Texture.init(self.ctx.args.disable_bindless) catch |err| {
+ std.debug.print("Failed to initialize brick texture: {}\n", .{err});
null
};
self.ice_texture = rhi.Texture.init(self.ctx.args.disable_bindless) catch |err| {
+ std.debug.print("Failed to initialize ice texture: {}\n", .{err});
null
};Committable suggestion was skipped due to low confidence.
| { | ||
| var s: rhi.Shader = .{ | ||
| .program = prog, | ||
| .instance_data = true, | ||
| .fragment_shader = .bindless, | ||
| .fragment_shader = rhi.Texture.frag_shader(self.brick_texture), |
There was a problem hiding this comment.
Handle potential null self.brick_texture to prevent errors
If self.brick_texture is null, passing it to rhi.Texture.frag_shader may cause a runtime error. Ensure that self.brick_texture is not null before using it, or provide a fallback fragment shader.
Apply this diff to add a null check:
var s: rhi.Shader = .{
.program = prog,
.instance_data = true,
- .fragment_shader = rhi.Texture.frag_shader(self.brick_texture),
+ .fragment_shader = if (self.brick_texture) |bt| {
+ rhi.Texture.frag_shader(bt)
+ } else {
+ rhi.Shader.default_frag_shader, // Replace with an appropriate default shader
+ },
};Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation