Refactor inventory system into dedicated InventoryConfig class #4356
Conversation
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
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
daveey
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This was referenced Dec 14, 2025
Closed
Contributor
Merge activity
|
daveey
force-pushed
the
daveey-inventory-config
branch
from
0735eea to
80c1cf6
Compare
daveey
force-pushed
the
daveey-inventory-config
branch
from
80c1cf6 to
7668fd5
Compare
daveey
changed the base branch from
graphite-base/4356
to
daveey-inventory-init
This was referenced Dec 15, 2025
daveey
changed the base branch from
daveey-inventory-init
to
graphite-base/4356
daveey
force-pushed
the
daveey-inventory-config
branch
from
7668fd5 to
c8b1cf2
Compare
daveey
force-pushed
the
graphite-base/4356
branch
from
3e018fc to
490050d
Compare
sasmith
reviewed
Dec 22, 2025
sasmith
reviewed
Dec 22, 2025
sasmith
reviewed
Dec 22, 2025
sasmith
reviewed
Dec 22, 2025
sasmith
reviewed
Dec 22, 2025
sasmith
reviewed
Dec 22, 2025
packages/mettagrid/cpp/include/mettagrid/systems/observation_encoder.hpp
Outdated
Show resolved
Hide resolved
sasmith
reviewed
Dec 22, 2025
packages/mettagrid/cpp/include/mettagrid/systems/observation_encoder.hpp
Outdated
Show resolved
Hide resolved
sasmith
reviewed
Dec 22, 2025
packages/mettagrid/cpp/include/mettagrid/systems/observation_encoder.hpp
Outdated
Show resolved
Hide resolved
sasmith
requested changes
Dec 22, 2025
Contributor
sasmith
left a comment
sasmith
left a comment
There was a problem hiding this comment.
Requesting changes again.
If you want to extract the increased inventory amounts, that would be fine. If you want to leave them in, the base-vs-max issue needs to get fixed (it will have a nice side effect of capping the number of tokens to 2).
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
daveey
force-pushed
the
daveey-inventory-config
branch
from
38ea7cd to
ef5f7f2
Compare
daveey
force-pushed
the
daveey-inventory-config
branch
from
ef5f7f2 to
0fdcc0b
Compare
Contributor
Author
|
I somehow merged the multi-token changes into this branch, instead of the one above. i folded them both into one, so it makes more sense now |
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
sasmith
reviewed
Dec 23, 2025
| } | ||
|
|
||
| // Encode inventory amount using multi-token encoding with configurable base. | ||
| // inv:{resource} = amount % token_value_base (always emitted) |
Contributor
There was a problem hiding this comment.
Sort of interesting that we leave this off if the total resource value is zero, but we include it if the total value is non-zero, even if this particular component is zero. Seems fine -- just interesting.
sasmith
approved these changes
Dec 23, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 24, 2025
## Summary - Default the agent obs shim to ignore inventory power tokens (`inv:*:pN`) and remap feature IDs to legacy ordering - Restore legacy inventory normalization (100.0) for those remapped features - Fix power-token detection regex so the shim is effective ## Why Inventory power tokens introduced in #4356 shifted feature IDs and broke older policies trained on the pre-split inventory encoding. We only care about counts up to 255 for agents, so remapping to the legacy scheme restores compatibility. ## Changes - `ObsShimTokens` now defaults `ignore_inventory_power_tokens=True` - Power-token names are detected correctly and remapped to UNKNOWN - Legacy feature ID ordering + legacy normalization restored for inventory features ## Testing - `uv run cogames run -m machina_1.open_world -c 4 -v heart_chorus -p s3://softmax-public/policies/relh.machina_bc_dinky_sup.hc.1221.10b.0.35/relh.machina_bc_dinky_sup.hc.1221.10b.0.35:v2700.mpt -e 1 -s 1000 --format json`
This was referenced Dec 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
TL;DR
Refactored inventory management by introducing a dedicated
InventoryConfigclass and restructuring inventory-related properties across the codebase.What changed?
InventoryConfigclass to encapsulate inventory-related propertiesAgentConfigandChestConfiginto the newInventoryConfig:initial_inventory→inventory.initialresource_limits→inventory.limitsdefault_resource_limit→inventory.default_limitinventory_regen_amounts→inventory.regen_amountsHow to test?
Why make this change?
This refactoring improves code organization by grouping related inventory properties into a dedicated class, making the codebase more maintainable and easier to understand. The change also removes unused features like inventory diversity tracking and soul-bound resources, simplifying the codebase. The increased inventory quantity size allows for larger inventory values, providing more flexibility in game design.