Skip to content

Build data to rayon#79

Open
doubleailes wants to merge 3 commits into
mainfrom
build_data_to_rayon
Open

Build data to rayon#79
doubleailes wants to merge 3 commits into
mainfrom
build_data_to_rayon

Conversation

@doubleailes
Copy link
Copy Markdown
Owner

@doubleailes doubleailes commented Apr 21, 2025

PR Type

Enhancement


Description

  • Refactored rendering logic for improved parallelism and efficiency

    • Collects rays per sample, processes in parallel, and accumulates color per pixel
    • Progress bar now tracks per-ray progress instead of per-row
  • Added set_mut_pixel method to buffer for color accumulation

  • Simplified and updated sample scene RON file structure

    • Flatter camera/object/material definitions and new frame setting
  • Minor code formatting and clarity improvements


Changes walkthrough 📝

Relevant files
Enhancement
buffer.rs
Add pixel color accumulation method to buffer                       

crates/crust-render/src/buffer.rs

  • Added set_mut_pixel method for in-place pixel color accumulation
  • Documented new method for clarity
  • +15/-0   
    tracer.rs
    Refactor rendering for parallelism and color accumulation

    crates/crust-render/src/tracer.rs

  • Refactored rendering loop to collect all rays and process in parallel
  • Progress bar now tracks per-ray instead of per-row
  • Uses new set_mut_pixel for accumulating color per pixel
  • Improved readability and efficiency of ray color calculation
  • +32/-28 
    teapot.ron
    Simplify and update sample scene RON structure                     

    samples/teapot.ron

  • Simplified RON structure for camera, objects, and materials
  • Added smooth: true to teapot object
  • Added new frame setting to settings
  • Flattened vector/matrix definitions for clarity
  • +18/-48 
    Formatting
    instance.rs
    Code formatting for normal transformation                               

    crates/crust-render/src/instance.rs

  • Reformatted code for clarity in normal transformation logic
  • No functional changes, only formatting
  • +5/-1     
    main.rs
    Minor formatting in render mode selection                               

    crates/crust-render/src/main.rs

    • Minor formatting: consistent spacing in conditional
    +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Memory Usage

    The refactored rendering code collects all rays upfront in a vector before processing, which could lead to high memory usage for large scenes or high sample counts. Consider if a chunked approach might be more memory-efficient.

    let mut d: Vec<(usize, usize, Ray)> = Vec::with_capacity(pixels_count);
    for j in (0..self.settings.height).rev() {
        for i in 0..self.settings.width {
            for sample in 0..self.settings.samples_per_pixel {
                let (dx, dy) = if (sample as usize) < cmj_samples.len() {
                    cmj_samples[sample as usize]
                } else {
                    (utils::random(), utils::random())
                };
    
                let u = (i as f32 + dx) / (self.settings.width - 1) as f32;
                let v = (j as f32 + dy) / (self.settings.height - 1) as f32;
    
                let ray = self.camera.get_ray(u, v);
                d.push((i, j, ray));
            }
        }
    }
    Progress Bar

    The progress bar is declared but not initialized with a value. The code references 'bar' on line 40 but the initialization is missing, which could cause a compilation error.

    let bar = ProgressBar::new(pixels_count as u64);
    bar.set_style(

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Reduce memory usage

    The memory usage is excessive because all rays are stored in memory before
    processing. For high-resolution renders with many samples, this could lead to
    out-of-memory errors. Consider processing rays in batches or chunks to maintain
    parallelism while reducing memory overhead.

    crates/crust-render/src/tracer.rs [47-64]

    -let mut d: Vec<(usize, usize, Ray)> = Vec::with_capacity(pixels_count);
    +let chunk_size = 1024; // Process rays in manageable chunks
    +let mut buffer = Buffer::new(self.settings.width, self.settings.height);
    +
     for j in (0..self.settings.height).rev() {
    -    for i in 0..self.settings.width {
    -        for sample in 0..self.settings.samples_per_pixel {
    -            let (dx, dy) = if (sample as usize) < cmj_samples.len() {
    -                cmj_samples[sample as usize]
    -            } else {
    -                (utils::random(), utils::random())
    -            };
    -
    -            let u = (i as f32 + dx) / (self.settings.width - 1) as f32;
    -            let v = (j as f32 + dy) / (self.settings.height - 1) as f32;
    -
    -            let ray = self.camera.get_ray(u, v);
    -            d.push((i, j, ray));
    +    for i_chunk in (0..self.settings.width).step_by(chunk_size) {
    +        let mut rays = Vec::with_capacity(chunk_size * self.settings.samples_per_pixel as usize);
    +        
    +        let i_end = (i_chunk + chunk_size).min(self.settings.width);
    +        for i in i_chunk..i_end {
    +            for sample in 0..self.settings.samples_per_pixel {
    +                let (dx, dy) = if (sample as usize) < cmj_samples.len() {
    +                    cmj_samples[sample as usize]
    +                } else {
    +                    (utils::random(), utils::random())
    +                };
    +                
    +                let u = (i as f32 + dx) / (self.settings.width - 1) as f32;
    +                let v = (j as f32 + dy) / (self.settings.height - 1) as f32;
    +                
    +                let ray = self.camera.get_ray(u, v);
    +                rays.push((i, j, ray));
    +            }
    +        }
    +        
    +        // Process this chunk in parallel
    +        let colors: Vec<(usize, usize, Vec3A)> = rays
    +            .into_par_iter()
    +            .map(|(i, j, r)| {
    +                let col = ray_color(
    +                    &r,
    +                    &self.world,
    +                    &self.lights,
    +                    self.settings.max_depth as i32,
    +                );
    +                bar.inc(1);
    +                (i, j, col)
    +            })
    +            .collect();
    +            
    +        // Accumulate colors for this chunk
    +        for (i, j, col) in colors {
    +            buffer.set_mut_pixel(i, j, col / self.settings.samples_per_pixel as f32);
             }
         }
     }
     
    +bar.finish();
    +return buffer;
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: This suggestion addresses a significant memory optimization issue. Processing all rays at once could lead to excessive memory usage for high-resolution renders. The chunked approach maintains parallelism while significantly reducing peak memory consumption, which could prevent out-of-memory errors in production.

    Medium
    • More

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    1 participant