Remove specific one relationship when its reference pointers are the identical #70
Conversation
…t by checking if the relationship references are the same, not if the objects are the same.
WalkthroughModified entity relationship removal to prefer an exact relationship instance removal first, suppressing subsequent pattern-based removals in that call. If no exact match is found, pattern-based removals proceed (respecting any limit). Added a test for removing a specific relationship instance; the test was accidentally duplicated. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Entity
rect rgb(245,250,255)
note right of Entity: remove_relationship(relationship, limit?)
end
Caller->>Entity: remove_relationship(relationship, limit)
Entity->>Entity: First pass: scan for exact match (rel == relationship)
alt Exact match found
Entity->>Entity: queue exact removal\nset pattern_remove = false
else No exact match
Entity->>Entity: keep pattern_remove = true
end
Entity->>Entity: Second pass: iterate relationships
alt pattern_remove == true
Entity->>Entity: remove rels where rel.matches(relationship)\n(honors limit)
else pattern_remove == false
Entity->>Entity: skip pattern-based removals
end
Entity->>Caller: emit relationship_removed for each removed\nreturn removal result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
addons/gecs/ecs/entity.gd (2)
273-297: Consider performance optimization and improve documentation.The two-pass approach correctly implements exact-match-first with pattern-match fallback, but iterating through
relationshipstwice may impact performance for entities with many relationships.Additionally, the docstring (lines 255-272) should document this new exact-match behavior to clarify that passing an actual relationship instance from
get_relationship()will remove only that specific instance, while other relationship objects trigger pattern-based removal.Consider optimizing to a single-pass implementation:
func remove_relationship(relationship: Relationship, limit: int = -1) -> void: if limit == 0: return var to_remove = [] var removed_count = 0 - - var pattern_remove = true - for rel in relationships: - if rel == relationship: - to_remove.append(rel) - pattern_remove = false - break + # Check for exact match first + var exact_match_index = relationships.find(relationship) + if exact_match_index != -1: + to_remove.append(relationships[exact_match_index]) + else: + # No exact match, use pattern matching for rel in relationships: - if pattern_remove and rel.matches(relationship): + if rel.matches(relationship): to_remove.append(rel) removed_count += 1 - # If limit is positive and we've reached it, stop collecting if limit > 0 and removed_count >= limit: break + } for rel in to_remove: relationships.erase(rel) relationship_removed.emit(self, rel)Also update the docstring to document the exact-match-first behavior:
## Removes a relationship from the entity.[br] ## [param relationship] The [Relationship] to remove.[br] ## [param limit] Maximum number of relationships to remove. -1 = all (default), 0 = none, >0 = up to that many.[br] ## [br] +## If the relationship parameter is an exact instance (same object reference) from the entity's relationships,[br] +## only that specific relationship is removed. Otherwise, pattern matching is used to find matching relationships.[br] +## [br] ## [b]Examples:[/b]
280-280: Minor: Unnecessary blank line added.This empty line doesn't add to code readability in this context.
addons/gecs/tests/core/test_relationships.gd (1)
981-999: Good test coverage for the new exact-match functionality.The test correctly validates that removing a specific relationship instance only removes that one relationship, not all pattern-matching relationships. The test setup is clear and assertions are comprehensive.
Consider adding edge case tests to strengthen coverage:
func test_remove_specific_relationship_not_in_entity(): # Test removing a relationship instance that's not in the entity var external_rel = Relationship.new(C_Likes.new(2), e_alice) e_bob.add_relationship(Relationship.new(C_Likes.new(1), e_alice)) # Should not remove anything since it's not the same instance e_bob.remove_relationship(external_rel) assert_int(e_bob.get_relationships(Relationship.new({C_Likes:{}}, null)).size()).is_equal(1) func test_remove_specific_relationship_with_limit(): # Test that limit parameter is ignored for exact matches var rel = Relationship.new(C_Likes.new(1), e_alice) e_bob.add_relationship(rel) e_bob.remove_relationship(rel, 0) # limit=0 should still work for exact match? # Document expected behavior
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
addons/gecs/ecs/entity.gd(1 hunks)addons/gecs/tests/core/test_relationships.gd(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.gd
📄 CodeRabbit inference engine (CLAUDE.md)
Always use the res:// prefix for file paths in Godot scripts and tests
Files:
addons/gecs/ecs/entity.gdaddons/gecs/tests/core/test_relationships.gd
🔇 Additional comments (2)
addons/gecs/tests/core/test_relationships.gd (2)
980-980: Clarification: Empty line addition.Line 980 shows an empty line was added (marked with
~). This is fine for spacing between test functions.
981-981: No duplicate function declaration found—original review concern is unfounded.The verification confirms there is only one declaration of
test_remove_specific_relationshipat line 981. The AI summary's claim about a duplicate is incorrect. The code requires no changes.Likely an incorrect or invalid review comment.
hwh008
force-pushed
the
fix/remove_relationship_by_pointer
branch
from
c04350f to
e506264
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
addons/gecs/tests/core/test_relationships.gd (1)
981-999: Consider consistent entity setup for better test isolation.The test creates a fresh
e_bobentity but relies one_alicefrom thebefore_test()setup. This mixing of fresh and setup entities could cause issues if the test runs in isolation or if the setup changes.Consider one of these approaches:
Option 1: Use setup entities consistently
func test_remove_specific_relationship(): - e_bob = Person.new() - world.add_entity(e_bob) + # Use e_bob from before_test() setup e_bob.add_relationship(Relationship.new(C_Likes.new(1), e_alice))Option 2: Create all entities fresh for complete isolation
func test_remove_specific_relationship(): e_bob = Person.new() + e_alice = Person.new() + e_alice.name = "e_alice" world.add_entity(e_bob) + world.add_entity(e_alice) e_bob.add_relationship(Relationship.new(C_Likes.new(1), e_alice))Otherwise, the test correctly validates the PR's objective of removing a specific relationship by reference identity. The assertions appropriately verify that only the targeted relationship is removed while others remain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
addons/gecs/ecs/entity.gd(1 hunks)addons/gecs/tests/core/test_relationships.gd(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- addons/gecs/ecs/entity.gd
🧰 Additional context used
📓 Path-based instructions (1)
**/*.gd
📄 CodeRabbit inference engine (CLAUDE.md)
Always use the res:// prefix for file paths in Godot scripts and tests
Files:
addons/gecs/tests/core/test_relationships.gd
Can we implement this suggestion I like this here if it works this way we only iterate once |
|
yes. i had made more commits. it's similar to that suggestion. |
|
Thanks! Looks good! |
2 participants
remove specific one relationship that you get from an entity. firstly, do it by checking if the relationship references are the identical, not if the objects are the same.
if the relationship isn't the inner object within entity, then use condition-match to take off similar relationships.
Summary by CodeRabbit
Bug Fixes
Tests