Skip to content

load resources and components from ECS#12

Merged
gariasf merged 22 commits into
mainfrom
feature/ecs
May 2, 2026
Merged

load resources and components from ECS#12
gariasf merged 22 commits into
mainfrom
feature/ecs

Conversation

@gariasf

@gariasf gariasf commented May 1, 2026

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • New Features

    • Instance-based batch rendering for improved performance
    • Runtime helmet spawning (G) and despawning (H)
    • On-frame preparation of draw lists to reduce per-entity GPU work
  • Improvements

    • Increased default window resolution to 2560×1440
    • Refactored input and camera to ECS-driven systems for smoother control
    • More detailed debug/tracing logs across asset loading and rendering
  • Chores

    • Project config and guidelines updated for consistent logging and reviews

@coderabbitai

coderabbitai Bot commented May 1, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2fc9c4a8-d954-402f-9e4c-435483da88d7

📥 Commits

Reviewing files that changed from the base of the PR and between 56b1e15 and 61111d0.

📒 Files selected for processing (6)
  • .coderabbit.yaml
  • src/assets/mesh.rs
  • src/assets/mod.rs
  • src/assets/pools.rs
  • src/state.rs
  • src/time/mod.rs
✅ Files skipped from review due to trivial changes (4)
  • src/assets/mod.rs
  • src/assets/mesh.rs
  • .coderabbit.yaml
  • src/state.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/time/mod.rs
  • src/assets/pools.rs

Walkthrough

This PR migrates the renderer to a bevy_ecs sub-crate model: introduces ECS components/resources/systems, a MeshPool and mesh handles, prepares per-frame instance data from queries, replaces per-entity uniforms with a single instance storage buffer and instanced shader, and updates scheduling, input, camera and asset wiring accordingly.

Changes

Phase 5: ECS Rendering Refactor

Layer / File(s) Summary
Dependencies & Core Types
Cargo.toml, src/components.rs
Adds bevy_ecs = "0.18.1" with trace. Adds #[derive(Component)] Transform(pub glam::Affine3A) and MeshRef(pub MeshHandle).
Asset Types
src/assets/mesh.rs
Adds Primitive (vertex/index buffers, num_indices, texture_bind_group), Mesh { primitives: Vec<Primitive> }, and MeshHandle(pub u32).
Asset Pooling & API
src/assets/pools.rs, src/assets/mod.rs, src/assets/loader.rs
Adds MeshPool Resource with insert/get; assets::mod re-exports loader/mesh/pools; loader.rs now imports Vertex from crate::render, removes local ModelUniform, and adds tracing spans/logs.
ECS Resources & Input/Time
src/input/mod.rs, src/time/mod.rs
Adds Input Resource (pressed/just_pressed) and clear_just_pressed system. Time becomes Resource with last_frame: Option<Instant> and zero-initial delta until first tick.
Camera & Movement Systems
src/camera/mod.rs
Replaces prior camera uniform with #[derive(Resource)] Camera { position, yaw, pitch, speed, sensitivity } and adds fly_camera system updating position from Input and Time.
Helmet Runtime Systems
src/helmet/mod.rs
Adds HelmetAssets and RuntimeHelmets Resources plus spawn_helmet/despawn_helmet systems that use Commands, Input and Camera to spawn/despawn entities with Transform+MeshRef.
Instance & Uniform GPU Types
src/render/instance.rs, src/render/uniforms.rs, src/render/mod.rs
Adds InstanceData (model + normal_matrix) and INSTANCE_BUFFER_INITIAL_CAPACITY; adds CameraUniform { view_proj, position }; declares prepare submodule and re-exports prepare_renderables.
Prepare System
src/render/prepare.rs
Adds prepare_renderables(world: &mut World) -> Vec<InstanceData>: queries (&Transform,&MeshRef), computes model and normal matrices, returns contiguous instance list.
Shader Instancing
src/shader.wgsl
Removes per-entity ModelUniform; adds InstanceData and storage buffer instances: array<InstanceData> at @group(3) @binding(0) indexed by @builtin(instance_index); introduces VertexInput and per-instance transforms; CameraUniform member renamed position.
State & Render Integration
src/state.rs
Reworks State to hold pub world: World, instance storage buffer, instance bind group and instance_capacity, and a Schedule. new inserts resources, uploads primitives into MeshPool, spawns entities with Transform+MeshRef. update runs schedule, builds CameraUniform, calls prepare_renderables, ensures GPU capacity, uploads instance buffer. render binds instance storage at group 3, iterates query (&Transform,&MeshRef) and issues draws using instance indexing; adds ensure_instance_capacity logic.
Main & I/O Wiring
src/main.rs
Restructures modules (adds assets, components, render, helmet; removes legacy modules), creates 2560x1440 window, and updates input/mouse handlers to mutate Input and Camera Resources. Adjusts tracing init to use with_env_filter.
Docs & Config
PLAN.md, ROADMAP.md, CLAUDE.md, .cargo/config.toml, .coderabbit.yaml
PLAN.md/ROADMAP.md rewritten to prescribe bevy_ecs sub-crate, handle pools, prepare-upload instance buffer approach and schedule-based systems. CLAUDE.md expanded with conventions and GPU/ECS rules. .cargo/config.toml sets default RUST_LOG. .coderabbit.yaml adds per-path review instructions and automation config.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive Title is vague and generic—'load resources and components from ECS' lacks specificity about which resources/components or the extent of the integration. It describes a high-level theme rather than the primary changeset (which restructures the codebase into a feature-based architecture, replaces custom state with Bevy ECS scheduling, and migrates rendering to instance-buffer-driven entity queries). Clarify the title to capture the main scope: e.g. 'Integrate Bevy ECS with instance-buffer rendering' or 'Refactor state and rendering into ECS-driven pipeline' to convey the architectural shift beyond just 'loading' resources/components.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/ecs

Review rate limit: 9/10 reviews remaining, refill in 6 minutes.

Comment @coderabbitai help to get the list of available commands and usage tips.

@gariasf

gariasf commented May 1, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 1, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@PLAN.md`:
- Around line 216-220: The sentence uses the abbreviation "sim" ambiguously;
update the Pull-renderer note (the paragraph mentioning
world.query::<(&Transform, &MeshRef, &MaterialRef)>) to replace "sim" with an
explicit phrase like "simulation code" or "game logic" and slightly expand the
wording so it reads: "Pull keeps the simulation code (game logic) ignorant of
wgpu — Design Principle `#3`," ensuring the intent is clear to readers unfamiliar
with the shorthand.

In `@src/resources/pools.rs`:
- Around line 19-20: The current MeshPools::get method uses unchecked indexing
(&self.0[handle.0 as usize]) which will panic on invalid MeshHandle; change the
signature of get to return Option<&Mesh> (or Result<&Mesh, _>) and use
self.0.get(handle.0 as usize) to avoid panics, and then update call sites that
use MeshHandle (e.g., where mesh_pool.get(...) is called in the render/draw
path) to handle None by skipping the draw and logging a single warning about the
invalid handle.

In `@src/state.rs`:
- Around line 355-372: The instance buffer is allocated with a fixed
INSTANCE_BUFFER_INITIAL_CAPACITY but prepare_renderables() writes all active
instances without checking capacity; modify prepare_renderables() to guard
against overflow by checking the active instance count against
State.instance_capacity and, if exceeded, allocate a new larger buffer (e.g.,
double the needed size or at least enough for count), update
State.instance_buffer and State.instance_capacity, and recreate the
instance_bind_group using the existing storage_buffer_layout before calling
queue.write_buffer(); alternatively clamp uploads to instance_capacity if you
intentionally limit renderables, but preferred fix is to grow the buffer and
update the bind group so queue.write_buffer() never writes past the buffer
bounds.

In `@src/systems/camera.rs`:
- Line 1: camera_system is currently a no-op; implement it to perform the ECS
camera update: query entities with Camera and Transform (and optional
FollowTarget or CameraController) components, read the Time resource for delta
time, compute the new camera Transform (position/rotation/zoom) by following the
target or applying controller input, apply smoothing/interpolation if a
smoothing parameter exists, and write the updated Transform back to the entity
(marking it changed). Use the Camera, Transform, FollowTarget/CameraController
component names and the Time resource within camera_system so the camera path
produces visible behavior when scheduled.

In `@src/systems/prepare.rs`:
- Around line 7-20: prepare_renderables currently assumes the shared
instance_buffer (allocated using INSTANCE_BUFFER_INITIAL_CAPACITY) is large
enough for all renderables; add a resize/rebind path: in prepare_renderables
compute required_bytes = instance_count * size_of::<InstanceData>() and compare
to the current buffer size (accessible via the buffer's tracked capacity in
state or by passing a mutable capacity/handle), and if required_bytes >
current_capacity, create a new wgpu::Buffer with the larger size (e.g., grow by
max(required, current*2) or at least to required_bytes), replace the
instance_buffer handle/state (and update the stored capacity), then write the
instance slice to the (new) buffer via queue.write_buffer; update any places
that store the buffer handle/capacity so future calls know the new size.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c6c5d859-c918-478e-8630-33b61a2b507f

📥 Commits

Reviewing files that changed from the base of the PR and between 1425e2b and 5b3858e.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (25)
  • Cargo.toml
  • PLAN.md
  • ROADMAP.md
  • src/assets/loader.rs
  • src/assets/material.rs
  • src/assets/mesh.rs
  • src/assets/mod.rs
  • src/assets/texture.rs
  • src/components.rs
  • src/main.rs
  • src/render/depth.rs
  • src/render/instance.rs
  • src/render/mod.rs
  • src/render/uniforms.rs
  • src/render/vertex.rs
  • src/resources/camera.rs
  • src/resources/input.rs
  • src/resources/mod.rs
  • src/resources/pools.rs
  • src/resources/time.rs
  • src/shader.wgsl
  • src/state.rs
  • src/systems/camera.rs
  • src/systems/mod.rs
  • src/systems/prepare.rs

Comment thread PLAN.md Outdated
Comment thread src/assets/pools.rs Outdated
Comment thread src/state.rs
Comment thread src/systems/camera.rs Outdated
Comment thread src/systems/prepare.rs Outdated
@gariasf gariasf marked this pull request as draft May 1, 2026 18:48

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/resources/time.rs`:
- Around line 11-15: The constructor Time::new currently sets last_frame =
Instant::now(), causing the first Time::tick() to measure startup delay and
produce an inflated delta_time; change the initialization so the first-frame
baseline is set in Time::tick() instead: make last_frame optional (e.g.,
Option<Instant>) or use a boolean sentinel, initialize last_frame to None in
Time::new(), and update Time::tick() to set last_frame to Instant::now() on the
first call without computing delta_time, otherwise compute delta_time as now -
last_frame and update last_frame; update all references to
Time::last_frame/Time::tick accordingly (fields: delta_time, last_frame;
methods: Time::new, Time::tick).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9177e2bd-2b2e-466a-b404-855c9a0e3c5a

📥 Commits

Reviewing files that changed from the base of the PR and between 5b3858e and 8fa1cf7.

📒 Files selected for processing (4)
  • src/main.rs
  • src/resources/mod.rs
  • src/resources/time.rs
  • src/state.rs
✅ Files skipped from review due to trivial changes (1)
  • src/resources/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main.rs
  • src/state.rs

Comment thread src/time/mod.rs
@gariasf gariasf marked this pull request as ready for review May 2, 2026 16:50

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/assets/pools.rs`:
- Around line 8-12: MaterialPool and TexturePool are declared as tuple structs
but their inner Vec fields are never read, triggering dead_code; add actual
read/write paths or postpone registering them as resources. Fix by adding simple
public APIs (e.g., impl MaterialPool { pub fn insert(&mut self, m: Material);
pub fn get(&self, idx: usize) -> Option<&Material> } and analogous methods for
TexturePool) and then call those methods from your initialization/asset-loading
system (where you register materials/textures) so the fields are actually used;
alternatively remove the Resource/Default derives and stop registering
MaterialPool/TexturePool until they are needed. Ensure references to the symbols
MaterialPool and TexturePool are updated accordingly.

In `@src/assets/texture.rs`:
- Around line 1-4: The Texture struct's fields (Texture.view and
Texture.gpu_handle) are unused and trigger Clippy errors; to unblock CI either
remove the struct until it's actually used or temporarily suppress the warnings
by adding #[allow(dead_code)] above the Texture declaration, or wire the type
into the render/material flow so those fields are read (e.g., reference
Texture.view and/or Texture.gpu_handle from the renderer/material code paths).
Update the declaration for Texture or the render code accordingly so Clippy no
longer reports unused fields.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 91bc7429-f85b-4d6e-b2e1-2ef3859530f3

📥 Commits

Reviewing files that changed from the base of the PR and between 8fa1cf7 and 28d9d73.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (20)
  • .cargo/config.toml
  • .coderabbit.yaml
  • CLAUDE.md
  • Cargo.toml
  • PLAN.md
  • ROADMAP.md
  • src/assets/loader.rs
  • src/assets/mod.rs
  • src/assets/pools.rs
  • src/assets/texture.rs
  • src/camera/mod.rs
  • src/helmet/mod.rs
  • src/input/mod.rs
  • src/main.rs
  • src/render/mod.rs
  • src/render/prepare.rs
  • src/render/uniforms.rs
  • src/shader.wgsl
  • src/state.rs
  • src/time/mod.rs
✅ Files skipped from review due to trivial changes (4)
  • .cargo/config.toml
  • .coderabbit.yaml
  • ROADMAP.md
  • src/assets/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/render/mod.rs
  • src/render/uniforms.rs
  • src/assets/loader.rs
  • Cargo.toml

Comment thread src/assets/pools.rs Outdated
Comment thread src/assets/texture.rs Outdated
gariasf and others added 4 commits May 2, 2026 19:01
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: Guillem Arias Fauste <gariasf@proton.me>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: Guillem Arias Fauste <gariasf@proton.me>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@PLAN.md`:
- Line 11: The code references a MaterialHandle/MaterialRef component that
doesn't exist in the ECS; only Transform and MeshRef are defined in
src/components.rs. Add a MaterialRef (or MaterialHandle) component type to
src/components.rs and update any systems and entity construction that expect it
(e.g., where Transform and MeshRef are assigned) so entities receive the new
MaterialRef component; ensure the component derives the same traits as MeshRef
(Send + Sync + Component/reflect as used) and update related systems to read
MaterialRef instead of a non-existent MaterialHandle symbol.
- Line 315: The PLAN's Step 2 shows a query including MaterialRef
(`world.query::<(&Transform, &MeshRef, &MaterialRef)>()`) but the actual
implementation in src/render/prepare.rs (function prepare_renderables) only
queries (&Transform, &MeshRef); update the documentation or the implementation
to match: either change the PLAN example to `world.query::<(&Transform,
&MeshRef)>()` to reflect prepare_renderables, or modify prepare_renderables to
also include MaterialRef in its query and downstream logic to handle
MaterialRef; reference prepare_renderables and the PLAN query snippet when
making the change.
- Line 38: The architecture table lists components that are not implemented;
update the table to reflect the actual code (currently only Transform and
MeshRef have `#[derive(Component)]`) or implement the missing components
`MaterialRef` and `LightRef` in the codebase; ensure the entry for `Components`
matches the real types and derives (e.g., `Transform`, `MeshRef`) and that the
authoring shape description (glam::Affine3A and handles) accurately describes
the implemented components and not phantom types.
- Line 309: The spawn_helmet implementation currently only attaches Transform
and MeshRef but the plan/example expects MaterialRef too; update the
spawn_helmet function to include MaterialRef when creating the entity (either by
adding MaterialRef to the spawn bundle or calling .insert(MaterialRef(...)) on
the Commands spawn), and adjust the function signature to accept or obtain the
required material handle so the spawned entity carries Transform + MeshRef +
MaterialRef as in the plan.
- Around line 271-273: The plan claims MaterialRef and LightRef exist but only
Transform and MeshRef are defined; add missing component definitions to
src/components.rs mirroring the style of Transform and MeshRef: declare
MaterialRef and LightRef types (e.g., simple newtype structs or small structs
holding a handle/id) and derive the same traits used for other components (e.g.,
#[derive(Component, Clone, Copy, Debug, Default)] or whatever pattern Transform
and MeshRef use) so they integrate with bevy_ecs; update any exports if the
module re-exports Transform/MeshRef so MaterialRef and LightRef are exported as
well, then mark the step complete.
- Line 367: The Step 3 example in PLAN.md shows world.query::<(&Transform,
&MeshRef, &MaterialRef)>() but the implementation in src/render/prepare.rs
(prepare_renderables) only queries (&Transform, &MeshRef); update the PLAN.md
example to match the code by removing &MaterialRef (or explicitly note that
MaterialRef is omitted in prepare_renderables), ensuring the documentation uses
world.query::<(&Transform, &MeshRef)>() and references prepare_renderables,
Transform, MeshRef, and MaterialRef consistently.
- Around line 374-375: The PLAN.md line incorrectly claims prepare_renderables
produces a (mesh, material, instance_idx) draw list; in reality
prepare_renderables (see prepare_renderables in src/render/prepare.rs) returns a
Vec<InstanceData> containing model/normal matrices only. Update the sentence to
accurately describe its output (e.g., "prepare_renderables returns
Vec<InstanceData> with model and normal matrices") and remove or relocate the
claim about the draw list, or point to the actual function/module that
constructs the (mesh, material, instance_idx) draw list if one exists.
- Line 217: The documentation example shows world.query::<(&Transform, &MeshRef,
&MaterialRef)>, but the implementation/query in src/render/prepare.rs only uses
world.query::<(&Transform, &MeshRef)> and there is no MaterialRef component;
either remove MaterialRef from the doc example to match the implementation or
add a MaterialRef component and update the query in prepare.rs to
world.query::<(&Transform, &MeshRef, &MaterialRef)>; locate the query call in
prepare.rs (the function doing the render preparation that calls world.query)
and make the example and code consistent by choosing one of these two fixes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 351701ef-929c-4723-8a86-9fda5cacbe75

📥 Commits

Reviewing files that changed from the base of the PR and between 28d9d73 and 56b1e15.

📒 Files selected for processing (2)
  • PLAN.md
  • src/assets/pools.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/assets/pools.rs

Comment thread PLAN.md
`MeshHandle` + `MaterialHandle` now come from an ECS `World` query instead of
hardcoded `Vec`s on `State`.
Replace ad-hoc scene data with `bevy_ecs`. Goal: the three helmets are still
on screen at the end, but their `Transform` + `MeshHandle` + `MaterialHandle`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

MaterialHandle not implemented in ECS.

Code has only Transform and MeshRef components. No MaterialRef or MaterialHandle component exists (see src/components.rs).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@PLAN.md` at line 11, The code references a MaterialHandle/MaterialRef
component that doesn't exist in the ECS; only Transform and MeshRef are defined
in src/components.rs. Add a MaterialRef (or MaterialHandle) component type to
src/components.rs and update any systems and entity construction that expect it
(e.g., where Transform and MeshRef are assigned) so entities receive the new
MaterialRef component; ensure the component derives the same traits as MeshRef
(Send + Sync + Component/reflect as used) and update related systems to read
MaterialRef instead of a non-existent MaterialHandle symbol.

Comment thread PLAN.md
| Decision | Choice | Why |
|---|---|---|
| ECS crate | `bevy_ecs` (current 0.x), sub-crate only | Just World/Component/Query/Schedule/Commands; no full Bevy app/render |
| Components | `#[derive(Component)]` on `Transform`, `MeshRef`, `MaterialRef`, `LightRef` | Authoring shape: `glam::Affine3A` and handles, never GPU bytes |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Architecture table lists unimplemented components.

Actual code defines only Transform and MeshRef. MaterialRef and LightRef components don't exist.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@PLAN.md` at line 38, The architecture table lists components that are not
implemented; update the table to reflect the actual code (currently only
Transform and MeshRef have `#[derive(Component)]`) or implement the missing
components `MaterialRef` and `LightRef` in the codebase; ensure the entry for
`Components` matches the real types and derives (e.g., `Transform`, `MeshRef`)
and that the authoring shape description (glam::Affine3A and handles) accurately
describes the implemented components and not phantom types.

Comment thread PLAN.md
planning but aren't built yet.

- **Pull-renderer.** Each frame, the renderer queries the world for
what to draw (`world.query::<(&Transform, &MeshRef, &MaterialRef)>`)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Query example includes MaterialRef, but implementation doesn't.

src/render/prepare.rs queries (&Transform, &MeshRef) only. MaterialRef component doesn't exist.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@PLAN.md` at line 217, The documentation example shows
world.query::<(&Transform, &MeshRef, &MaterialRef)>, but the
implementation/query in src/render/prepare.rs only uses
world.query::<(&Transform, &MeshRef)> and there is no MaterialRef component;
either remove MaterialRef from the doc example to match the implementation or
add a MaterialRef component and update the query in prepare.rs to
world.query::<(&Transform, &MeshRef, &MaterialRef)>; locate the query call in
prepare.rs (the function doing the render preparation that calls world.query)
and make the example and code consistent by choosing one of these two fixes.

Comment thread PLAN.md
Comment on lines +271 to +273
_Pin a current `bevy_ecs` 0.x version. Define `Transform`, `MeshRef`,
`MaterialRef`, `LightRef` components. Define `MeshPool`, `TexturePool`,
`MaterialPool` as resources._

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Step 1 claims to define MaterialRef and LightRef, but they don't exist.

Step marked [x] Done but src/components.rs defines only Transform and MeshRef. No MaterialRef or LightRef components in codebase.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@PLAN.md` around lines 271 - 273, The plan claims MaterialRef and LightRef
exist but only Transform and MeshRef are defined; add missing component
definitions to src/components.rs mirroring the style of Transform and MeshRef:
declare MaterialRef and LightRef types (e.g., simple newtype structs or small
structs holding a handle/id) and derive the same traits used for other
components (e.g., #[derive(Component, Clone, Copy, Debug, Default)] or whatever
pattern Transform and MeshRef use) so they integrate with bevy_ecs; update any
exports if the module re-exports Transform/MeshRef so MaterialRef and LightRef
are exported as well, then mark the step complete.

Comment thread PLAN.md
### Step 2: Migrate 3 helmets into ECS

_Replace `State.transforms` and `State.primitives` with ECS entities
carrying `Transform` + `MeshRef` + `MaterialRef`. Old render path stays —

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Step 2 spawn example includes MaterialRef, but implementation doesn't.

src/helmet/mod.rs::spawn_helmet spawns (Transform, MeshRef) only. No MaterialRef in actual entity spawns.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@PLAN.md` at line 309, The spawn_helmet implementation currently only attaches
Transform and MeshRef but the plan/example expects MaterialRef too; update the
spawn_helmet function to include MaterialRef when creating the entity (either by
adding MaterialRef to the spawn bundle or calling .insert(MaterialRef(...)) on
the Commands spawn), and adjust the function signature to accept or obtain the
required material handle so the spawned entity carries Transform + MeshRef +
MaterialRef as in the plan.

Comment thread PLAN.md

**Notes.** Spawn at startup via `cmd.spawn((Transform(...), MeshRef(h),
MaterialRef(m)))`. The render path can iterate
`world.query::<(&Transform, &MeshRef, &MaterialRef)>()` and look meshes /

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Step 2 query example includes MaterialRef, not in implementation.

prepare_renderables queries (&Transform, &MeshRef) only (see src/render/prepare.rs).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@PLAN.md` at line 315, The PLAN's Step 2 shows a query including MaterialRef
(`world.query::<(&Transform, &MeshRef, &MaterialRef)>()`) but the actual
implementation in src/render/prepare.rs (function prepare_renderables) only
queries (&Transform, &MeshRef); update the documentation or the implementation
to match: either change the PLAN example to `world.query::<(&Transform,
&MeshRef)>()` to reflect prepare_renderables, or modify prepare_renderables to
also include MaterialRef in its query and downstream logic to handle
MaterialRef; reference prepare_renderables and the PLAN query snippet when
making the change.

Comment thread PLAN.md
- A `prepare_renderables` pass each frame walks
`world.query::<(Transform, MeshHandle, MaterialHandle)>()`, builds a
- A `prepare_renderables` system each frame walks
`world.query::<(&Transform, &MeshRef, &MaterialRef)>()`, builds a

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Step 3 query example includes MaterialRef, not in implementation.

src/render/prepare.rs::prepare_renderables queries (&Transform, &MeshRef) only.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@PLAN.md` at line 367, The Step 3 example in PLAN.md shows
world.query::<(&Transform, &MeshRef, &MaterialRef)>() but the implementation in
src/render/prepare.rs (prepare_renderables) only queries (&Transform, &MeshRef);
update the PLAN.md example to match the code by removing &MaterialRef (or
explicitly note that MaterialRef is omitted in prepare_renderables), ensuring
the documentation uses world.query::<(&Transform, &MeshRef)>() and references
prepare_renderables, Transform, MeshRef, and MaterialRef consistently.

Comment thread PLAN.md
Comment on lines +374 to +375
- `prepare_renderables` also produces the `(mesh, material,
instance_idx)` draw list the renderer iterates.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

prepare_renderables doesn't produce material in draw list.

Implementation returns Vec<InstanceData> with model/normal matrices only (see src/render/prepare.rs). No (mesh, material, instance_idx) tuple list exists.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@PLAN.md` around lines 374 - 375, The PLAN.md line incorrectly claims
prepare_renderables produces a (mesh, material, instance_idx) draw list; in
reality prepare_renderables (see prepare_renderables in src/render/prepare.rs)
returns a Vec<InstanceData> containing model/normal matrices only. Update the
sentence to accurately describe its output (e.g., "prepare_renderables returns
Vec<InstanceData> with model and normal matrices") and remove or relocate the
claim about the draw list, or point to the actual function/module that
constructs the (mesh, material, instance_idx) draw list if one exists.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/assets/pools.rs`:
- Around line 16-20: The extra closing brace after the implementation of
Pools::get is causing a syntax error; open the impl/block containing pub fn
get(&self, handle: MeshHandle) -> Option<&Mesh> and remove the stray `}` so the
braces balance (ensure the closing brace only ends the impl/struct scope and not
an extra block), leaving the function get, MeshHandle and Mesh definitions
correctly enclosed.

In `@src/state.rs`:
- Around line 594-612: The code assumes mesh_pool.get(mesh_ref.0) returns &Mesh
but it returns Option<&Mesh>, so update the loop in the render pass to handle
None: call mesh_pool.get(mesh_ref.0) and use pattern matching (e.g., if let
Some(mesh) = ... { ... } or continue on None) before accessing mesh.primitives
and primitive.texture_bind_group / vertex_buffer / index_buffer; ensure the body
that computes instance_idx and calls render_pass.draw_indexed stays inside that
Some branch so invalid handles are skipped safely.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b598843a-4567-42e0-b2d8-e05fed7b99ff

📥 Commits

Reviewing files that changed from the base of the PR and between 56b1e15 and 8fa84d4.

📒 Files selected for processing (6)
  • .coderabbit.yaml
  • src/assets/mesh.rs
  • src/assets/mod.rs
  • src/assets/pools.rs
  • src/state.rs
  • src/time/mod.rs
✅ Files skipped from review due to trivial changes (1)
  • src/assets/mesh.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/time/mod.rs
  • .coderabbit.yaml

Comment thread src/assets/pools.rs
Comment thread src/state.rs
@gariasf

gariasf commented May 2, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented May 2, 2026

Copy link
Copy Markdown
✅ Actions performed

Reviews resumed.

@gariasf

gariasf commented May 2, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 2, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@gariasf gariasf merged commit 52bf2a6 into main May 2, 2026
5 checks passed
gariasf added a commit that referenced this pull request May 2, 2026
@gariasf gariasf deleted the feature/ecs branch May 2, 2026 19:41
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