Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
39 changes: 39 additions & 0 deletions .devcontainer/devcontainer.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
"name": "echo-dev",
"image": "mcr.microsoft.com/devcontainers/base:ubuntu-24.04",
"features": {
"ghcr.io/devcontainers/features/common-utils:2": {
"username": "vscode",
"installZsh": false
},
"ghcr.io/devcontainers/features/rust:1": {
"version": "1.90",
"profile": "minimal",
"components": ["rustfmt", "clippy"]
},
"ghcr.io/devcontainers/features/node:1": {
"version": "20"
},
"ghcr.io/devcontainers/features/github-cli:1": {}
},
"customizations": {
"vscode": {
"extensions": [
"rust-lang.rust-analyzer",
"serayuzgur.crates",
"tamasfe.even-better-toml",
"vadimcn.vscode-lldb"
]
}
},
"mounts": [
"source=devcontainer-cargo-cache,target=/usr/local/cargo,type=volume",
"source=devcontainer-rustup-cache,target=/usr/local/rustup,type=volume"
],
"containerEnv": {
"CARGO_TERM_COLOR": "always"
},
"overrideCommand": false,
"postCreateCommand": "/bin/bash .devcontainer/post-create.sh"
}

19 changes: 19 additions & 0 deletions .devcontainer/post-create.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#!/usr/bin/env bash
set -euo pipefail

echo "[devcontainer] Installing MSRV toolchain (1.68.0) and common targets..."
if ! command -v rustup >/dev/null 2>&1; then
curl --proto '=https' --tlsv1.2 --retry 10 --retry-connrefused --location --silent --show-error --fail https://sh.rustup.rs | sh -s -- --default-toolchain none -y
export PATH="$HOME/.cargo/bin:$PATH"
fi
Comment on lines +5 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fragile rustup installation flow.

If rustup is not found, you install it but only export PATH in the current shell context. If the curl succeeds but the install script fails silently (wrong arch, permissions, whatever), subsequent rustup commands will blow up with cryptic errors. Plus, you're not verifying the installation actually worked.

Add explicit verification:

 if ! command -v rustup >/dev/null 2>&1; then
   curl --proto '=https' --tlsv1.2 --retry 10 --retry-connrefused --location --silent --show-error --fail https://sh.rustup.rs | sh -s -- --default-toolchain none -y
   export PATH="$HOME/.cargo/bin:$PATH"
+  if ! command -v rustup >/dev/null 2>&1; then
+    echo "[devcontainer] ERROR: rustup installation failed" >&2
+    exit 1
+  fi
 fi
📝 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 ! command -v rustup >/dev/null 2>&1; then
curl --proto '=https' --tlsv1.2 --retry 10 --retry-connrefused --location --silent --show-error --fail https://sh.rustup.rs | sh -s -- --default-toolchain none -y
export PATH="$HOME/.cargo/bin:$PATH"
fi
if ! command -v rustup >/dev/null 2>&1; then
curl --proto '=https' --tlsv1.2 --retry 10 --retry-connrefused --location --silent --show-error --fail https://sh.rustup.rs | sh -s -- --default-toolchain none -y
export PATH="$HOME/.cargo/bin:$PATH"
if ! command -v rustup >/dev/null 2>&1; then
echo "[devcontainer] ERROR: rustup installation failed" >&2
exit 1
fi
fi
🤖 Prompt for AI Agents
In .devcontainer/post-create.sh around lines 5 to 8, the rustup install only
updates PATH in the current shell and doesn't verify the installer succeeded,
which can leave subsequent rustup calls failing silently; change the flow to run
the installer and check its exit status, source or append the cargo environment
export to a persistent shell profile (e.g. ~/.profile or ~/.bashrc) so PATH is
available in new shells, then verify rustup by running `rustup --version` (or
`command -v rustup`) and if verification fails output a clear error and exit
non‑zero so CI/dev containers fail fast.


rustup toolchain install 1.68.0 --profile minimal
rustup default stable
rustup component add rustfmt clippy
rustup target add wasm32-unknown-unknown

echo "[devcontainer] Priming cargo registry cache (optional)..."
cargo fetch || true
Comment on lines +15 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

|| true makes cargo fetch failures invisible.

You're swallowing ALL exit codes from cargo fetch. If there's a real network issue, corrupted registry, or missing Cargo.toml, you'll never know. The "optional" comment doesn't justify hiding actual errors.

Either remove the || true and let it fail fast, or at least log the failure:

 echo "[devcontainer] Priming cargo registry cache (optional)..."
-cargo fetch || true
+cargo fetch || echo "[devcontainer] WARNING: cargo fetch failed; dependencies may be fetched on first build" >&2
📝 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
echo "[devcontainer] Priming cargo registry cache (optional)..."
cargo fetch || true
echo "[devcontainer] Priming cargo registry cache (optional)..."
cargo fetch || echo "[devcontainer] WARNING: cargo fetch failed; dependencies may be fetched on first build" >&2
🤖 Prompt for AI Agents
In .devcontainer/post-create.sh around lines 15-16, the script currently
silences all failures from `cargo fetch` by appending `|| true`; remove that so
the container build fails fast on real errors, or if you must keep it optional,
replace the silent suppression with an explicit failure path that logs the error
and exits non-zero (or at minimum logs the exit code to stderr) so
network/registry/Cargo.toml issues are visible during setup.


echo "[devcontainer] Done. Run 'cargo test -p rmg-core' or 'make ci-local' to validate."
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

References potentially non-existent make ci-local target.

You're telling users to run make ci-local, but there's no Makefile in this PR. If that target doesn't exist, developers will get a cryptic error and wonder if the devcontainer is broken.

Verify the target exists or update the message:


🏁 Script executed:

#!/bin/bash
# Check if make ci-local target exists
if [ -f Makefile ]; then
  grep -q "^ci-local:" Makefile && echo "Target exists" || echo "Target missing"
else
  echo "No Makefile found"
fi

Length of output: 14



Fix the broken make ci-local reference in post-create.sh

Your devcontainer post-create message tells users to run a make target that doesn't exist. The target verification came back negative, confirming the reviewer's concern. Either remove the reference from the message or actually add the make ci-local target to your Makefile—don't send developers on a wild goose chase.

echo "[devcontainer] Done. Run 'cargo test -p rmg-core' to validate."

Update line 18 in .devcontainer/post-create.sh accordingly.


BunBun

🤖 Prompt for AI Agents
In .devcontainer/post-create.sh around line 18, the completion message
references a non-existent make target `make ci-local`; update the echo to remove
the broken `make ci-local` reference (leave only a valid instruction such as
"Run 'cargo test -p rmg-core' to validate.") so the post-create message points
developers to an actual command.


11 changes: 9 additions & 2 deletions .githooks/pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,15 @@ if command -v rustup >/dev/null 2>&1; then
fi
fi

# 3) Format check (fast)
cargo fmt --all -- --check
# 3) Format (auto-fix if opted in)
if [[ "${ECHO_AUTO_FMT:-1}" == "1" ]]; then
echo "pre-commit: ECHO_AUTO_FMT=1 (default) → running cargo fmt (auto-fix)"
cargo fmt --all
# Re-stage any formatting changes
git add -A
else
cargo fmt --all -- --check
fi

# 4) Docs guard (scaled): only require docs when core public API changed
STAGED=$(git diff --cached --name-only)
Expand Down
6 changes: 6 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ Welcome to the **Echo** project. This file captures expectations for any LLM age
- Respect determinism: preferably no random seeds without going through the Echo PRNG.
- Run `cargo clippy --all-targets -- -D missing_docs` and `cargo test` before every PR; CI will expect a zero-warning, fully documented surface.

### Git Hooks & Local CI
- Install repo hooks once with `make hooks` (configures `core.hooksPath`).
- Formatting: pre-commit auto-fixes with `cargo fmt` by default. Set `ECHO_AUTO_FMT=0` to run check-only instead.
- Toolchain: pre-commit verifies your active toolchain matches `rust-toolchain.toml`.
- Docs Guard: when core API files change, the hook requires updating `docs/execution-plan.md` and `docs/decision-log.md` (mirrors the CI check).

## Git Real
1. **NEVER** use `--force` with any git command. If you think you need it, stop and ask the human for help.
2. **NEVER** use rebase. Embrace messy distributed history; plain merges capture the truth, rebases rewrite it.
Expand Down
12 changes: 12 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ Echo is a deterministic, renderer-agnostic engine. We prioritize:
1. Clone the repo and run `cargo check` to ensure the Rust workspace builds.
2. Read `docs/architecture-outline.md` and `docs/execution-plan.md`.
3. Review `AGENTS.md` for collaboration norms before touching runtime code.
4. Optional: develop inside the devcontainer for toolchain parity with CI.
- Open in VS Code → "Reopen in Container" (requires the Dev Containers extension).
- The container includes Rust stable + MSRV toolchains, clippy/rustfmt, Node, and gh.
- Post-create installs MSRV 1.68.0 and wasm target.

## Branching & Workflow
- Keep `main` pristine. Create feature branches like `echo/<feature>` or `timeline/<experiment>`.
Expand Down Expand Up @@ -53,6 +57,14 @@ Echo is a deterministic, renderer-agnostic engine. We prioritize:
- TypeScript tooling (when active) lives in `reference/typescript/`; follow local lint configs when reactivated.
- Avoid non-deterministic APIs (no wall-clock, no uncontrolled randomness). Use Echo’s deterministic services.

### Git Hooks (recommended)
- Install repo hooks once: `make hooks` (configures `core.hooksPath` to `.githooks`).
- Pre-commit runs:
- cargo fmt (auto-fix by default; set `ECHO_AUTO_FMT=0` for check-only)
- Toolchain pin verification (matches `rust-toolchain.toml`)
- A minimal docs-guard: when core API files change, it requires updating `docs/execution-plan.md` and `docs/decision-log.md` (mirrors CI)
- To auto-fix formatting on commit: `ECHO_AUTO_FMT=1 git commit -m "message"`

## Communication
- Major updates should land in `docs/execution-plan.md` and `docs/decision-log.md`; rely on GitHub discussions or issues for longer-form proposals.
- Respect the temporal theme—leave the codebase cleaner for the next timeline traveler.
Expand Down
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ members = [
"crates/rmg-core",
"crates/rmg-ffi",
"crates/rmg-wasm",
"crates/rmg-cli"
"crates/rmg-cli",
"crates/rmg-geom"
]
resolver = "2"

Expand Down
17 changes: 17 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,23 @@ There's a ton of other advanced reasons why it's cool, but that's nerd stuff. Le
- Use expressive commits (`subject` / `body` / optional `trailer`)—tell future us the *why*, not just the *what*.
- Treat determinism as sacred: prefer Echo’s PRNG, avoid non-deterministic APIs without wrapping them.

### Git Hooks

Install the repo’s hooks so formatting and quick checks run before commits:

```
make hooks
```

- The pre-commit hook auto-fixes formatting by default (runs `cargo fmt --all`).
- To switch to check-only mode for a commit, set `ECHO_AUTO_FMT=0`:

```
ECHO_AUTO_FMT=0 git commit -m "your message"
```

You can also export `ECHO_AUTO_FMT=0` in your shell rc if you prefer check-only always.

### Development Principles

1. **Tests First** – Write failing unit/integration/branch tests before new engine work.
Expand Down
2 changes: 1 addition & 1 deletion crates/rmg-core/src/tx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
/// - Zero (`TxId(0)`) is reserved as invalid. [`crate::Engine::begin`] never returns zero.
/// - External callers using [`TxId::from_raw`] must not construct `TxId(0)` unless
/// they have a valid reason (e.g., sentinel in FFI); using invalid ids with engine
/// operations returns [`EngineError::UnknownTx`].
/// operations returns [`crate::engine_impl::EngineError::UnknownTx`].
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify the module path for EngineError::UnknownTx exists and is accessible.

The rustdoc comment references crate::engine_impl::EngineError::UnknownTx, but I cannot confirm this path is correct or that the module is publicly re-exported from this crate. Ensure the path resolves correctly in generated documentation.


🏁 Script executed:

#!/bin/bash
# Verify the EngineError type and UnknownTx variant exist in the engine_impl module
ast-grep --pattern 'enum EngineError { $$$ UnknownTx $$$ }'
rg -n 'mod engine_impl' crates/rmg-core/src/
rg -n 'pub.*EngineError' crates/rmg-core/src/

Length of output: 429


I need to examine the full EngineError enum definition to verify if the UnknownTx variant exists.


🏁 Script executed:

#!/bin/bash
# Extract the EngineError enum definition
sed -n '26,100p' crates/rmg-core/src/engine_impl.rs

Length of output: 77



---

🏁 Script executed:

```shell
#!/bin/bash
# Extract the EngineError enum definition from engine_impl.rs
sed -n '26,80p' crates/rmg-core/src/engine_impl.rs

Length of output: 2269


🏁 Script executed:

#!/bin/bash
# Also check the tx.rs file to see the exact comment
sed -n '10,20p' crates/rmg-core/src/tx.rs

Length of output: 724


🏁 Script executed:

#!/bin/bash
# Search for UnknownTx variant in the codebase
rg 'UnknownTx' crates/rmg-core/src/

Length of output: 552


Use the public re-export path in rustdoc references.

The UnknownTx variant exists and is accessible, but the rustdoc path violates Rust conventions. Since EngineError is publicly re-exported at the crate root (lib.rs:54), the reference should use crate::EngineError::UnknownTx instead of crate::engine_impl::EngineError::UnknownTx. Internal module paths don't belong in public documentation—use the public re-export.

Change line 15 in crates/rmg-core/src/tx.rs from:

operations returns [`crate::engine_impl::EngineError::UnknownTx`].

to:

operations returns [`crate::EngineError::UnknownTx`].

From: Linus Torvalds torvalds@linux-foundation.org
To: {PR_AUTHOR_NAME}
Cc: code-rabbit@vger.kernel.org

Look, the path technically resolves, but you're bypassing the entire reason we have public re-exports. This isn't even a style thing—it's about readers seeing the actual public API. Don't bury it under internal module names. Fix it.

BunBun

🤖 Prompt for AI Agents
In crates/rmg-core/src/tx.rs around line 15, the rustdoc reference uses an
internal module path (`crate::engine_impl::EngineError::UnknownTx`) instead of
the public re-export; update the doc comment to reference the public API path
`crate::EngineError::UnknownTx` so the documentation points to the crate root
re-export rather than an internal module.

///
/// The `#[repr(transparent)]` attribute ensures FFI ABI compatibility: `TxId` has
/// the same memory layout as `u64` across the FFI/Wasm boundary.
Expand Down
15 changes: 15 additions & 0 deletions crates/rmg-geom/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
[package]
name = "rmg-geom"
version = "0.1.0"
edition = "2021"
description = "Echo geometry primitives: AABB, transforms, temporal proxies, and broad-phase scaffolding"
license = "Apache-2.0"
repository = "https://example.invalid/echo"
readme = "../../README.md"
keywords = ["echo", "geometry", "aabb", "broad-phase"]
categories = ["game-engines", "data-structures"]

[dependencies]
rmg-core = { path = "../rmg-core" }

#[dev-dependencies]
33 changes: 33 additions & 0 deletions crates/rmg-geom/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#![deny(
warnings,
clippy::all,
clippy::pedantic,
rust_2018_idioms,
missing_docs,
clippy::unwrap_used,
clippy::expect_used,
clippy::panic
)]
#![doc = r"Geometry primitives for Echo.

This crate provides:
- Axis-aligned bounding boxes (`Aabb`).
- Rigid transforms (`Transform`).
- Temporal utilities (`Tick`, `TemporalTransform`, `TemporalProxy`).
- A minimal broad-phase trait and an AABB-based pairing structure.

Design notes:
- Deterministic: no ambient RNG; ordering of pair outputs is canonical.
- Float32 throughout; operations favor clarity and reproducibility.
- Rustdoc is treated as part of the contract; public items are documented.
"]

/// Foundational geometric types.
pub mod types;
/// Time-aware utilities for broad-phase and motion.
pub mod temporal;
// Broad-phase will land in a follow-up PR.
// pub mod broad;

pub use types::aabb::Aabb;
pub use types::transform::Transform;
8 changes: 8 additions & 0 deletions crates/rmg-geom/src/temporal/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
//! Temporal types and helpers used for tick-based motion and broad-phase.

#[doc = "Discrete Chronos ticks (u64 newtype)."]
pub mod tick;
#[doc = "Start/end transforms over a tick and fat AABB computation."]
pub mod temporal_transform;
#[doc = "Broad-phase proxy carrying entity id, tick, and fat AABB."]
pub mod temporal_proxy;
32 changes: 32 additions & 0 deletions crates/rmg-geom/src/temporal/temporal_proxy.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
use crate::temporal::tick::Tick;
use crate::types::aabb::Aabb;

/// Broad-phase proxy summarizing an entity’s motion window for a tick.
///
/// Stores a conservative fat AABB and the owning `entity` identifier (opaque
/// to the geometry layer). The proxy is suitable for insertion into a broad-
/// phase accelerator.
#[derive(Debug, Copy, Clone, PartialEq)]
pub struct TemporalProxy {
entity: u64,
tick: Tick,
fat: Aabb,
}

impl TemporalProxy {
/// Creates a new proxy for `entity` at `tick` with precomputed `fat` AABB.
#[must_use]
pub const fn new(entity: u64, tick: Tick, fat: Aabb) -> Self { Self { entity, tick, fat } }

/// Opaque entity identifier owning this proxy.
#[must_use]
pub const fn entity(&self) -> u64 { self.entity }

/// Tick associated with the motion window.
#[must_use]
pub const fn tick(&self) -> Tick { self.tick }

/// Conservative fat AABB for this proxy.
#[must_use]
pub const fn fat(&self) -> Aabb { self.fat }
}
44 changes: 44 additions & 0 deletions crates/rmg-geom/src/temporal/temporal_transform.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
use crate::types::{aabb::Aabb, transform::Transform};

/// Transform at two adjacent ticks used to bound motion in the broad-phase.
///
/// - `start` corresponds to tick `n`.
/// - `end` corresponds to tick `n+1`.
///
/// Determinism and plan:
/// - `fat_aabb` below currently uses a union of the start/end AABBs. This is
/// conservative for linear motion and sufficient for pairing/CCD triggers.
/// - Future: introduce a quantized margin policy (based on velocity, `dt`, and
/// shape scale) so that fat proxies are identical across peers/branches. The
/// policy and quantization will be recorded in the graph/spec.
#[derive(Debug, Copy, Clone, PartialEq)]
pub struct TemporalTransform {
start: Transform,
end: Transform,
}

impl TemporalTransform {
/// Creates a new `TemporalTransform` from start and end transforms.
#[must_use]
pub const fn new(start: Transform, end: Transform) -> Self { Self { start, end } }

/// Returns the start transform.
#[must_use]
pub const fn start(&self) -> Transform { self.start }

/// Returns the end transform.
#[must_use]
pub const fn end(&self) -> Transform { self.end }

/// Computes a conservative fat AABB for a collider with local-space `shape` AABB.
///
/// The fat box is defined as the union of the shape’s AABBs at the start and
/// end transforms. This is conservative for linear motion and suffices for
/// broad-phase pairing and CCD triggering.
#[must_use]
pub fn fat_aabb(&self, shape: &Aabb) -> Aabb {
let a0 = shape.transformed(&self.start.to_mat4());
let a1 = shape.transformed(&self.end.to_mat4());
a0.union(&a1)
Comment on lines +33 to +42

Choose a reason for hiding this comment

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

P1 Badge Include rotation sweep in fat AABB

TemporalTransform::fat_aabb unions the shape’s AABBs at ticks n and n+1 and treats that as a conservative bound. That holds for pure translation but fails when the transform includes rotation or non‑uniform scale: an object can grow outside both endpoint AABBs mid‑tick (e.g. a box rotating 45°), letting the broad phase miss overlaps and CCD triggers. The method should inflate for rotational sweep or explicitly forbid rotation; otherwise it under-approximates motion envelopes.

Useful? React with 👍 / 👎.

}
}
Comment on lines +1 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

CRITICAL: Still not running cargo fmt, I see.

Three files in a row with formatting failures. Do you not have a pre-commit hook? Do you not run CI locally? This is basic hygiene and you're failing at it spectacularly.

#!/bin/bash
cargo fmt -- crates/rmg-geom/src/temporal/temporal_transform.rs

BunBun

🧰 Tools
🪛 GitHub Actions: CI

[error] 1-1: cargo fmt --check failed: rustfmt would reformatted code in temporal_transform.rs. Run 'cargo fmt' to fix formatting.

🤖 Prompt for AI Agents
crates/rmg-geom/src/temporal/temporal_transform.rs lines 1-44: the file is not
formatted with rustfmt; run the formatter and commit the result. Fix by running
`cargo fmt -- crates/rmg-geom/src/temporal/temporal_transform.rs` (or `cargo
fmt` for the whole workspace), stage and commit the changed file, and
(optionally) add a pre-commit hook or enable CI formatting check to prevent
future misses.

19 changes: 19 additions & 0 deletions crates/rmg-geom/src/temporal/tick.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/// Discrete simulation tick in Chronos time.
///
/// The engine advances in integer ticks with a fixed `dt` per branch. This
/// newtype ensures explicit tick passing across APIs.
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct Tick {
index: u64,
}

impl Tick {
/// Creates a new tick with the given index.
#[must_use]
pub const fn new(index: u64) -> Self { Self { index } }

/// Returns the tick index.
#[must_use]
pub const fn index(&self) -> u64 { self.index }
}

Loading
Loading