Skip to content

Address review feedback - #24

Merged
mate-h merged 2 commits into
m/solari-metalfrom
cursor/jms55-feedback-eda4
Jul 27, 2026
Merged

Address review feedback#24
mate-h merged 2 commits into
m/solari-metalfrom
cursor/jms55-feedback-eda4

Conversation

@mate-h

@mate-h mate-h commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Address feedback

@mate-h mate-h changed the title Address JMS55 review feedback Address review feedback Jul 27, 2026
Comment on lines +47 to +52
/// Bytes contributed by each world-cache cell in the packed SoA buffer, excluding the fixed-size
/// `b` array that sits between `a` and `active_cell_indices`.
///
/// The `ShaderType` mirror lives in this module and is intentionally private so it cannot be
/// constructed, because it would be too large.
mod world_cache_layout {
use bevy_math::{Vec3, Vec4};
use bevy_render::render_resource::ShaderType;
/// Keep in sync with `WorldCache` in `realtime_bindings.wgsl`:
/// `checksums` (4) + `life` (4) + `radiance` (16) + `geometry_data` (32) + `luminance_deltas` (4)
/// + `active_cells_new_radiance` (16; `vec3` array stride) + `a` (4) + `active_cell_indices` (4).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

These code comments are way too verbose, will simplify to just say it's the sum of the fields sizes.

Comment on lines +55 to +58
/// Size of the fixed `b` array (`array<u32, WORLD_CACHE_SIZE / 1024>`) in the packed buffer.
const WORLD_CACHE_B_SIZE: u64 = (WORLD_CACHE_SIZE / 1024) * size_of::<u32>() as u64;

/// Offset of `active_cells_count` in the packed world cache buffer.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

No need to reiterate this indexes the packed buffer, self explanatory can simplify too.

@mate-h
mate-h marked this pull request as ready for review July 27, 2026 08:14
@mate-h
mate-h merged commit edef23a into m/solari-metal Jul 27, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant