Don't inherit from Academy, remove virtual methods #3184

chriselion · 2020-01-08T21:07:47Z

Make Academy non-abstract, and replace all child classes of it.

Remove all virtual methods of Academy.
All scenes now use an Academy instead of the child class
Some scenes used their Academy to store things like agent speeds or materials. These were converted to *Settings.cs files instead.
ProjectSettingsOverrides handles exposing gravity as a reset parameter, as well as a few advanced physics settings used by Walker and Crawler scenes, and resets these values when stopped. This was added to most of the scenes (not everything had an override for gravity)
Remove the saved gravity and framerate variables from Academy.

Suggested replacements for the Academy virtual methods:

InitializeAcademy -> MonoBehaviour.OnAwake
AcademyStep -> MonoBehaviour.Update or FixedUpdate
AcademyReset -> subscribe to Academy.OnEnvironmentReset
OnDestroy -> MonoBehaviour.OnDestroy or subscribe to Academy.DestroyAction

UnitySDK/Assets/ML-Agents/Examples/Crawler/Scripts/CrawlerAcademy.cs

chriselion · 2020-01-08T21:09:20Z

UnitySDK/Assets/ML-Agents/Examples/SharedAssets/Scripts/SettingsOverrides.cs

+
+        // Override
+        Physics.gravity *= gravityMultiplier;
+        Time.fixedDeltaTime = fixedDeltaTime;


Most scenes only care about gravity, but Crawler and Walker override these deltaTime and solver iterations settings.

chriselion · 2020-01-08T21:09:37Z

UnitySDK/Assets/ML-Agents/Examples/SharedAssets/Scripts/SettingsOverrides.cs

+        Physics.defaultSolverIterations = solverIterations;
+        Physics.defaultSolverVelocityIterations = solverVelocityIterations;
+
+        var academy = FindObjectOfType<Academy>();


Will eventually be replaced with singleton access.

vincentpierre

I like it a lot so far. (Not approving only because it is WIP)

vincentpierre · 2020-01-08T21:19:43Z

UnitySDK/Assets/ML-Agents/Examples/Crawler/Scripts/CrawlerAcademy.cs

@@ -8,7 +8,7 @@ public override void InitializeAcademy()
        Monitor.verticalOffset = 1f;
        Physics.defaultSolverIterations = 12;
        Physics.defaultSolverVelocityIterations = 12;
-        Time.fixedDeltaTime = 0.01333f; // (75fps). default is .2 (60fps)
+        Time.fixedDeltaTime = 0.01333f; // (75fps). default is .02 (60fps)


<opinion>
I do think we should avoid changing those in our environments.
</opinion>

I don't know the history of them; since the scenes that override them are using ragdolls, I assume it was to improve stability. It might all change with the physx changes anyway.

UnitySDK/Assets/ML-Agents/Examples/SharedAssets/Scripts/SettingsOverrides.cs

vincentpierre · 2020-01-08T21:23:44Z

UnitySDK/Assets/ML-Agents/Examples/SharedAssets/Scripts/SettingsOverrides.cs

+    int m_OriginalSolverIterations;
+    int m_OriginalSolverVelocityIterations;
+
+    public float gravityMultiplier = 1.0f;


Not sure if we should have gravity multiplier or gravity setting (with default at -9.81)

All of the scenes today use a multiplier (but FloatPropertiesChannel use the negative of the value)

UnitySDK/Assets/ML-Agents/Examples/Walker/Scripts/WalkerAcademy.cs

vincentpierre · 2020-01-08T21:25:10Z

UnitySDK/Assets/ML-Agents/Examples/SharedAssets/Scripts/SettingsOverrides.cs

@@ -0,0 +1,52 @@
+using UnityEngine;


Is SimulationSettingsOverrides more appropriate?

This is directly modifying what unity calls ProjectSettings, maybe ProjectSettingsOverrides?

That works for me too. Since this will only be used in our scenes, I'm less worried about the naming or making sure that it handles all possible cases.

UnitySDK/Assets/ML-Agents/Examples/SharedAssets/Scripts/SettingsOverrides.cs

surfnerd · 2020-01-08T21:46:00Z

Seems ok to me.

chriselion · 2020-01-09T00:35:43Z

Diffs aren't very useful, so here's how Soccer "Academy" GameObject looks now

UnitySDK/Assets/ML-Agents/Scripts/Academy.cs

chriselion · 2020-01-09T22:31:20Z

UnitySDK/Assets/ML-Agents/Editor/Tests/MLAgentsEditModeTest.cs

            Assert.AreEqual(0, aca.GetStepCount());
            Assert.AreEqual(0, aca.GetEpisodeCount());
+            Assert.AreEqual(0, aca.GetTotalStepCount());
+            Assert.AreEqual(null, aca.FloatProperties);


Suggestions for other things to check before and after initialization?

chriselion · 2020-01-09T22:36:51Z

UnitySDK/Assets/ML-Agents/Scripts/Academy.cs

@@ -39,23 +45,11 @@ namespace MLAgents
    /// </remarks>
    [HelpURL("https://github.com/Unity-Technologies/ml-agents/blob/master/" +
        "docs/Learning-Environment-Design-Academy.md")]
-    public abstract class Academy : MonoBehaviour
+    public class Academy : MonoBehaviour


should we make this sealed or is that overkill?

I think it would clearly communicate that the Academy is not to be subclassed anymore. It makes sense to add sealed to me.

Doh, forgot to add this. If for some reason, I don't get the singleton implemented before the next release, I'll seal it for that.

vincentpierre

Looks like a no brainer to me

surfnerd · 2020-01-09T23:11:06Z

UnitySDK/Assets/ML-Agents/Examples/SharedAssets/Scripts/ProjectSettingsOverrides.cs

+        var academy = FindObjectOfType<Academy>();
+        academy.LazyInitialization();
+
+        academy.FloatProperties.RegisterCallback("gravity", f => { Physics.gravity = new Vector3(0, -f, 0); });


Looks like there's no UnRegisterCallback on FloatProperties at the moment. This could be an issue when Academy is a singleton and scenes are switched.

…erride

MikeWise2718 · 2020-03-07T17:36:37Z

So if I have something I need to do every time there is an Academy Step, should I be using Settings.Update or Settings.FixedUpdate? The design seems to indicate both of them are okay, but they are not really equivalent, so I am not sure what to do.

Hmm, looking at the old code, AcademyStep was called by EnvironmentStep which was called by FixedUpdate, so I am going with that.

Chris Elion added 2 commits January 8, 2020 12:36

WIP settings override for scenes

c241b97

fix default values

fb9cef9

chriselion commented Jan 8, 2020

View reviewed changes

UnitySDK/Assets/ML-Agents/Examples/Crawler/Scripts/CrawlerAcademy.cs Show resolved Hide resolved

chriselion commented Jan 8, 2020

View reviewed changes

vincentpierre reviewed Jan 8, 2020

View reviewed changes

chriselion commented Jan 8, 2020

View reviewed changes

UnitySDK/Assets/ML-Agents/Examples/SharedAssets/Scripts/SettingsOverrides.cs Outdated Show resolved Hide resolved

Chris Elion added 15 commits January 8, 2020 14:12

redo 3dball scene

abee213

convert Basic scene

5e21640

convert Bouncer

ad556a8

convert Bouncer

d6b3718

add Monitor offset, convert Crawler

45cee86

convert pyramids

44c37a5

update Reacher and Tennis

f6889d7

commit walker

25627b4

Convert WallJump

de8be79

convert pushblock

d6b8b1e

convert Hallway

24afce2

fix hallway

0905dea

convert GridWorld

d3760a6

fix HallwaySettings

465ddfd

convert soccer

c6f74f1

Chris Elion added 3 commits January 8, 2020 16:37

remove TemplateAcademy

2cc0d96

tooltips and remove saved settings from Academy

a241222

cleanup

4ee978c

chriselion commented Jan 9, 2020

View reviewed changes

UnitySDK/Assets/ML-Agents/Scripts/Academy.cs Show resolved Hide resolved

chriselion changed the title ~~WIP settings override for scenes~~ Don't inherit from Academy Jan 9, 2020

EnvReset delegate, remove virtual methods, convert FoodCollector

4559afa

chriselion commented Jan 9, 2020

View reviewed changes

forgot to add FoodCollectorSettings.cs

75243e0

chriselion commented Jan 9, 2020

View reviewed changes

chriselion changed the title ~~Don't inherit from Academy~~ Don't inherit from Academy, remove virtual methods Jan 9, 2020

fix usings

839794a

vincentpierre approved these changes Jan 9, 2020

View reviewed changes

surfnerd reviewed Jan 9, 2020

View reviewed changes

surfnerd approved these changes Jan 9, 2020

View reviewed changes

Chris Elion added 2 commits January 10, 2020 10:55

Doc changes for making Academy non-virtual (#3195)

61a9548

Merge remote-tracking branch 'origin/master' into develop-settings-ov…

eb7c0d3

…erride

chriselion merged commit f389e85 into master Jan 10, 2020

delete-merged-branch bot deleted the develop-settings-override branch January 10, 2020 19:26

github-actions bot locked as resolved and limited conversation to collaborators May 15, 2021

Don't inherit from Academy, remove virtual methods #3184

Don't inherit from Academy, remove virtual methods #3184

Uh oh!

Conversation

chriselion commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentpierre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

surfnerd commented Jan 8, 2020

Uh oh!

chriselion commented Jan 9, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentpierre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeWise2718 commented Mar 7, 2020

Uh oh!

Uh oh!

chriselion commented Jan 8, 2020 •

edited

Loading