Separate boxoverlap out of GridSensor #5238
Conversation
| /// Whether to use one-hot detected tag as observation. | ||
| /// Note that changing this after the sensor is created has no effect. | ||
| /// </summary> | ||
| public bool UseOneHotTag |
There was a problem hiding this comment.
I can make UseOneHotTag and CountColliders into a GridSensorObservationType enum if that's better.
| ); | ||
| // debug data is positive int value and will trigger data validation exception if SensorCompressionType is not None. | ||
| m_DebugSensor = new GridSensorBase("DebugGridSensor", m_CellScale, m_GridSize, new int[] { 1 }, m_DetectableObjects, SensorCompressionType.None); |
There was a problem hiding this comment.
Maybe lazily allocate this in OnDrawGizmos()?
There was a problem hiding this comment.
That will cause several issues. When there's any changes on component, the debug sensor has to be updated too. So if we turn on gizmo -> turn it off -> change some config -> and turn it back on, it will not be updated.
Also decoupling the creation of DebugSensor and BoxOverlapChecker cause errors in editor. m_DebugSensor will attach to the existing BoxOverlapChecker at creation time, but editor refresh will keep re-creating new BoxOverlapChecker and the debug sensor won't know about it.
I think in terms of performance it's not causing too much trouble since when showGizmo=false it doesn't trigger any unnecessary calls.
com.unity.ml-agents.extensions/Runtime/Sensors/GridSensorBase.cs
Outdated
Show resolved
Hide resolved
com.unity.ml-agents.extensions/Runtime/Sensors/GridSensorBase.cs
Outdated
Show resolved
Hide resolved
| /// </summary> | ||
| /// <param name="currentColliderGo">The game object that was found colliding with a certain cell</param> | ||
| /// <param name="cellIndex">The index of the current cell</param> | ||
| protected internal void LoadObjectData(GameObject currentColliderGo, int cellIndex) |
There was a problem hiding this comment.
Rename to ProcessCellOverlaps()?
There was a problem hiding this comment.
Renamed to ProcessDetectedObject since it processes one object
| { | ||
| if (!ReferenceEquals(currentColliderGo, null) && currentColliderGo.CompareTag(m_DetectableObjects[i])) | ||
| { | ||
| Array.Clear(m_CellDataBuffer, 0, m_CellDataBuffer.Length); |
There was a problem hiding this comment.
This feels like a a good usecase for an ArraySegment that's a "view' on the part of m_CellDataBuffer that we're able to write to. Now that we're on 2019.4, we might be able to use a new enough version of C# that we can write, otherwise we could roll our own small version
com.unity.ml-agents.extensions/Runtime/Sensors/GridSensorBase.cs
Outdated
Show resolved
Hide resolved
com.unity.ml-agents.extensions/Runtime/Sensors/GridSensorComponent.cs
Outdated
Show resolved
Hide resolved
|
|
||
| [HideInInspector, SerializeField] | ||
| internal string[] m_DetectableObjects; | ||
| internal string[] m_DetectableTags; |
| m_DebugSensor = new GridSensorBase("DebugGridSensor", m_CellScale, m_GridSize, m_DetectableTags, SensorCompressionType.None); | ||
| m_BoxOverlapChecker.RegisterDebugSensor(m_DebugSensor); | ||
|
|
||
| if (m_UseOneHotTag) |
There was a problem hiding this comment.
Move the creation of OneHotGridSensor and CountingGridSensor to the default impl of GetGridSensors?
There was a problem hiding this comment.
I can see how this make sense, but then when GetGridSensors is overridden the bool m_UseOneHotTag specified in UI will have no effect which is a bit confusing.
There was a problem hiding this comment.
If you override GetGridSensors(), you can still call base.GetGridSensors() (or not)
There was a problem hiding this comment.
ok, I moved it.
| } | ||
| } | ||
| // Only one sensor needs to reference the boxOverlapChecker, so that it gets updated exactly once | ||
| ((GridSensorBase)m_Sensors[0]).m_BoxOverlapChecker = m_BoxOverlapChecker; |
There was a problem hiding this comment.
m_Sensors will be empty if both m_UseOneHotTag and m_CountColliders are false, right? I think you need to check m_Sensors.Count first.
There was a problem hiding this comment.
Fixed it. Also added tests for this.
|
|
||
| internal void RegisterSensor(GridSensorBase sensor) | ||
| { | ||
| if (sensor.ProcessAllCollidersInCell()) |
There was a problem hiding this comment.
nit: instead of bool ProcessAllCollidersInCell() and two separate registration methods, could you make the GridSensorBase return an enum (ProcessAllColliders, ProcessClosestCollider, DebugColliders) ? I think that would simplify this a bit.
There was a problem hiding this comment.
I made two separate registration methods because I don't want to expose DebugColliders since user should never use that.
I can make enum for ProcessAllColliders and ProcessClosestCollider.
Proposed change(s)
Separate BoxOverlap logic into separate class, and also make each encoding into a class derived from GridSensorBase.
The component holds a list of GridSensors, and the first one has the BoxOverlapChecker.
The component also has a dummy GridSensorBase which will not be passed to Agent, only for collecting debug gizmo data.
For each cell, the data collection process is as following:
Derived GridSensors share 1 and 2, and only overwrites 3.
1 only runs once for each cell.
2 runs once for each way of parsing colliders. This allows the parsed colliders being reused by different sensors. For example, there's a one-hot sensor and a continuous value sensor both using one collider per cell. Then ParseColliders only has to compute closest collider once.
3 runs once for each sensor and loads different data for different sensor.
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
Types of change(s)
Checklist
Other comments