Support lighting models with textures #35
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes include the addition of new shaders, modifications to existing shaders, and updates to scene management in the rendering pipeline. A new Changes
Possibly related PRs
Poem
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: 15
Outside diff range and nitpick comments (5)
src/foundations/scenes/cgpoc/chapter7/dolphin/blinn_phong_frag.glsl (3)
5-5: Consider using a dynamic array or a smaller fixed size forf_ls.The local array
f_lsis initialized with a fixed size of 10, but only the first element is used in the code. If the actual number of lights needed is much smaller, this could be wasteful in terms of performance and memory usage.Consider using a dynamic array or a smaller fixed size based on the actual number of lights required for the scene.
8-8: Consider making the global ambient color configurable or adjusting the values.The global ambient color is currently hardcoded with RGB values of (0.7, 0.7, 0.7), which results in a relatively bright ambient light. This might not be suitable for all scenes or lighting conditions and could wash out the details and contrast of the rendered object.
Consider making the global ambient color configurable or adjusting the values based on the specific requirements of the scene to achieve the desired visual outcome.
9-9: Consider making the blending factors configurable or adaptable.The current code equally weights the texture color and the lighting contribution (0.5 each) in the final fragment color. While this might produce acceptable results in some cases, it might not always be the desired visual outcome, depending on the specific material properties, lighting conditions, and artistic intent.
Consider making the blending factors configurable or adaptable based on the material properties and the desired visual style. This could provide more flexibility and control over the final appearance of the rendered object.
src/foundations/scenes/cgpoc/chapter7/dolphin/blinn_phong_vert.glsl (1)
10-10: Clarify the purpose off_xup.The normal matrix calculation is correct. However, please provide more context on the purpose and origin of
f_xupto improve code readability and maintainability.src/foundations/scenes/cgpoc/chapter7/dolphin/Dolphin.zig (1)
101-101: Consider implementing or removing the emptyupdateCamerafunctionThe
updateCamerafunction is currently empty. If it's intended for future use, add aTODOcomment specifying the planned implementation. If not needed, consider removing it to clean up the code.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (18)
- src/foundations/rhi/Shader.zig (2 hunks)
- src/foundations/scenes/cgpoc/chapter6/shuttle/Shuttle.zig (1 hunks)
- src/foundations/scenes/cgpoc/chapter7/chapter7.zig (1 hunks)
- src/foundations/scenes/cgpoc/chapter7/dolphin/Dolphin.zig (1 hunks)
- src/foundations/scenes/cgpoc/chapter7/dolphin/blinn_phong_frag.glsl (1 hunks)
- src/foundations/scenes/cgpoc/chapter7/dolphin/blinn_phong_vert.glsl (1 hunks)
- src/foundations/scenes/cgpoc/chapter7/lighting/blinn_phong_frag.glsl (1 hunks)
- src/foundations/scenes/cgpoc/chapter7/lighting/phong_frag.glsl (1 hunks)
- src/foundations/scenes/scenes.zig (1 hunks)
- src/foundations/shaders/frag_bindless.glsl (0 hunks)
- src/foundations/shaders/frag_bindless_header.glsl (1 hunks)
- src/foundations/shaders/frag_blinn_phong_lighting.glsl (1 hunks)
- src/foundations/shaders/frag_texture.glsl (0 hunks)
- src/foundations/shaders/frag_texture_header.glsl (1 hunks)
- src/foundations/shaders/i_obj_vert.glsl (1 hunks)
- src/foundations/shaders/i_obj_wavefront_vert.glsl (0 hunks)
- src/foundations/shaders/lighting.glsl (2 hunks)
- src/foundations/ui/ui_state.zig (2 hunks)
Files not reviewed due to no reviewable changes (3)
- src/foundations/shaders/frag_bindless.glsl
- src/foundations/shaders/frag_texture.glsl
- src/foundations/shaders/i_obj_wavefront_vert.glsl
Additional comments not posted (22)
src/foundations/shaders/frag_texture_header.glsl (1)
2-2: LGTM!The addition of the
uniform sampler2D f_sampvariable bound to location 0 is a necessary step towards supporting textures in the lighting models, which aligns with the PR objective.Ensure that the application code sets up the texture correctly and binds it to the same texture unit (0) before rendering.
src/foundations/shaders/frag_bindless_header.glsl (1)
3-3: LGTM!The new uniform variable
f_sampwith thebindless_samplerlayout qualifier is declared correctly. This change enhances the shader's capability to utilize bindless textures, allowing for more efficient texture sampling. It aligns well with the PR objective of supporting lighting models with textures.src/foundations/scenes/cgpoc/chapter7/chapter7.zig (1)
2-2: LGTM!The import statement for the
Dolphincomponent is syntactically correct and follows the expected pattern for importing a component from a separate file.The public visibility of the constant is appropriate if the
Dolphincomponent is intended to be used by other modules that import this file.src/foundations/shaders/i_obj_vert.glsl (1)
10-10: Verify the intended behavior of the additional transformation.The change incorporates an additional multiplication by
f_xupmatrix, which affects the final position of the vertex. This appears to be an intentional change to support lighting models with textures, as mentioned in the PR objectives.Please verify that the change produces the intended behavior and does not introduce any unintended side effects. Consider adding a test case to ensure the correctness of the transformation.
src/foundations/scenes/cgpoc/chapter7/dolphin/blinn_phong_frag.glsl (4)
6-6: Verify the initialization of thef_lightsarray.Ensure that the
f_lightsarray is properly initialized and contains at least one element before accessing it. If the array is empty or not properly initialized, it could lead to undefined behavior or rendering issues.Run the following script to verify the initialization:
#!/bin/bash # Description: Verify the initialization of the `f_lights` array. # Test: Search for the declaration and initialization of `f_lights`. Expect: At least one occurrence. rg --type glsl $'f_lights'
9-9: Verify the implementation and correctness of thef_blinn_phong_lightingfunction.Ensure that the
f_blinn_phong_lightingfunction is properly implemented and returns the expected lighting contribution based on the provided material, light array, light count, and global ambient color. Incorrect implementation could lead to incorrect lighting or visual artifacts in the rendered output.Run the following script to verify the implementation:
#!/bin/bash # Description: Verify the implementation of the `f_blinn_phong_lighting` function. # Test: Search for the definition of the `f_blinn_phong_lighting` function. Expect: One occurrence. ast-grep --lang glsl --pattern $'f_blinn_phong_lighting($_, $_, $_, $_) { $$$ }'
4-4: Verify the initialization of thef_materialsarray.Ensure that the
f_materialsarray is properly initialized and contains at least one element before accessing it. If the array is empty or not properly initialized, it could lead to undefined behavior or rendering issues.Run the following script to verify the initialization:
7-7: Verify the setup of the sampler and texture coordinates.Ensure that the
f_sampsampler andf_tctexture coordinates are properly initialized and valid before using them for texture sampling. Incorrect setup could lead to visual artifacts or unexpected appearance of the rendered object.Run the following script to verify the setup:
Verification successful
Further verify the declarations of
f_sampandf_tcby searching the entire codebase.Please run the following scripts to search for the declarations and initializations of
f_sampandf_tcwithout restricting the file type:
Correct the shell script syntax and retry searching for
f_sampandf_tc.Please run the following corrected scripts to search for the declarations and initializations of
f_sampandf_tcacross all files:
Correct the shell script syntax and retry searching for
f_sampandf_tc.Please run the following corrected scripts to search for the declarations and initializations of
f_sampandf_tcacross all files:
Sampler and texture coordinates are properly set up and used.
The verification process has confirmed that both
f_samp(sampler) andf_tc(texture coordinates) are correctly initialized and used throughout the codebase. The sampler is properly set up in the Zig code before being used in shaders, and the texture coordinates are correctly passed between vertex and fragment shaders. There are no apparent issues with their implementation or usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the setup of the sampler and texture coordinates. # Test: Search for the declaration and initialization of `f_samp` and `f_tc`. Expect: At least one occurrence each. rg --type glsl $'f_samp' rg --type glsl $'f_tc'Length of output: 192
Script:
#!/bin/bash # Description: Search for declarations and initializations of `f_samp` and `f_tc` across all files. # Search for `f_samp` rg 'sampler|'f_samp' -A 2 # Search for `f_tc` rg 'vec[234]|f_tc' -A 2Length of output: 93
Script:
#!/bin/bash # Description: Search for declarations and initializations of `f_samp` and `f_tc` across all files. # Search for `f_samp` rg "sampler|f_samp" -A 2 # Search for `f_tc` rg "vec[234]|f_tc" -A 2Length of output: 149555
Script:
#!/bin/bash # Description: Search for declarations and initializations of `f_samp` and `f_tc` across all files. # Search for `f_samp` rg "sampler|f_samp" -A 2 # Search for `f_tc` rg "vec[234]|f_tc" -A 2Length of output: 149555
src/foundations/scenes/cgpoc/chapter7/dolphin/blinn_phong_vert.glsl (3)
2-9: LGTM!The transformation matrix is constructed correctly using the provided input vectors.
12-14: LGTM!The vertex position and normals are transformed correctly and assigned to the appropriate output variables.
16-17: LGTM!The final clip space position and texture coordinates are computed and assigned correctly.
src/foundations/scenes/scenes.zig (1)
23-23: Provide more context about theseven_dolphinscene.The change from
seven_lightingtoseven_dolphinaligns with the PR objectives of supporting lighting models with textures. However, it would be helpful to provide more information about the specific configuration and features of theseven_dolphinscene.
- How does the
seven_dolphinscene differ from theseven_lightingscene in terms of its lighting models and texture support?- What specific improvements or additions does the
seven_dolphinscene bring to the project?Providing this context will help reviewers better understand the purpose and impact of this change.
src/foundations/ui/ui_state.zig (2)
35-35: LGTM!The addition of the
seven_dolphinenum value to thescene_typeenum is consistent with the existing code and aligns with the PR objective of supporting lighting models with textures.
72-72: Verify the usage of the newseven_dolphinscene type andDolphinstructure.The addition of the
seven_dolphinunion entry to thescenesunion is consistent with the existing code and aligns with the PR objective of supporting lighting models with textures. However, please ensure that the corresponding changes have been made in other parts of the codebase to handle the rendering and management of the newDolphinscene.Run the following script to verify the usage of the new scene type and structure:
Verification successful
Usage of the new
seven_dolphinscene type andDolphinstructure verified.The
seven_dolphinscene type is properly initialized and utilized in bothui_state.zigandscenes.zig. Additionally, theDolphinstructure fromscenes_list.cgpoc.chapter7is correctly referenced inui_state.zig. No further modifications are necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new `seven_dolphin` scene type and `Dolphin` structure. # Test 1: Search for the usage of the `seven_dolphin` scene type. Expect: At least one occurrence. rg --type zig $'seven_dolphin' # Test 2: Search for the usage of the `Dolphin` structure. Expect: At least one occurrence. rg --type zig $'scenes_list\.cgpoc\.chapter7\.Dolphin'Length of output: 406
src/foundations/scenes/cgpoc/chapter6/shuttle/Shuttle.zig (1)
9-9: Verify the impact of the vertex shader change.The change in the vertex shader file path looks good. However, please ensure that the new vertex shader (
i_obj_vert.glsl) is compatible with the rest of the code and produces the desired visual output for the shuttle object.Run the following script to verify the rendering of the shuttle object:
Please visually inspect the rendered shuttle object to confirm that it appears as expected with the new vertex shader.
src/foundations/rhi/Shader.zig (2)
52-52: LGTM!The addition of the
frag_texture_headerconstant follows the existing pattern and naming convention for fragment shader headers. The embedded GLSL code will be used to support a new type of fragment shader for handling textures.
101-103: Looks good!The new conditional branch correctly handles the
.texturefragment shader type by adding thefrag_texture_headerto the shader source. This change is consistent with the existing logic for other shader types and ensures that the appropriate header is included for texture shaders.src/foundations/shaders/frag_blinn_phong_lighting.glsl (1)
9-9: Function call tof_blinn_phong_lightingappears correctThe updated function call to
f_blinn_phong_lightingincludes the light arrayf_ls, the number of lights1, and the global ambient lightf_global_ambient. This aligns with the changes made for supporting multiple lights and ambient lighting.src/foundations/scenes/cgpoc/chapter7/lighting/blinn_phong_frag.glsl (1)
14-14: Verify the correct usage off_blinn_phong_lightingwith new parametersThe function call to
f_blinn_phong_lightingnow includes additional parameters. Ensure that the function definition is updated accordingly and that all its internal logic handles these changes correctly.Run the following script to check the function definition:
Verification successful
Verified:
f_blinn_phong_lightingusage and implementation are correctThe function
f_blinn_phong_lightingis correctly defined with the updated parameters:Material,Light[],uint, andvec4. The internal logic appropriately handles the additionalvec4 f_ambientparameter, ensuring accurate lighting calculations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Locate the definition of `f_blinn_phong_lighting` and verify its parameters. # Expected: Function signature matches the updated call with parameters (Material, Light[], int, vec4). rg --type-add glsl:'*.glsl' -tglsl "vec4 f_blinn_phong_lighting" -A 2Length of output: 372
Script:
#!/bin/bash # Description: Retrieve the full definition and implementation of `f_blinn_phong_lighting` to verify the handling of additional parameters. rg --type-add glsl:'*.glsl' -A 20 "vec4 f_blinn_phong_lighting" src/foundations/shaders/lighting.glslLength of output: 959
Script:
#!/bin/bash # Description: Retrieve the remaining lines of the `f_blinn_phong_lighting` function to complete the verification of parameter handling. rg --type-add glsl:'*.glsl' -A 50 "vec4 f_blinn_phong_lighting" src/foundations/shaders/lighting.glslLength of output: 1893
src/foundations/scenes/cgpoc/chapter7/lighting/phong_frag.glsl (2)
15-15: Verify the correctness ofnum_lightsparameter inf_phong_lighting.Ensure that the third argument passed to
f_phong_lightingaccurately reflects the number of initialized lights inf_lsto prevent unintended behavior.Run the following script to confirm that the
num_lightsparameter matches the number of initialized lights:#!/bin/bash # Description: Check that the num_lights parameter equals the number of initialized lights. # Count the number of initialized lights in f_ls initialized_lights=$(rg --type glsl -o 'f_ls\[\d+\]' src/foundations/scenes/cgpoc/chapter7/lighting/ | sort | uniq | wc -l) # Extract the num_lights argument passed to f_phong_lighting num_lights=$(rg --type glsl 'f_phong_lighting\([^,]+, [^,]+, (\d+),' src/foundations/scenes/cgpoc/chapter7/lighting/ | grep -oP '\d+') echo "Initialized lights: $initialized_lights" echo "Num lights in function call: $num_lights" # Expected: The numbers should match.
8-13: Ensure consistency in light source initialization.While you've initialized two light sources, make sure that any future additions to
f_lsare handled appropriately to prevent potential out-of-bounds errors or uninitialized data whennum_lightsincreases.Run the following script to check for any references to
f_lsbeyond the initialized indices:src/foundations/shaders/lighting.glsl (1)
55-55: Verify Consistency of Specular Exponent CalculationIn
f_blinn_phong_lighting, the specular exponent is calculated usingf_mat.shininess * 4.0, whereas inf_phong_lighting, it's calculated withf_mat.shininessalone. Please confirm if this discrepancy is intentional to ensure consistent lighting effects.Also applies to: 80-80
| Light f_ls[10]; | ||
| f_ls[0] = f_lights[0]; |
There was a problem hiding this comment.
Initialize all lights or adjust the array size
The array f_ls is declared with a fixed size of 10, but only f_ls[0] is initialized. If the lighting calculations utilize other elements of the array, there is a risk of accessing uninitialized data. Consider initializing all elements of f_ls or adjusting the array size to match the actual number of lights.
Apply this diff to adjust the array size:
-Light f_ls[10];
+Light f_ls[1];Or, if you intend to support multiple lights, initialize all required lights:
const int NUM_LIGHTS = 10;
Light f_ls[NUM_LIGHTS];
for (int i = 0; i < NUM_LIGHTS; ++i) {
f_ls[i] = f_lights[i];
}There was a problem hiding this comment.
@coderabbitai I supply a number arg to f_blinn_phong_lighting
There was a problem hiding this comment.
@btipling, thank you for the clarification. I recall that in this codebase, arrays like f_ls are declared with larger sizes to accommodate future use, and the actual number of active lights is supplied to f_blinn_phong_lighting. My apologies for the oversight; I'll keep this in mind for future reviews.
Learnings used
Learnt from: btipling
PR: btipling/foundations#35
File: src/foundations/scenes/cgpoc/chapter7/lighting/phong_frag.glsl:0-0
Timestamp: 2024-09-23T00:22:00.064Z
Learning: In the codebase, arrays like `f_ls` for light sources may be declared with larger sizes (e.g., `Light f_ls[10]`) to accommodate future use of additional lights. The actual number of active lights is supplied as an argument to functions like `f_phong_lighting`.
|
|
||
| fo_frag_color = f_blinn_phong_lighting(f_m, f_l); | ||
| vec4 f_global_ambient = vec4(0.7, 0.7, 0.7, 1.0); |
There was a problem hiding this comment.
Consider making f_global_ambient a uniform variable
Currently, f_global_ambient is hardcoded within the shader. To provide greater flexibility and allow dynamic adjustment of ambient light, consider declaring f_global_ambient as a uniform variable. This change enables you to modify the ambient light from your application without altering the shader code.
Apply this diff to declare f_global_ambient as a uniform:
- vec4 f_global_ambient = vec4(0.7, 0.7, 0.7, 1.0);
+ uniform vec4 f_global_ambient;Then, set the uniform value from your application code as needed.
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.
| vec4 f_global_ambient = vec4(0.7, 0.7, 0.7, 1.0); | |
| uniform vec4 f_global_ambient; |
There was a problem hiding this comment.
@coderabbitai fixed in 02cf5c1
There was a problem hiding this comment.
@btipling Great, thank you for fixing the issue!
(_/)
( •_•)
🐇
| fo_frag_color = f_blinn_phong_lighting(f_m, f_ls, f_l_dirs); | ||
| Light f_ls[10]; | ||
| f_ls[0] = f_lights[0]; | ||
| f_ls[0].direction = vec4(fo_light_1_dir, 1.0); |
There was a problem hiding this comment.
Incorrect w-component for directional light vectors
Setting the w-component to 1.0 denotes a positional vector, not a directional one. For directional lights, the w-component should be 0.0 to represent a vector at infinity.
Apply this diff to fix the w-component:
-f_ls[0].direction = vec4(fo_light_1_dir, 1.0);
+f_ls[0].direction = vec4(fo_light_1_dir, 0.0);
-f_ls[1].direction = vec4(fo_light_2_dir, 1.0);
+f_ls[1].direction = vec4(fo_light_2_dir, 0.0);Also applies to: 12-12
There was a problem hiding this comment.
@coderabbitai fixed in 02cf5c1
| Light f_ls[2] = Light[2](f_lights[0], f_lights[1]); | ||
| vec3 f_l_dirs[2] = vec3[2](fo_light_1_dir, fo_light_2_dir); | ||
| fo_frag_color = f_blinn_phong_lighting(f_m, f_ls, f_l_dirs); | ||
| Light f_ls[10]; |
There was a problem hiding this comment.
Uninitialized elements in the light array may cause issues
Only the first two elements of f_ls are initialized, while the remaining entries are uninitialized. If f_blinn_phong_lighting accesses these uninitialized elements, it could lead to undefined behavior or errors.
Consider resizing the light array to match the number of lights in use:
-Light f_ls[10];
+Light f_ls[2];Alternatively, ensure all elements of f_ls are properly initialized before use.
Also applies to: 14-14
There was a problem hiding this comment.
I supply a number arg to f_blinn_phong_lighting
There was a problem hiding this comment.
@btipling, thank you for the clarification. Since you're supplying the number of lights to f_blinn_phong_lighting, and the function uses this argument to limit access to the array, uninitialized elements in f_ls should not cause any issues. I appreciate the explanation.
(_/)
(o.O)
( )
Learnings added
Learnt from: btipling
PR: btipling/foundations#35
File: src/foundations/scenes/cgpoc/chapter7/lighting/blinn_phong_frag.glsl:8-8
Timestamp: 2024-09-23T00:22:35.464Z
Learning: In GLSL shaders, when passing an array to functions like `f_blinn_phong_lighting` along with a count parameter, it's acceptable to have uninitialized elements in the array as long as the function uses the count to limit access to the initialized elements.
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.
| f_ls[0].direction = vec4(fo_light_1_dir, 1.0); | ||
| f_ls[1] = f_lights[1]; | ||
| f_ls[1].direction = vec4(fo_light_2_dir, 1.0); | ||
| vec4 f_global_ambient = vec4(0.7, 0.7, 0.7, 1.0); |
There was a problem hiding this comment.
Make f_global_ambient a uniform for flexibility
Hardcoding the global ambient light limits flexibility. By declaring f_global_ambient as a uniform, you allow the ambient light to be configured externally, accommodating different scenes and lighting conditions.
Apply this diff to adjust f_global_ambient:
-vec4 f_global_ambient = vec4(0.7, 0.7, 0.7, 1.0);
+uniform vec4 f_global_ambient;Remember to set f_global_ambient from the application code supplying shader uniforms.
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.
| vec4 f_global_ambient = vec4(0.7, 0.7, 0.7, 1.0); | |
| uniform vec4 f_global_ambient; |
There was a problem hiding this comment.
@coderabbitai fixed in 02cf5c1
There was a problem hiding this comment.
@btipling, thanks for fixing this!
(_/)
( ^.^ )
(") (")
| vec3 f_H = normalize(f_L + f_V).xyz; | ||
|
|
||
| float cosTheta = dot(f_L, f_N); | ||
| float cosPhi = dot(f_H, f_N); | ||
|
|
||
| vec4 f_global_ambient = vec4(0.7, 0.7, 0.7, 1.0); | ||
| vec3 f_ambient = ((f_global_ambient * f_mat.ambient) + (f_light.ambient * f_mat.ambient)).xyz; | ||
| vec3 f_ambient = ((f_ambient * f_mat.ambient) + (f_light.ambient * f_mat.ambient)).xyz; |
There was a problem hiding this comment.
Avoid Shadowing the Parameter f_ambient
The local variable f_ambient declared inside the function shadows the parameter f_ambient. This can lead to confusion and potential bugs. Consider renaming the local variable to avoid shadowing.
Apply this diff to fix the issue:
- vec3 f_ambient = ((f_ambient * f_mat.ambient) + (f_light.ambient * f_mat.ambient)).xyz;
+ vec3 ambient_light = ((f_ambient * f_mat.ambient) + (f_light.ambient * f_mat.ambient)).xyz;Update subsequent references to f_ambient within the function to ambient_light.
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.
| vec3 f_ambient = ((f_ambient * f_mat.ambient) + (f_light.ambient * f_mat.ambient)).xyz; | |
| vec3 ambient_light = ((f_ambient * f_mat.ambient) + (f_light.ambient * f_mat.ambient)).xyz; |
There was a problem hiding this comment.
@coderabbitai fixed in b532392
There was a problem hiding this comment.
@btipling, glad to see the issue has been fixed!
(^.^/)
| pub fn init(allocator: std.mem.Allocator, ctx: scenes.SceneContext) *Dolphin { | ||
| const pd = allocator.create(Dolphin) catch @panic("OOM"); | ||
| errdefer allocator.destroy(pd); | ||
| const integrator = physics.Integrator(physics.SmoothDeceleration).init(.{}); | ||
| const cam = physics.camera.Camera(*Dolphin, physics.Integrator(physics.SmoothDeceleration)).init( | ||
| allocator, | ||
| ctx.cfg, | ||
| pd, | ||
| integrator, | ||
| .{ 0, 2, -3 }, | ||
| std.math.pi - std.math.pi / 4.0, | ||
| ); | ||
| errdefer cam.deinit(allocator); | ||
|
|
||
| const mats = [_]lighting.Material{ | ||
| .{ | ||
| .ambient = [4]f32{ 0.2, 0.2, 0.2, 1.0 }, | ||
| .diffuse = [4]f32{ 0.8, 0.8, 0.8, 1.0 }, | ||
| .specular = [4]f32{ 0.5, 0.5, 0.5, 1.0 }, | ||
| .shininess = 32.0, | ||
| }, | ||
| }; | ||
|
|
||
| const bd: rhi.Buffer.buffer_data = .{ .materials = mats[0..] }; | ||
| var mats_buf = rhi.Buffer.init(bd); | ||
| errdefer mats_buf.deinit(); | ||
|
|
||
| const lights = [_]lighting.Light{ | ||
| .{ | ||
| .ambient = [4]f32{ 0.1, 0.1, 0.1, 1.0 }, | ||
| .diffuse = [4]f32{ 1.0, 1.0, 1.0, 1.0 }, | ||
| .specular = [4]f32{ 1.0, 1.0, 1.0, 1.0 }, | ||
| .location = [4]f32{ 0.0, 0.0, 0.0, 1.0 }, | ||
| .direction = [4]f32{ 0.75, -0.5, -0.5, 0.0 }, | ||
| .cutoff = 0.0, | ||
| .exponent = 0.0, | ||
| .attenuation_constant = 1.0, | ||
| .attenuation_linear = 0.0, | ||
| .attenuation_quadratic = 0.0, | ||
| .light_kind = .direction, | ||
| }, | ||
| }; | ||
| const ld: rhi.Buffer.buffer_data = .{ .lights = lights[0..] }; | ||
| var lights_buf = rhi.Buffer.init(ld); | ||
| errdefer lights_buf.deinit(); | ||
|
|
||
| pd.* = .{ | ||
| .allocator = allocator, | ||
| .view_camera = cam, | ||
| .dolphin_texture = null, | ||
| .ctx = ctx, | ||
| .materials = mats_buf, | ||
| .lights = lights_buf, | ||
| }; | ||
| pd.renderDolphin(); | ||
| return pd; | ||
| } |
There was a problem hiding this comment.
Review error handling and use of errdefer in init function
The init function employs errdefer statements for resource cleanup, but it does not return an error type. If an error occurs (e.g., allocator.create(Dolphin) fails and panics), the errdefer statements won't execute because panicking bypasses them. This can lead to resource leaks.
Consider refactoring the function to return an error type and handle errors appropriately. This ensures that errdefer statements execute for proper resource cleanup.
Apply this diff to adjust the function signature and error handling:
-pub fn init(allocator: std.mem.Allocator, ctx: scenes.SceneContext) *Dolphin {
+pub fn init(allocator: std.mem.Allocator, ctx: scenes.SceneContext) !*Dolphin {
const pd = allocator.create(Dolphin) catch return error.OutOfMemory;
errdefer allocator.destroy(pd);
// ... additional error handling for other operations ...
return pd;
}Ensure that all fallible operations within init are properly handled, and propagate errors using !.
Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores