Academy interface cleanup (#3376)

Chris Elion · web-flow · commit 1d2399456695 · 2020-02-07T13:31:43.000-08:00
* Academy interface cleanup

* fix tests

* fix destroyImmediate use in test

* use isEditor to switch between Destroy and DestroyImmediate
diff --git a/com.unity.ml-agents/Runtime/Academy.cs b/com.unity.ml-agents/Runtime/Academy.cs
@@ -148,14 +148,14 @@ public bool IsCommunicatorOn
         {
             Application.quitting += Dispose;
 
-            LazyInitialization();
+            LazyInitialize();
         }
 
         /// <summary>
         /// Initialize the Academy if it hasn't already been initialized.
         /// This method is always safe to call; it will have no effect if the Academy is already initialized.
         /// </summary>
-        internal void LazyInitialization()
+        internal void LazyInitialize()
         {
             if (!m_Initialized)
             {
@@ -168,7 +168,7 @@ internal void LazyInitialization()
         /// Enable stepping of the Academy during the FixedUpdate phase.  This is done by creating a temporary
         /// GameObject with a MonoBehavior that calls Academy.EnvironmentStep().
         /// </summary>
-        public void EnableAutomaticStepping()
+        void EnableAutomaticStepping()
         {
             if (m_FixedUpdateStepper != null)
             {
@@ -185,15 +185,15 @@ public void EnableAutomaticStepping()
         /// Disable stepping of the Academy during the FixedUpdate phase. If this is called, the Academy must be
         /// stepped manually by the user by calling Academy.EnvironmentStep().
         /// </summary>
-        public void DisableAutomaticStepping(bool destroyImmediate = false)
+        void DisableAutomaticStepping()
         {
             if (m_FixedUpdateStepper == null)
             {
                 return;
             }
 
             m_FixedUpdateStepper = null;
-            if (destroyImmediate)
+            if (Application.isEditor)
             {
                 UnityEngine.Object.DestroyImmediate(m_StepperObject);
             }
@@ -206,11 +206,21 @@ public void DisableAutomaticStepping(bool destroyImmediate = false)
         }
 
         /// <summary>
-        /// Returns whether or not the Academy is automatically stepped during the FixedUpdate phase.
+        /// Determines whether or not the Academy is automatically stepped during the FixedUpdate phase.
         /// </summary>
-        public bool IsAutomaticSteppingEnabled
+        public bool AutomaticSteppingEnabled
         {
             get { return m_FixedUpdateStepper != null; }
+            set {
+                if (value)
+                {
+                    EnableAutomaticStepping();
+                }
+                else
+                {
+                    DisableAutomaticStepping();
+                }
+            }
         }
 
         // Used to read Python-provided environment parameters
@@ -335,9 +345,9 @@ void OnResetCommand()
         /// <returns>
         /// Current episode number.
         /// </returns>
-        public int GetEpisodeCount()
+        public int EpisodeCount
         {
-            return m_EpisodeCount;
+            get { return m_EpisodeCount; }
         }
 
         /// <summary>
@@ -346,9 +356,9 @@ public int GetEpisodeCount()
         /// <returns>
         /// Current step count.
         /// </returns>
-        public int GetStepCount()
+        public int StepCount
         {
-            return m_StepCount;
+            get { return m_StepCount; }
         }
 
         /// <summary>
@@ -357,9 +367,9 @@ public int GetStepCount()
         /// <returns>
         /// Total step count.
         /// </returns>
-        public int GetTotalStepCount()
+        public int TotalStepCount
         {
-            return m_TotalStepCount;
+            get { return m_TotalStepCount; }
         }
 
         /// <summary>
@@ -445,7 +455,7 @@ internal ModelRunner GetOrCreateModelRunner(
         /// </summary>
         public void Dispose()
         {
-            DisableAutomaticStepping(true);
+            DisableAutomaticStepping();
             // Signal to listeners that the academy is being destroyed now
             DestroyAction?.Invoke();
 
diff --git a/com.unity.ml-agents/Tests/Editor/MLAgentsEditModeTest.cs b/com.unity.ml-agents/Tests/Editor/MLAgentsEditModeTest.cs
@@ -128,9 +128,9 @@ public void TestAcademy()
         {
             var aca = Academy.Instance;
             Assert.AreNotEqual(null, aca);
-            Assert.AreEqual(0, aca.GetEpisodeCount());
-            Assert.AreEqual(0, aca.GetStepCount());
-            Assert.AreEqual(0, aca.GetTotalStepCount());
+            Assert.AreEqual(0, aca.EpisodeCount);
+            Assert.AreEqual(0, aca.StepCount);
+            Assert.AreEqual(0, aca.TotalStepCount);
         }
 
         [Test]
@@ -164,12 +164,12 @@ public void TestAcademy()
             Assert.AreEqual(true, Academy.IsInitialized);
 
             // Check that init is idempotent
-            aca.LazyInitialization();
-            aca.LazyInitialization();
+            aca.LazyInitialize();
+            aca.LazyInitialize();
 
-            Assert.AreEqual(0, aca.GetEpisodeCount());
-            Assert.AreEqual(0, aca.GetStepCount());
-            Assert.AreEqual(0, aca.GetTotalStepCount());
+            Assert.AreEqual(0, aca.EpisodeCount);
+            Assert.AreEqual(0, aca.StepCount);
+            Assert.AreEqual(0, aca.TotalStepCount);
             Assert.AreNotEqual(null, aca.FloatProperties);
 
             // Check that Dispose is idempotent
@@ -246,8 +246,8 @@ public void TestAcademy()
             var numberReset = 0;
             for (var i = 0; i < 10; i++)
             {
-                Assert.AreEqual(numberReset, aca.GetEpisodeCount());
-                Assert.AreEqual(i, aca.GetStepCount());
+                Assert.AreEqual(numberReset, aca.EpisodeCount);
+                Assert.AreEqual(i, aca.StepCount);
 
                 // The reset happens at the beginning of the first step
                 if (i == 0)
@@ -262,11 +262,11 @@ public void TestAcademy()
         public void TestAcademyAutostep()
         {
             var aca = Academy.Instance;
-            Assert.IsTrue(aca.IsAutomaticSteppingEnabled);
-            aca.DisableAutomaticStepping(true);
-            Assert.IsFalse(aca.IsAutomaticSteppingEnabled);
-            aca.EnableAutomaticStepping();
-            Assert.IsTrue(aca.IsAutomaticSteppingEnabled);
+            Assert.IsTrue(aca.AutomaticSteppingEnabled);
+            aca.AutomaticSteppingEnabled = false;
+            Assert.IsFalse(aca.AutomaticSteppingEnabled);
+            aca.AutomaticSteppingEnabled = true;
+            Assert.IsTrue(aca.AutomaticSteppingEnabled);
         }
 
         [Test]
@@ -357,9 +357,9 @@ public void TestAcademy()
             var stepsSinceReset = 0;
             for (var i = 0; i < 50; i++)
             {
-                Assert.AreEqual(stepsSinceReset, aca.GetStepCount());
-                Assert.AreEqual(numberReset, aca.GetEpisodeCount());
-                Assert.AreEqual(i, aca.GetTotalStepCount());
+                Assert.AreEqual(stepsSinceReset, aca.StepCount);
+                Assert.AreEqual(numberReset, aca.EpisodeCount);
+                Assert.AreEqual(i, aca.TotalStepCount);
                 // Academy resets at the first step
                 if (i == 0)
                 {
@@ -395,10 +395,10 @@ public void TestAgent()
             var agent2StepSinceReset = 0;
             for (var i = 0; i < 5000; i++)
             {
-                Assert.AreEqual(acaStepsSinceReset, aca.GetStepCount());
-                Assert.AreEqual(numberAcaReset, aca.GetEpisodeCount());
+                Assert.AreEqual(acaStepsSinceReset, aca.StepCount);
+                Assert.AreEqual(numberAcaReset, aca.EpisodeCount);
 
-                Assert.AreEqual(i, aca.GetTotalStepCount());
+                Assert.AreEqual(i, aca.TotalStepCount);
 
                 Assert.AreEqual(agent2StepSinceReset, agent2.GetStepCount());
                 Assert.AreEqual(numberAgent1Reset, agent1.agentResetCalls);

Original file line number	Diff line number	Diff line change
`@@ -148,14 +148,14 @@ public bool IsCommunicatorOn`
`148`	`148`	`{`
`149`	`149`	`Application.quitting += Dispose;`
`150`	`150`
`151`		`- LazyInitialization();`
	`151`	`+ LazyInitialize();`
`152`	`152`	`}`
`153`	`153`
`154`	`154`	`/// <summary>`
`155`	`155`	`/// Initialize the Academy if it hasn't already been initialized.`
`156`	`156`	`/// This method is always safe to call; it will have no effect if the Academy is already initialized.`
`157`	`157`	`/// </summary>`
`158`		`- internal void LazyInitialization()`
	`158`	`+ internal void LazyInitialize()`
`159`	`159`	`{`
`160`	`160`	`if (!m_Initialized)`
`161`	`161`	`{`
`@@ -168,7 +168,7 @@ internal void LazyInitialization()`
`168`	`168`	`/// Enable stepping of the Academy during the FixedUpdate phase. This is done by creating a temporary`
`169`	`169`	`/// GameObject with a MonoBehavior that calls Academy.EnvironmentStep().`
`170`	`170`	`/// </summary>`
`171`		`- public void EnableAutomaticStepping()`
	`171`	`+ void EnableAutomaticStepping()`
`172`	`172`	`{`
`173`	`173`	`if (m_FixedUpdateStepper != null)`
`174`	`174`	`{`
`@@ -185,15 +185,15 @@ public void EnableAutomaticStepping()`
`185`	`185`	`/// Disable stepping of the Academy during the FixedUpdate phase. If this is called, the Academy must be`
`186`	`186`	`/// stepped manually by the user by calling Academy.EnvironmentStep().`
`187`	`187`	`/// </summary>`
`188`		`- public void DisableAutomaticStepping(bool destroyImmediate = false)`
	`188`	`+ void DisableAutomaticStepping()`
`189`	`189`	`{`
`190`	`190`	`if (m_FixedUpdateStepper == null)`
`191`	`191`	`{`
`192`	`192`	`return;`
`193`	`193`	`}`
`194`	`194`
`195`	`195`	`m_FixedUpdateStepper = null;`
`196`		`- if (destroyImmediate)`
	`196`	`+ if (Application.isEditor)`
`197`	`197`	`{`
`198`	`198`	`UnityEngine.Object.DestroyImmediate(m_StepperObject);`
`199`	`199`	`}`
`@@ -206,11 +206,21 @@ public void DisableAutomaticStepping(bool destroyImmediate = false)`
`206`	`206`	`}`
`207`	`207`
`208`	`208`	`/// <summary>`
`209`		`- /// Returns whether or not the Academy is automatically stepped during the FixedUpdate phase.`
	`209`	`+ /// Determines whether or not the Academy is automatically stepped during the FixedUpdate phase.`
`210`	`210`	`/// </summary>`
`211`		`- public bool IsAutomaticSteppingEnabled`
	`211`	`+ public bool AutomaticSteppingEnabled`
`212`	`212`	`{`
`213`	`213`	`get { return m_FixedUpdateStepper != null; }`
	`214`	`+ set {`
	`215`	`+ if (value)`
	`216`	`+ {`
	`217`	`+ EnableAutomaticStepping();`
	`218`	`+ }`
	`219`	`+ else`
	`220`	`+ {`
	`221`	`+ DisableAutomaticStepping();`
	`222`	`+ }`
	`223`	`+ }`
`214`	`224`	`}`
`215`	`225`
`216`	`226`	`// Used to read Python-provided environment parameters`
`@@ -335,9 +345,9 @@ void OnResetCommand()`
`335`	`345`	`/// <returns>`
`336`	`346`	`/// Current episode number.`
`337`	`347`	`/// </returns>`
`338`		`- public int GetEpisodeCount()`
	`348`	`+ public int EpisodeCount`
`339`	`349`	`{`
`340`		`- return m_EpisodeCount;`
	`350`	`+ get { return m_EpisodeCount; }`
`341`	`351`	`}`
`342`	`352`
`343`	`353`	`/// <summary>`
`@@ -346,9 +356,9 @@ public int GetEpisodeCount()`
`346`	`356`	`/// <returns>`
`347`	`357`	`/// Current step count.`
`348`	`358`	`/// </returns>`
`349`		`- public int GetStepCount()`
	`359`	`+ public int StepCount`
`350`	`360`	`{`
`351`		`- return m_StepCount;`
	`361`	`+ get { return m_StepCount; }`
`352`	`362`	`}`
`353`	`363`
`354`	`364`	`/// <summary>`
`@@ -357,9 +367,9 @@ public int GetStepCount()`
`357`	`367`	`/// <returns>`
`358`	`368`	`/// Total step count.`
`359`	`369`	`/// </returns>`
`360`		`- public int GetTotalStepCount()`
	`370`	`+ public int TotalStepCount`
`361`	`371`	`{`
`362`		`- return m_TotalStepCount;`
	`372`	`+ get { return m_TotalStepCount; }`
`363`	`373`	`}`
`364`	`374`
`365`	`375`	`/// <summary>`
`@@ -445,7 +455,7 @@ internal ModelRunner GetOrCreateModelRunner(`
`445`	`455`	`/// </summary>`
`446`	`456`	`public void Dispose()`
`447`	`457`	`{`
`448`		`- DisableAutomaticStepping(true);`
	`458`	`+ DisableAutomaticStepping();`
`449`	`459`	`// Signal to listeners that the academy is being destroyed now`
`450`	`460`	`DestroyAction?.Invoke();`
`451`	`461`