Integrate IActuators into ML-Agents core code. #4315
Conversation
com.unity.ml-agents/Runtime/Agent.cs
Outdated
|
|
||
| /// <summary> | ||
| /// DiscreteVectorActuator which is used by default if no other sensors exist on this Agent. This VectorSensor will | ||
| /// delegate its actions to <see cref="IActionReceiver.OnActionReceived"/> by default in order to keep backward compatibility |
There was a problem hiding this comment.
Maybe I need to read more, but it sends to Agent.OnActionReceived, right?
There was a problem hiding this comment.
Yeah, Agent Implements that interface method.
There was a problem hiding this comment.
oh, I misunderstood this comment. I'll fix it.
Co-authored-by: Chris Elion <[email protected]>
…g the action mask in Agent.
| @@ -15,19 +14,13 @@ namespace Unity.MLAgents | |||
| /// may be illegal. For example, if an agent is adjacent to a wall or other obstacle | |||
| /// you could mask any actions that direct the agent to move into the blocked space. | |||
| /// </remarks> | |||
| public class DiscreteActionMasker | |||
| public class DiscreteActionMasker : IDiscreteActionMask | |||
There was a problem hiding this comment.
Just checking - this isn't considered a breaking change, right? I've seen some chatter about things the look harmless because the old code still compiles, but the assemblies would be incompatible.
There was a problem hiding this comment.
good question. I wonder if we can check the version bump with the validation suite somehow
There was a problem hiding this comment.
My intuition would be that since the default behavior didn't change, and the method's signature that uses this type is concretely defined, therefore, it should be ok. Would be good to know the answer to this question concretely.
| @@ -45,6 +49,7 @@ internal class BarracudaPolicy : IPolicy | |||
| { | |||
| var modelRunner = Academy.Instance.GetOrCreateModelRunner(model, brainParameters, inferenceDevice); | |||
| m_ModelRunner = modelRunner; | |||
| m_SapceType = brainParameters.VectorActionSpaceType; | |||
There was a problem hiding this comment.
- typo: m_SapceType -> m_SpaceType
- Should we be dealing in ActionSpecs here instead? Would adding a method to BrainParameters to return an equivalent ActionSpec be helpful anywhere else?
There was a problem hiding this comment.
Could be used in creating the vector actuator.
Proposed change(s)
This PR is an attempt to integrate Actuators into the core of ML-Agents code without breaking backward compatibility.
The biggest architectural change (aside from the Actuator abstraction) is splitting the action buffer between the types of actions that are available for use. This propagates all the way down to the IPolicy interface and makes for some unflattering hoop-jumping in order to keep Heuristic backward compatible.
Two follow up PRs will be made:
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
Types of change(s)
Checklist
Other comments