Fix attack no detach #373
Fix attack no detach #373
Conversation
PR MathCancer#340 put the correct check in the wrong place to prevent detachment when a cell is attacking another cell. This fixes that by first moving the check into the spring attachments function as an attack produces a spring attachment. Second, it moves the check into the detachment block rather than the attachment block.
- replace while loop with for loop and a break statement
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where random detachment was incorrectly handled during an attack and simplifies the code for managing dynamic attachments.
- Fixes the detachment logic in spring attachments to correctly prevent detachment during an attack.
- Refactors attachment handling in both dynamic_attachments and dynamic_spring_attachments to use range-based loops for improved clarity.
| for( auto pTest : pCell->state.attached_cells ) | ||
| { | ||
| if( UniformRandom() <= detachment_probability ) | ||
| { detach_cells( pCell , pTest ); } | ||
| } |
There was a problem hiding this comment.
Iterating over 'attached_cells' with a range-based loop may cause iterator invalidation if detach_cells modifies the container. Consider iterating over a copy of the container or using a safe iteration method.
| for( auto pTest : pCell->state.spring_attachments ) | ||
| { | ||
| if( phenotype.cell_interactions.pAttackTarget==pTest || pTest->phenotype.cell_interactions.pAttackTarget==pCell ) // do not let attackers detach randomly | ||
| { continue; } | ||
| if( UniformRandom() <= detachment_probability ) | ||
| { detach_cells_as_spring( pCell , pTest ); } |
There was a problem hiding this comment.
Iterating over 'spring_attachments' using a range-based loop may result in undefined behavior if detach_cells_as_spring modifies the container. A safer iteration strategy should be used to avoid potential iterator invalidation.
| { | ||
| Cell* pTest = pCell->state.neighbors[j]; | ||
| if (phenotype.cell_interactions.pAttackTarget==pTest || pTest->phenotype.cell_interactions.pAttackTarget==pCell) // do not let attackers detach randomly | ||
| { continue; } |
There was a problem hiding this comment.
this if block is the wrong fix from #340. It is in the dynamic_attachment function but attacks result in spring attachments. Furthermore, the logic is applied to prevent pTest and pCell from attaching again rather than from preventing them from spontaneously detaching.
| for( auto pTest : pCell->state.spring_attachments ) | ||
| { | ||
| if( phenotype.cell_interactions.pAttackTarget==pTest || pTest->phenotype.cell_interactions.pAttackTarget==pCell ) // do not let attackers detach randomly | ||
| { continue; } |
There was a problem hiding this comment.
Here's the corrected check to ensure that spring attachments to not spontaneously detach when they were formed for attacks.
1 participant
In PR #340, the fix targeted the wrong function for disallowing random detachment during an attack. Not sure how I made that mistake, but now it's fixed. As of opening this PR, there are two commits:
dynamic_attachmentsanddynamic_spring_attachmentsso that the code is more straightforwardIf the code refactor is not desired, I can roll back that commit easily enough.
To prove this has fixed the issue, here is a movie from the model provided in #340 in which the spring attachment persists throughout the attack duration from the updated code despite the high detachment rate.
fix_attack_detach.mp4