-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make On Demand Decision the default #3243
Conversation
public void Awake() | ||
{ | ||
m_Agent = gameObject.GetComponent<Agent>(); | ||
Academy.Instance.AgentSetStatus += MakeRequests; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rename Academy.AgentSetStatus too? Not really sure what makes sense for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also remove it and use fixed update in the decision requester. It will change the logic slightly but would simplify greatly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like it better coming from the Academy, especially since users might not be stepping the Academy each FixedUpdate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about that, for Physics based environments, it is better to be synced with the Physics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general that's true. But we shouldn't force that if the users knows (or thinks they know) better than us; that's why there's an option to disable automatic stepping on the Academy.
Maybe we should just clarify in the tooltips what "step" means here - "The agent will automatically request a decision every X Academy steps." and "Whether or not AgentAction will be called on Academy steps..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @chriselion here. We should drive the ticking through our systems, rather than have users subscribe to random ticking loops.
} | ||
void MakeRequests(int count) | ||
{ | ||
if (count % DecisionPeriod == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either add a Range
or Min
attribute to DecisionPeriod, or take Mathf.max(DecisionPeriod, 1)
(or both)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's "new" functionality, but I really think the ability to stagger which agents request each step would be really useful for developers with lots of agents. Otherwise you get large spikes every DecisionPeriod steps.
I think either something like
public int offset = 0
...
if ((count + offset) % DecisionPeriod == 0)
or
public bool offsetStep = True;
...
var offset = offsetStep ? m_Agent.GetId() : 0; // or something similar
if ((count + offset) % DecisionPeriod == 0)
The latter is probably more useful with prefabs since it doesn't require setting a different value on each.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's "new" functionality, but I really think the ability to stagger which agents request each step would be really useful for developers with lots of agents. Otherwise you get large spikes every DecisionPeriod steps.
This is a good candidate for thinking of jobification of ML-Agents. Not for 1.0, but later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
No description provided.