feat(mettagrid): add TagIndex for fast event filtering #4990
+2,543
−154
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
This comment has been minimized.
This comment has been minimized.
![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.
Reviewed commit: 708946fc58
ℹ️ 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".
Comment on lines
118
to
122
| cpp_filter = CppObjectTypeFilterConfig() | ||
| cpp_filter.entity = convert_entity_ref(filter_config.target) | ||
| # Use hash of tag name as ID (this is a placeholder - actual implementation | ||
| # should use proper tag name to ID mapping) | ||
| cpp_filter.tag_id = hash(tag_name) % (2**31) |
There was a problem hiding this comment.
The ObjectTypeFilter conversion sets tag_id to hash(tag_name) % (2**31), which does not match the tag IDs generated by IdMap/tag_name_to_id and is randomized by PYTHONHASHSEED. That means any ObjectTypeFilter in handlers/events will never match actual objects and TagIndex pre-filtering will always return empty candidates, so those events/handlers won't fire. Use the tag_name_to_id mapping (and raise if the tag is unknown) instead of hashing.
Useful? React with 👍 / 👎.
daveey
force-pushed
the
daveey-tag-index
branch
from
fd2c1f1 to
e22fe4f
Compare
daveey
force-pushed
the
daveey-tag-index
branch
from
39fd14f to
f59dc30
Compare
Contributor
Author
This was referenced Jan 19, 2026
Draft
This comment has been minimized.
This comment has been minimized.
|
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 |
Add a TagIndex class that maintains a mapping from tag IDs to lists of GridObjects with that tag. This provides O(1) lookup for finding objects by tag, which will be used by EventScheduler for fast event filtering. The TagIndex provides: - register_object: Index all tags on an object - unregister_object: Remove object from all tag indices - on_tag_added/removed: Dynamic tag updates - get_objects_with_tag: O(1) lookup by tag ID - count_objects_with_tag: Get count without allocation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add has_tag, add_tag, and remove_tag methods to GridObject for runtime tag manipulation. These methods integrate with TagIndex to keep indices synchronized when tags change. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add TagIndex member to MettaGrid class - Wire up TagIndex to objects on creation via set_tag_index and register_object - Add tag_index() Python binding for accessing TagIndex from Python - Add count_objects_with_tag method to TagIndex Python binding - Fix tag ID mapping to include default agent tags for consistency between Python IdMap.tag_names() and C++ convert_to_cpp_game_config This enables efficient tag-based object lookup from both C++ and Python. The TagIndex is populated when objects are created and stays in sync when tags are added or removed via add_tag/remove_tag methods. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add mutation classes that allow handlers and events to add or remove tags from objects. This enables dynamic tag manipulation during gameplay, allowing objects to change their tags based on game events. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add get_filters() accessor to Event class - Update EventScheduler::process_timestep to accept TagIndex - Implement pre-filtering logic that finds most selective tag filter - Use TagIndex to get candidate objects before applying remaining filters Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ciency - Add model_validator to EventConfig requiring at least one ObjectTypeFilter - Events must have tag-based filters for efficient O(1) lookups via TagIndex - Update all existing event configurations in tests and missions to include HasTag - Add test_event_validation.py with validation tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add note to test_tag_index.py explaining that event-based integration tests are pending EventScheduler integration with MettaGrid. The core tag mutation + TagIndex sync functionality is already tested in test_tag_index_syncs_with_add_remove. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ective resolution - Add events field to C++ GameConfig and pybind11 bindings - Add convert_event_configs_to_cpp for Python-to-C++ event conversion - Pass collectives_by_name to EventScheduler for resolving collective pointers - Add get_mutations() accessor to Event class for collective resolution - Call EventScheduler.process_timestep() from MettaGrid._step() - Add integration tests verifying events fire and apply mutations correctly The key fix was resolving collective pointers in AlignToCollectiveMutation during EventScheduler initialization, as set_collective() was never being called, leaving the resolved pointer as nullptr. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
After rebasing daveey-tag-index onto daveey-events, several inconsistencies arose due to overlapping changes. This commit fixes: - Add ObjectTypeFilterConfig, CollectiveFilterConfig, and NearCollectiveFilterConfig to C++ handler_config.hpp - Add pybind11 bindings for new filter config types - Fix TagFilter/ObjectTypeFilter to use single tag_id - Add max_targets field to EventConfig and its pybind11 binding - Update tests to use HasTag (ObjectTypeFilter) instead of hasTag (TagFilter) where EventConfig requires it - Add Python imports for new filter config types Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
daveey
force-pushed
the
daveey-tag-index
branch
from
f59dc30 to
944a2f8
Compare
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.
Summary
std::unordered_map<int, std::vector<GridObject*>>) for O(1) tag-based object lookupshas_tag,add_tag,remove_tag) that automatically sync with TagIndexTest plan
🤖 Generated with Claude Code