-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Add warning to academy docs #4226
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
Conversation
docs/Learning-Environment-Design.md
Outdated
|
||
In some games, it may be desirable to step the environment with the Academy manually via `Academy.Instance.EnvironmentStep()`. | ||
If this is the case, be sure `Academy.Instance.EnvironmentStep()` is not called directly or indirectly by the agent's | ||
`CollectObservations()` function as this will cause an infinite loop that prevents that main Academy update function from being called. |
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.
CollectObservations is not the only method that might pose problems. Anything called BY the EnvironmentStep will be problematic. For example: Heuristic and OnActionReceived.
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.
Would it be better to guard against this? We could do something like
public void EnvironmentStep() {
if(m_Stepping) {
throw new UnityException("Infinite recursion prevented!");
}
m_Stepping = true;
// do all the work
m_Stepping = false;
}
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.
That works for me. It's up to you 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.
(with maybe a try/finally around the main body to reset the stepping flag, and/or change the throw to just return)
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.
@vincentpierre thoughts?
Co-authored-by: Vincent-Pierre BERGES <[email protected]>
Closing because this was made obsolete by #4227 |
Proposed change(s)
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
#4220
Types of change(s)
Checklist
Other comments