Conversation
interim17
changed the title
| ); | ||
| return; | ||
| } | ||
| this.agentPathGroup.remove(path.line); |
There was a problem hiding this comment.
what's the difference between agentPathGroup and agentPaths?
There was a problem hiding this comment.
Seems like the agentPaths are the line data and the group paths are the threeJS objects associated with that data?
There was a problem hiding this comment.
agentPathGroup is a threejs Group object which is just a parent node for all the paths to belong to. agentPaths is a mapping of agent id to path, for all current agents that are drawing paths.
toloudis
left a comment
toloudis
left a comment
There was a problem hiding this comment.
LGTM! the paths were never being removed from their Group!
interim17
requested review from
ShrimpCryptid,
Copilot and
meganrm
and removed request for
a team,
ShrimpCryptid and
meganrm
There was a problem hiding this comment.
Pull request overview
Fixes a bug in agent path removal where paths were being deleted from the data structure without properly removing their visual representations from the scene graph.
- Adds explicit removal of path lines from the agent path group before deleting path data
- Ensures proper cleanup in both single path removal and bulk path removal operations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.agentPaths.forEach((path) => { | ||
| this.agentPathGroup.remove(path.line); | ||
| }); |
There was a problem hiding this comment.
[nitpick] Consider iterating with for...of instead of forEach for better performance when removing multiple paths, as it avoids function call overhead for each iteration.
| this.agentPaths.forEach((path) => { | |
| this.agentPathGroup.remove(path.line); | |
| }); | |
| for (const path of this.agentPaths.values()) { | |
| this.agentPathGroup.remove(path.line); | |
| } |
Time Estimate or Size
Closes #364
Problem
What is the problem this work solves, including
Link to story or ticket
Solution
What I/we did to solve this problem
with @pairperson1
Type of change
Please delete options that are not relevant.
Change summary:
Steps to Verify:
Screenshots (optional):
Show-n-tell images/animations here
Keyfiles (delete if not relevant):
Thanks for contributing!