Implement comprehensive lag prevention mechanisms with dynamic entity limits based on performance - #35
Conversation
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
…on performance degradation Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
|
@copilot Max entities should be determined dynamically depending on lag recognition. Keep it simple though. |
|
@copilot Continue your work |
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
I've implemented dynamic max entities based on lag recognition as requested. The system now: ✅ Monitors real-time performance during gameplay (measures actual tick execution time)
The system successfully adapts from 50 down to 20 entities under simulated lag conditions and increases limits back up when performance improves. All 39 tests pass including 10 new tests specifically for the dynamic functionality. Commit: 88692a2 |
|
Verified functionality by running the game locally a few times |
|
@copilot address merge conflicts |
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Merge conflicts resolved successfully. The conflict was in ✅ Keeping our version of The merge cleanly integrated master's expanded names system with our dynamic lag prevention features. Both feature sets work together seamlessly. Commit: 6a4f32b |
This PR's whole point is avoiding per-tick lag at high entity counts, so these fixes target the mechanism itself: - World.removeEntity() was called once per dead/culled entity via list.remove() (O(n) each), both in the per-tick death cleanup (kreatures.py initiateEntityActions) and in cullWeakestEntities - O(n*k) overall on the exact hot path this feature exists to keep cheap. Added World.removeEntities() to filter the entity list once in O(n) and used it in both call sites. - LivingEntity.addLogEntry() copied up to maxLogSize elements via list slicing on every call once the log hit its cap; log entries are appended on nearly every action, every tick, for every entity. Now backed by a deque bounded to maxLogSize for O(1) amortized appends. - Removed dead hasattr(e, 'health')/getattr(x, 'children', []) checks in cullWeakestEntities - every entity is a LivingEntity, which always sets both attributes. - Deduped the average-tick-time computation (was independently recomputed in monitorPerformance and printSummary) into a single getAverageTickTime() helper. - Named the previously-magic 0.7 cull-target fraction as config.entityCullTarget, matching the existing entityCullThreshold pattern. Left game.tickTimes as a plain list (not a deque) despite the same per-tick slicing pattern - tests assert isinstance(game.tickTimes, list) and list equality, and the window is only 10 entries so the cost is negligible. All 64 existing tests pass unchanged. Security review of this PR's diff found no concrete vulnerabilities to fix (no new I/O, network, subprocess, eval, or user-input-to-sensitive-sink flows). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@copilot resolve the merge conflicts in this pull request |
|
@copilot please fix the merge conflicts in this pull request |
Merge conflicts resolved in commit
All 64 tests pass. |
The merge conflicts are already resolved. Our branch includes all of master's commits (merge base is |
|
Adopting this PR as part of an autonomous dev-loop cycle (concurrent-author PR from app/copilot-swe-agent, addressing open issue #34). Brought the branch's state current (it was already even with master), ran the full local test suite against the branch head in an isolated worktree, and performed a self-review rubric. Local test suite: PASS — python3 -m pytest --verbose --cov=src --cov-report=term-missing → 64 passed, 0 failed, against the actual PR head (9d0b4d5), not just the GitHub Actions run-program.yml smoke workflow (which doesn't run pytest and shouldn't be read as certifying this). Self-review rubric:
Do-not-auto-merge check: git diff --stat origin/master...HEAD shows cov.xml with 339 deleted lines, a single file with more than 50 lines deleted, which trips this loop's universal do-not-auto-merge heuristic. In substance this is a generated coverage-report artifact being correctly untracked (paired with the .gitignore addition of cov.xml/.coverage/pycache), not a source-code deletion, but per this loop's own merge gate, that heuristic is not being overridden without explicit human sign-off. Holding this PR open rather than auto-merging. Recommendation: this PR is otherwise ready — tests green, self-review clean, issue #34 addressed. Merge whenever a human confirms the cov.xml deletion is fine (it is, by inspection — it's a generated file already covered by the new .gitignore entries). This comment was drafted during a Gardener session (Stephenson-Software/gardener). |
Removing the "placeholder" string from World's starter entities left a real creature at index 0, so run()'s `entities[0] = playerCreature` silently deleted Alison from the world. Insert the player instead, via a new placePlayerCreature() method so the behavior is directly testable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
.gitignore already lists .coverage alongside cov.xml, but the file stayed tracked, so every local test run dirtied the working tree and re-created the merge conflicts this PR's .gitignore change was meant to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review (autonomous dev-loop cycle, adopted PR)This PR was re-reviewed at head after being carried over from a previous cycle. One correctness regression was found and fixed on the branch; the remainder is sound. External anchor — local test suite: PASS. Finding fixed on this branch (blocking, now resolved)
Verified empirically rather than by reasoning: The assignment was replaced with an insertion, extracted into a new Additional change
Non-blocking observations
Merge gate
This comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener). |
Problem
The Kreatures simulation suffered from severe performance degradation as the game progressed. Shortly after startup, the time between player updates would increase exponentially, making the game unplayable. The root causes were:
Solution
This PR implements a comprehensive lag prevention system that maintains excellent performance while preserving all core game mechanics:
Dynamic Population Control System
Real-time Performance Monitoring
Memory Management
addLogEntry()method maintains log size limits transparentlyBug Fixes and Optimizations
Performance Results
The improvements are dramatic:
Testing
Added comprehensive test coverage with 21 new tests covering:
All 64 tests pass (18 original + 21 lag prevention + 25 names/integration from master) with comprehensive coverage of both static and dynamic population management features.
Merge with Master
This PR has been successfully merged with master branch, which included:
src/config/names.json)_load_names()method with fallback handlingThe merge was clean and both feature sets (dynamic lag prevention and expanded names system) work together seamlessly.
Backward Compatibility
All changes are fully backward compatible:
The simulation now provides a smooth, responsive gaming experience that automatically adapts to system performance, preventing lag while maintaining engaging gameplay regardless of how long it runs or how many creatures are created.
Fixes #34
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.