refactor(cogames): use agent_config method in CogConfig #4876
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
daveey
force-pushed
the
graphite-base/4876
branch
from
7e49aad to
0bebaae
Compare
daveey
force-pushed
the
daveey-cog-move
branch
from
59c4687 to
0ff01fb
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| min_val = resource_limit.get("min", resource_limit.get("limit", 0)) | ||
| max_val = resource_limit.get("max", 65535) | ||
| limit_defs.append(CppLimitDef(resource_ids, min_val, max_val, modifier_ids)) |
There was a problem hiding this comment.
Backward compatibility issue: When reading old configs that have limit but not min/max, the code uses limit as min (line 322). This breaks the semantics for configs with modifiers.
Problem: Old config with limit=100 and modifiers that add +50 would give effective limit of 150. After migration, min=100 with +50 modifiers gives max(100, 50) = 100.
Fix: Should migrate limit to max instead of min when modifiers are present:
min_val = resource_limit.get("min", 0)
max_val = resource_limit.get("max", resource_limit.get("limit", 65535))This preserves the old behavior where limit was effectively a cap that modifiers couldn't exceed.
| min_val = resource_limit.get("min", resource_limit.get("limit", 0)) | |
| max_val = resource_limit.get("max", 65535) | |
| limit_defs.append(CppLimitDef(resource_ids, min_val, max_val, modifier_ids)) | |
| min_val = resource_limit.get("min", 0) | |
| max_val = resource_limit.get("max", resource_limit.get("limit", 65535)) | |
| limit_defs.append(CppLimitDef(resource_ids, min_val, max_val, modifier_ids)) | |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| } | ||
| } | ||
| // Apply formula: min(max_limit, max(min_limit, modifier_sum)) | ||
| int effective = std::max(static_cast<int>(min_limit), modifier_sum); |
There was a problem hiding this comment.
Claude, can you change this to use std::clamp?
| InventoryQuantity amount; | ||
|
|
||
| // Get the effective limit (base + sum of modifier bonuses) | ||
| // Get the effective limit: min(max_limit, max(min_limit, sum(modifier_bonus * quantity_held))) |
There was a problem hiding this comment.
The description(s) should also use clamp
|
|
||
| struct SharedInventoryLimit { | ||
| InventoryQuantity base_limit; | ||
| InventoryQuantity min_limit; |
There was a problem hiding this comment.
This all strikes me as a little funky. E.g., why are we at the min_limit if there are no modifiers? It's also pretty easy for ResourceLimitsConfig(min=...) to be confusing, since it definitely suggests that this inventory amount can't be less than the min.
Let's talk about what we're trying to solve?
There was a problem hiding this comment.
I think if we renamed "limit" to "capacity" it would be less confusing.
Apply suggested changes
| limits={ | ||
| "hp": ResourceLimitsConfig(min=self.hp_limit, resources=["hp"], modifiers=self.hp_modifiers), | ||
| # when hp == 0, the cog can't hold gear or hearts | ||
| "gear": ResourceLimitsConfig(max=self.gear_limit, resources=gear, modifiers={"hp": 100}), |
There was a problem hiding this comment.
This feels like a hacky way to accomplish this result, but let's ship it and revisit if needed.
Merge activity
|
# Rename ResourceLimitsConfig.limit to min/max for better semantics ### TL;DR Renamed `limit` to `min`/`max` in `ResourceLimitsConfig` to better represent inventory capacity constraints, and updated all references throughout the codebase. ### What changed? - Changed `ResourceLimitsConfig.limit` to `min` (floor) and `max` (ceiling) parameters - Updated the effective limit calculation formula to: `min(max_limit, max(min_limit, sum(modifier_bonus * quantity_held)))` - Modified C++ implementation in `inventory.hpp` to support the new min/max semantics - Added `agent_config()` method to `CogConfig` to centralize agent configuration - Updated all references to `limit` across the codebase to use the new parameters ### How to test? 1. Run the Cogs vs Clips mission to verify inventory limits work correctly 2. Test dynamic inventory limits with modifiers to ensure they respect both min and max constraints 3. Verify that existing inventory behavior is preserved in all game modes 4. Check that the new agent configuration method correctly applies all settings ### Why make this change? This change provides more intuitive and flexible inventory limit semantics: - `min` establishes a floor for capacity even with no modifiers - `max` sets a ceiling that can't be exceeded regardless of modifiers - The naming better reflects how these values actually constrain inventory capacity - Centralizing agent configuration in `CogConfig` improves code organization and maintainability
Rename ResourceLimitsConfig.limit to min/max for better semantics
TL;DR
Renamed
limittomin/maxinResourceLimitsConfigto better represent inventory capacity constraints, and updated all references throughout the codebase.What changed?
ResourceLimitsConfig.limittomin(floor) andmax(ceiling) parametersmin(max_limit, max(min_limit, sum(modifier_bonus * quantity_held)))inventory.hppto support the new min/max semanticsagent_config()method toCogConfigto centralize agent configurationlimitacross the codebase to use the new parametersHow to test?
Why make this change?
This change provides more intuitive and flexible inventory limit semantics:
minestablishes a floor for capacity even with no modifiersmaxsets a ceiling that can't be exceeded regardless of modifiersCogConfigimproves code organization and maintainability