Manage CameraSensors in CameraSensorComponent - #6324
Conversation
Sorry, something went wrong.
…single one. For RenderTextureSensorComponent, have a list of RenderTextureSensors instead of a single one.
|
/review |
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Harness Review
The change correctly gives concurrently initialized agents distinct camera and render-texture sensors, but the new component-owned collections are not reconciled with the agent lifecycle.
Reviewed commit 0421fc0
🤖 Helpful? 👍/👎
Sorry, something went wrong.
| Dispose(); | ||
| m_Sensor = new CameraSensor(m_Camera, m_Width, m_Height, Grayscale, m_SensorName, m_Compression, m_ObservationType); | ||
| var sensor = new CameraSensor(m_Camera, m_Width, m_Height, Grayscale, m_SensorName, m_Compression, m_ObservationType); | ||
| m_Sensors.Add(sensor); |
There was a problem hiding this comment.
When an agent is disabled, Agent.OnDisable() disposes the ISensor instances it received but never calls SensorComponent.Dispose(). Re-enabling that agent initializes it again and calls CreateSensors(), so this append retains the already-disposed sensor indefinitely; repeated enable/disable cycles grow this list (and make every runtime property update iterate stale entries). The same issue exists in RenderTextureSensorComponent. Deregister each sensor when its owning agent disposes it, or otherwise prune disposed entries without disposing sensors still owned by other agents.
🤖 Helpful? 👍/👎
Sorry, something went wrong.
|
/review |
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Harness Review
The change now gives each agent its own visual sensor and disposes stacked wrappers so the wrapped texture is released during agent cleanup. I examined the creation, cleanup, stacking, and inspector-temporary-sensor paths and found no new actionable issues beyond the previously reported finding.
Reviewed commit 6ccc12c
🤖 Helpful? 👍/👎
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Harness Review
The change now gives each agent its own camera or render-texture sensor and ensures stacked wrappers release their owned concrete sensors. I examined the agent cleanup/reinitialization path, component update and disposal behavior, inspector-created sensor cleanup, and other stacking call sites; no new actionable defect was identified beyond the previously reported issue.
Reviewed commit 2f0c204
🤖 Helpful? 👍/👎
Sorry, something went wrong.
Codengineer
left a comment
There was a problem hiding this comment.
This is great! Please address UPR issue. We need to make sure no memory leaks can be introduced by any path and that everything is properly disposed.
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
Proposed change(s)
For CameraSensorComponent, have a list of CameraSensors instead of a single one.
For RenderTextureSensorComponent, have a list of RenderTextureSensors instead of a single one.
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
Fixes #6320
Types of change(s)
Checklist