Skip to content

support texture binding when bindless is not available #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 19, 2024
Merged

support texture binding when bindless is not available #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2bc9478
Support texture binding
btipling Sep 19, 2024
28b35bb
fix the torus
btipling Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/foundations/Args.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
disable_bindless: bool = false,
Copy link

@coderabbitai coderabbitai bot Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
disable_bindless: bool = false,
const Args = struct {
disable_bindless: bool = false,
};

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai My approach is using the file struct feature of zig and it is defined properly.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


const Args = @This();
Copy link

@coderabbitai coderabbitai bot Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
const Args = @This();

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai that is not redundant, this is an anonymous file struct and I have to use a reference via @This();

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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");
Copy link

@coderabbitai coderabbitai bot Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai I panic on oom intentionally

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

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", .{});
}
Comment on lines +14 to +16
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
std.debug.print("args.disable_bindless: {any}\n", .{rv.disable_bindless});
return rv;
}
Comment on lines +5 to +20
Copy link

@coderabbitai coderabbitai bot Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai I prefer imports at the bottom.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


const std = @import("std");
4 changes: 4 additions & 0 deletions src/foundations/app.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ var app: *App = undefined;
pub fn init(allocator: std.mem.Allocator) *App {
const glsl_version: []const u8 = "#version 460";

const args = Args.init(allocator);

const cfg = config.init(allocator);
errdefer cfg.deinit();
cfg.open();
Expand All @@ -37,6 +39,7 @@ pub fn init(allocator: std.mem.Allocator) *App {

const scene_ctx: scenes.SceneContext = .{
.cfg = cfg,
.args = args,
.textures_loader = textures_loader,
.obj_loader = obj_loader,
};
Expand Down Expand Up @@ -81,4 +84,5 @@ const ui = @import("ui/ui.zig");
const scenes = @import("scenes/scenes.zig");
const rhi = @import("rhi/rhi.zig");
const config = @import("config/config.zig");
const Args = @import("Args.zig");
const assets = @import("assets/assets.zig");
79 changes: 55 additions & 24 deletions src/foundations/rhi/Texture.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,45 +4,68 @@ handle: c.GLuint64 = 0,
uniform: Uniform = undefined,
wrap_s: c.GLint = c.GL_CLAMP_TO_EDGE,
wrap_t: c.GLint = c.GL_CLAMP_TO_EDGE,
disable_bindless: bool = false,

const Texture = @This();

pub const TextureError = error{
BindlessHandleCreationFailed,
BindlessNotSupported,
};

pub fn init(image: *assets.Image, program: u32, uniform_name: []const u8) TextureError!Texture {
var t: Texture = .{};
return t.setup(image, program, uniform_name);
pub fn frag_shader(tx: ?Texture) Shader.fragment_shader_type {
if (tx) |t| if (!t.disable_bindless) return .bindless;
return .texture;
Comment on lines +15 to +17
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify the frag_shader function logic.

To improve readability, consider combining the conditions in the frag_shader function.

Apply this diff to simplify the function:

pub fn frag_shader(tx: ?Texture) Shader.fragment_shader_type {
-    if (tx) |t| if (!t.disable_bindless) return .bindless;
+    if (tx and !tx.disable_bindless) {
+        return .bindless;
+    }
    return .texture;
}
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.

Suggested change
pub fn frag_shader(tx: ?Texture) Shader.fragment_shader_type {
if (tx) |t| if (!t.disable_bindless) return .bindless;
return .texture;
pub fn frag_shader(tx: ?Texture) Shader.fragment_shader_type {
if (tx and !tx.disable_bindless) {
return .bindless;
}
return .texture;
}

}

pub fn setup(self: *Texture, image: *assets.Image, program: u32, uniform_name: []const u8) TextureError!Texture {
pub fn init(disable_bindless: bool) TextureError!Texture {
var t: Texture = .{
.disable_bindless = disable_bindless,
};

if (c.glfwExtensionSupported("GL_ARB_bindless_texture") != 1 or c.glfwExtensionSupported("GL_ARB_bindless_texture") != 1) {
return TextureError.BindlessNotSupported;
t.disable_bindless = true;
}
return t;
}

pub fn setup(self: *Texture, image: ?*assets.Image, program: u32, uniform_name: []const u8) TextureError!void {
var name: u32 = undefined;
c.glCreateTextures(c.GL_TEXTURE_2D, 1, @ptrCast(&name));
c.glTextureParameteri(name, c.GL_TEXTURE_WRAP_S, self.wrap_s);
c.glTextureParameteri(name, c.GL_TEXTURE_WRAP_T, self.wrap_t);
c.glTextureParameteri(name, c.GL_TEXTURE_MIN_FILTER, c.GL_LINEAR_MIPMAP_LINEAR);
c.glTextureParameteri(name, c.GL_TEXTURE_MAG_FILTER, c.GL_LINEAR);
const w: f32 = @floatFromInt(image.width);
const h: f32 = @floatFromInt(image.height);
const mip_map_levels: c.GLsizei = @intFromFloat(@ceil(@log2(@max(w, h))));
c.glTextureStorage2D(name, mip_map_levels, c.GL_RGBA8, @intCast(image.width), @intCast(image.height));
c.glTextureSubImage2D(
name,
0,
0,
0,
@intCast(image.width),
@intCast(image.height),
c.GL_RGBA,
c.GL_UNSIGNED_BYTE,
image.stb_data.ptr,
);
if (image) |img| {
const w: f32 = @floatFromInt(img.width);
const h: f32 = @floatFromInt(img.height);
const mip_map_levels: c.GLsizei = @intFromFloat(@ceil(@log2(@max(w, h))));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix calculation of mipmap levels to prevent off-by-one error.

The calculation for mip_map_levels may result in an incorrect number of levels. The standard formula is floor(log2(max(width, height))) + 1.

Adjust the calculation as follows:

- const mip_map_levels: c.GLsizei = @intFromFloat(@ceil(@log2(@max(w, h))));
+ const mip_map_levels: c.GLsizei = @intFromFloat(@floor(@log2(@max(w, h)))) + 1;
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.

Suggested change
const mip_map_levels: c.GLsizei = @intFromFloat(@ceil(@log2(@max(w, h))));
const mip_map_levels: c.GLsizei = @intFromFloat(@floor(@log2(@max(w, h)))) + 1;

c.glTextureStorage2D(name, mip_map_levels, c.GL_RGBA8, @intCast(img.width), @intCast(img.height));
c.glTextureSubImage2D(
name,
0,
0,
0,
@intCast(img.width),
@intCast(img.height),
c.GL_RGBA,
c.GL_UNSIGNED_BYTE,
img.stb_data.ptr,
);
} else {
const magenta_rgba_color = [4]u8{ 255, 0, 255, 255 };
c.glTextureStorage2D(name, 1, c.GL_RGBA8, 1, 1);
c.glTextureSubImage2D(
name,
0,
0,
0,
1,
1,
c.GL_RGBA,
c.GL_UNSIGNED_BYTE,
&magenta_rgba_color,
);
}

c.glGenerateTextureMipmap(name);
if (c.glfwExtensionSupported("GL_EXT_texture_filter_anisotropic") == 1) {
Expand All @@ -53,6 +76,11 @@ pub fn setup(self: *Texture, image: *assets.Image, program: u32, uniform_name: [

self.name = name;

self.uniform = Uniform.init(program, uniform_name);

if (self.disable_bindless) {
return;
}
// Generate bindless handle
self.handle = c.glGetTextureHandleARB(self.name);
if (self.handle == 0) {
Expand All @@ -62,9 +90,7 @@ pub fn setup(self: *Texture, image: *assets.Image, program: u32, uniform_name: [
// Make the texture resident
c.glMakeTextureHandleResidentARB(self.handle);

self.uniform = Uniform.init(program, uniform_name);

return self.*;
return;
}

pub fn makeNonResident(self: Texture) void {
Expand All @@ -81,10 +107,15 @@ pub fn deinit(self: Texture) void {
}

pub fn bind(self: Texture) void {
if (self.disable_bindless) {
c.glBindTextureUnit(self.texture_unit, self.name);
return;
}
self.uniform.setUniformHandleui64ARB(self.handle);
}

const std = @import("std");
const c = @import("../c.zig").c;
const Uniform = @import("Uniform.zig");
const assets = @import("../assets/assets.zig");
const Shader = @import("Shader.zig");
22 changes: 11 additions & 11 deletions src/foundations/scenes/cgpoc/chapter5/textured_pyramid/TexturedPyramid.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,13 @@ pub fn updateCamera(_: *TexturedPyramid) void {}

pub fn renderParallepiped(self: *TexturedPyramid) void {
const prog = rhi.createProgram();
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;
Comment on lines +91 to +92
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
const partials = [_][]const u8{vertex_shader};
s.attach(self.allocator, @ptrCast(partials[0..]));
Expand All @@ -119,17 +121,15 @@ pub fn renderParallepiped(self: *TexturedPyramid) void {
),
};
self.view_camera.addProgram(prog, "f_mvp");

if (self.ctx.textures_loader.loadAsset("cgpoc\\luna\\brick1.jpg") catch null) |img| {
self.brick_texture = rhi.Texture.init(img, prog, "f_samp") catch null;
} else {
std.debug.print("no brick image\n", .{});
if (self.brick_texture) |*bt| {
bt.setup(self.ctx.textures_loader.loadAsset("cgpoc\\luna\\brick1.jpg") catch null, prog, "f_samp") catch {
self.brick_texture = null;
};
Comment on lines +125 to +127
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;
}
}

}

if (self.ctx.textures_loader.loadAsset("cgpoc\\luna\\ice.jpg") catch null) |img| {
self.ice_texture = rhi.Texture.init(img, prog, "f_samp") catch null;
} else {
std.debug.print("no ice image\n", .{});
if (self.ice_texture) |*it| {
it.setup(self.ctx.textures_loader.loadAsset("cgpoc\\luna\\ice.jpg") catch null, prog, "f_samp") catch {
self.ice_texture = null;
};
Comment on lines +130 to +132
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;
};

}
self.pyramid = pyramid;
}
Expand Down
14 changes: 7 additions & 7 deletions src/foundations/scenes/cgpoc/chapter6/earth/Earth.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,12 @@ pub fn updateCamera(_: *Earth) void {}

pub fn renderSphere(self: *Earth) void {
const prog = rhi.createProgram();
self.earth_texture = rhi.Texture.init(self.ctx.args.disable_bindless) catch null;
{
var s: rhi.Shader = .{
.program = prog,
.instance_data = true,
.fragment_shader = .bindless,
.fragment_shader = rhi.Texture.frag_shader(self.earth_texture),
Copy link

@coderabbitai coderabbitai bot Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure earth_texture is non-null before setting the fragment shader.

If self.earth_texture is null, calling rhi.Texture.frag_shader(self.earth_texture) may cause a runtime error. Consider adding a null check and providing a default shader when the texture is not available.

Apply this diff to safely assign the fragment shader:

 var s: rhi.Shader = .{
     .program = prog,
     .instance_data = true,
-    .fragment_shader = rhi.Texture.frag_shader(self.earth_texture),
+    .fragment_shader = if (self.earth_texture) |et| rhi.Texture.frag_shader(et) else default_fragment_shader,
 };

Ensure that default_fragment_shader is defined appropriately in your context.

Committable suggestion was skipped due to low confidence.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai frag_shader handles nulls

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@btipling, thank you for the clarification. I've noted that frag_shader handles nulls.

(_/)
(•_•)
(>🍃<)

Learnings added 
Learnt from: btipling
PR: btipling/foundations#33
File: src/foundations/scenes/cgpoc/chapter6/earth/Earth.zig:73-73
Timestamp: 2024-09-19T02:08:33.481Z
Learning: The function `rhi.Texture.frag_shader` handles `null` textures without causing runtime errors.

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.

};
const partials = [_][]const u8{vertex_shader};
s.attach(self.allocator, @ptrCast(partials[0..]));
Expand All @@ -95,13 +96,12 @@ pub fn renderSphere(self: *Earth) void {
false,
),
};
self.view_camera.addProgram(prog, "f_mvp");

if (self.ctx.textures_loader.loadAsset("cgpoc\\earth.jpg") catch null) |img| {
self.earth_texture = rhi.Texture.init(img, prog, "f_samp") catch null;
} else {
std.debug.print("no earth image\n", .{});
if (self.earth_texture) |*bt| {
bt.setup(self.ctx.textures_loader.loadAsset("cgpoc\\earth.jpg") catch null, prog, "f_samp") catch {
self.earth_texture = null;
};
}
self.view_camera.addProgram(prog, "f_mvp");
self.sphere = sphere;
}

Expand Down
14 changes: 7 additions & 7 deletions src/foundations/scenes/cgpoc/chapter6/shuttle/Shuttle.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,12 @@ pub fn renderShuttle(self: *Shuttle) void {
}

const prog = rhi.createProgram();
self.shuttle_texture = rhi.Texture.init(self.ctx.args.disable_bindless) catch null;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log errors when initializing shuttle texture fails

Currently, if rhi.Texture.init(self.ctx.args.disable_bindless) fails, the error is caught and self.shuttle_texture is set to null without any logging. It would be helpful to log the error to aid in debugging texture initialization issues.

Apply this diff to log the error:

 self.shuttle_texture = rhi.Texture.init(self.ctx.args.disable_bindless) catch |e| {
+    std.debug.print("Failed to initialize shuttle texture: {}\n", .{e});
     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.

Suggested change
self.shuttle_texture = rhi.Texture.init(self.ctx.args.disable_bindless) catch null;
self.shuttle_texture = rhi.Texture.init(self.ctx.args.disable_bindless) catch |e| {
std.debug.print("Failed to initialize shuttle texture: {}\n", .{e});
null
};

{
var s: rhi.Shader = .{
.program = prog,
.instance_data = true,
.fragment_shader = .bindless,
.fragment_shader = rhi.Texture.frag_shader(self.shuttle_texture),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
const partials = [_][]const u8{vertex_shader};
s.attach(self.allocator, @ptrCast(partials[0..]));
Expand All @@ -97,14 +98,13 @@ pub fn renderShuttle(self: *Shuttle) void {
};
i_datas[0] = i_data;
}
if (self.shuttle_texture) |*bt| {
bt.setup(self.ctx.textures_loader.loadAsset("cgpoc\\NasaShuttle\\spstob_1.jpg") catch null, prog, "f_samp") catch {
self.shuttle_texture = null;
};
}
Comment on lines +101 to +105
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider refactoring texture initialization and setup into a helper function

To improve code readability and maintainability, consider encapsulating the texture initialization and setup logic into a helper function. This would reduce code duplication and provide a single point of modification for texture handling.

Example:

fn initializeShuttleTexture(self: *Shuttle) void {
    self.shuttle_texture = rhi.Texture.init(self.ctx.args.disable_bindless) catch |e| {
        std.debug.print("Failed to initialize shuttle texture: {}\n", .{e});
        null
    };
    if (self.shuttle_texture) |*bt| {
        bt.setup(
            self.ctx.textures_loader.loadAsset("cgpoc\\NasaShuttle\\spstob_1.jpg") catch null,
            prog,
            "f_samp",
        ) catch |e| {
            self.shuttle_texture = null;
            std.debug.print("Failed to setup shuttle texture: {}\n", .{e});
        };
    }
}

Then call this function in renderShuttle:

+    self.initializeShuttleTexture();

Log errors when shuttle texture setup fails

When bt.setup fails, the error is caught, and self.shuttle_texture is set to null, but the error details are not logged. Logging the error can help diagnose issues during texture setup.

Apply this diff to log the error:

 bt.setup(self.ctx.textures_loader.loadAsset("cgpoc\\NasaShuttle\\spstob_1.jpg") catch null, prog, "f_samp") catch |e| {
     self.shuttle_texture = null;
+    std.debug.print("Failed to setup shuttle texture: {}\n", .{e});
 };
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.

Suggested change
if (self.shuttle_texture) |*bt| {
bt.setup(self.ctx.textures_loader.loadAsset("cgpoc\\NasaShuttle\\spstob_1.jpg") catch null, prog, "f_samp") catch {
self.shuttle_texture = null;
};
}
if (self.shuttle_texture) |*bt| {
bt.setup(self.ctx.textures_loader.loadAsset("cgpoc\\NasaShuttle\\spstob_1.jpg") catch null, prog, "f_samp") catch |e| {
self.shuttle_texture = null;
std.debug.print("Failed to setup shuttle texture: {}\n", .{e});
};
}

const shuttle_object: object.object = shuttle_model.toObject(prog, i_datas[0..]);
self.view_camera.addProgram(prog, "f_mvp");

if (self.ctx.textures_loader.loadAsset("cgpoc\\NasaShuttle\\spstob_1.jpg") catch null) |img| {
self.shuttle_texture = rhi.Texture.init(img, prog, "f_samp") catch null;
} else {
std.debug.print("no shuttle image\n", .{});
}
self.shuttle = shuttle_object;
}

Expand Down
16 changes: 8 additions & 8 deletions src/foundations/scenes/cgpoc/chapter6/textured_torus/TexturedTorus.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,12 @@ pub fn updateCamera(_: *TexturedTorus) void {}

pub fn renderTorus(self: *TexturedTorus) void {
const prog = rhi.createProgram();
self.brick_texture = rhi.Texture.init(self.ctx.args.disable_bindless) catch null;
{
var s: rhi.Shader = .{
.program = prog,
.instance_data = true,
.fragment_shader = .bindless,
.fragment_shader = rhi.Texture.frag_shader(self.brick_texture),
};
const partials = [_][]const u8{vertex_shader};
s.attach(self.allocator, @ptrCast(partials[0..]));
Expand All @@ -95,14 +96,13 @@ pub fn renderTorus(self: *TexturedTorus) void {
false,
),
};
self.view_camera.addProgram(prog, "f_mvp");

if (self.ctx.textures_loader.loadAsset("cgpoc\\luna\\brick1.jpg") catch null) |img| {
var t: rhi.Texture = .{ .wrap_s = c.GL_REPEAT };
self.brick_texture = t.setup(img, prog, "f_samp") catch null;
} else {
std.debug.print("no brick image\n", .{});
if (self.brick_texture) |*bt| {
bt.wrap_s = c.GL_REPEAT;
bt.setup(self.ctx.textures_loader.loadAsset("cgpoc\\luna\\brick1.jpg") catch null, prog, "f_samp") catch {
self.brick_texture = null;
};
}
self.view_camera.addProgram(prog, "f_mvp");
self.torus = torus;
}

Expand Down
2 changes: 2 additions & 0 deletions src/foundations/scenes/scenes.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const Scenes = @This();

pub const SceneContext = struct {
cfg: *const config,
args: Args,
textures_loader: *assets.loader.Loader(assets.Image),
obj_loader: *assets.loader.Loader(assets.Obj),
};
Expand Down Expand Up @@ -76,6 +77,7 @@ pub fn drawScene(self: Scenes, frame_time: f64) void {
const std = @import("std");
const ui = @import("../ui/ui.zig");
const config = @import("../config/config.zig");
const Args = @import("../Args.zig");

pub const cgpoc = @import("cgpoc/cgpoc.zig");
pub const color = @import("color/color.zig");
Expand Down