docs: pin the ECS layering as the decided design; stop allocating bundle lists per tick - #2359
Merged
Merged
Conversation
…dle lists per tick Components own state, generated bundles are the facade, extensions are per-entity behaviour, systems are for iteration-heavy queries - written down so new state stops landing in whichever layer was convenient. The map life loop also enumerated the Monsters/Npcs properties, which materialise a fresh List per access; the tick now walks the backing dictionaries directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
erwan-joly
added a commit
that referenced
this pull request
Aug 30, 2026
…ty (#2354) * perf: one awaited life loop per map instead of a 400ms timer per entity Every monster and NPC ran its own Observable.Interval with a fire-and-forget async tick: a slow tick overlapped the next one on the same entity, and idle maps kept thousands of timers alive just to early-out on IsSleeping. Entity AI now steps sequentially inside the map's PeriodicTimer loop, which awaits each tick before scheduling the next, so load shows up as tick latency instead of unbounded overlap. Movement cadence is unchanged - MoveAsync keeps its own randomized 400-3200ms gate. The per-entity Life handle and the stop/start plumbing around sleep transitions go away with the timers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: pin the ECS layering as the decided design; stop allocating bundle lists per tick (#2359) Components own state, generated bundles are the facade, extensions are per-entity behaviour, systems are for iteration-heavy queries - written down so new state stops landing in whichever layer was convenient. The map life loop also enumerated the Monsters/Npcs properties, which materialise a fresh List per access; the tick now walks the backing dictionaries directly. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: start the life loop once, and let a tick finish before the world goes Two from review. StartLifeAsync checked _lifeLoop and assigned it without holding anything, so two callers could both see null and start a second loop - every entity on the map ticked twice per interval. The check and the assignment are one critical section now. Dispose cancelled the token and disposed EcsWorld in the same breath. The token only ends the wait between ticks: a tick already inside TickLifeAsync walks the monsters, npcs and sessions of that world and never sees it. It now waits for the loop to come back before the world goes. Tested: builds with 0 warnings, suite green - 1125 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Architecture-review PR 6: "decide what the ECS is".
The review flagged the component/bundle hybrid as a half-migration risk. Reading the generator changed the verdict: bundles are generated projections over real Arch components — the design is coherent, it just wasn't written down, so nothing stopped the next feature from parking state in a bundle body, a keyed service dictionary, or a static.
Monsters/Npcs, which allocate a freshListper access — 4 list snapshots per map per 400 ms tick. The tick now enumerates the backing dictionaries.Stacked on #2354 (base:
arch/map-tick-loop) since it edits the same tick body — merge #2354 first, then retarget/rebase this onto master (one click).Build clean; GameObject.Tests 535/535.
If you'd rather push further toward real systems (e.g. moving
MonsterAitarget scans into a query-based system), that's a follow-up we can scope separately — this PR deliberately locks in the current shape instead.🤖 Generated with Claude Code