Skip to content
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

Don't clamp timescale to 100 outside of editor #4867

Merged
merged 2 commits into from
Jan 19, 2021

Conversation

chriselion
Copy link
Contributor

@chriselion chriselion commented Jan 16, 2021

Proposed change(s)

Remove the upper limit of 100 for the timescale when using the player. We can't remove the limit entirely, since the Editor still requires it.

Note that having a lower bound of 1 is still worthwhile, so we clamp to [1, Infinity] in the player for simplicity.

Also made a SideChannel tests directory, since I have another one to add in another PR.

Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)

https://jira.unity3d.com/browse/MLA-748
First noted in #3596
Editor code

Types of change(s)

  • QoL improvement

Checklist

  • Added tests that prove my fix is effective or that my feature works
  • Updated the changelog (if applicable)

#else
const cloat maxTimeScale = float.PositiveInfinity;
#endif
timeScale = Mathf.Clamp(timeScale, 1, maxTimeScale);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could there be cases where someone wants to train at timescales between (0,1]?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and do we want to support that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't imagine for training, but there could hypothetically be an inference use-case where someone might want it to go slower to better inspect the agent behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block of code only affects timescale information coming from python (generally mlagents-learn commandline, but could be the yaml config or low-level API too), so inference isn't affected by this.

So far there haven't been any requests for timescale < 1, but we can change the lower bound to, say, 0.01.

Copy link
Contributor

@awjuliani awjuliani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I wasn't aware of the 100 limit in the editor, but I imagine it was kept in place to prevent locking up the editor when in play mode.

@awjuliani
Copy link
Contributor

I wonder if we have an environment which would benefit from the higher timescale. It would likely have to have neither physics nor rendering, so perhaps Match3?

@chriselion
Copy link
Contributor Author

I wonder if we have an environment which would benefit from the higher timescale. It would likely have to have neither physics nor rendering, so perhaps Match3?

AFAIK we don't override the defaults for training in any scene.

I'm not sure Match3 would benefit since it doesn't have any physics or animation; moves are requested on every step.

@chriselion chriselion merged commit c05e1a1 into master Jan 19, 2021
@delete-merged-branch delete-merged-branch bot deleted the MLA-748-timeScale-clamping branch January 19, 2021 20:00
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants