Convert Academy to a singleton #3210
Conversation
UnitySDK/Assets/ML-Agents/Examples/Pyramids/Scripts/PyramidAgent.cs
Outdated
Show resolved
Hide resolved
UnitySDK/Assets/ML-Agents/Examples/SharedAssets/Scripts/ProjectSettingsOverrides.cs
Outdated
Show resolved
Hide resolved
| { | ||
| Application.quitting += OnDestroy; |
There was a problem hiding this comment.
Do we evert want to deregister from Application.quitting?
| @@ -203,9 +284,9 @@ void InitializeEnvironment() | |||
| new CommunicatorInitParameters | |||
| { | |||
| version = k_ApiVersion, | |||
| name = gameObject.name, | |||
| name = "AcademySingleton", | |||
There was a problem hiding this comment.
Should we have a name field? Or default this to the current scene?
There was a problem hiding this comment.
name used to be given to Python at some point. we removed this (the Python environment is now nameless) so we can delete that field I think.
There was a problem hiding this comment.
It was coming from the Unity GameObject (e.g. "3DBallAcademy"). I'll remove it from the protos in a subsequent PR.
There was a problem hiding this comment.
https://jira.unity3d.com/browse/MLA-518 for followup
| @@ -203,9 +284,9 @@ void InitializeEnvironment() | |||
| new CommunicatorInitParameters | |||
| { | |||
| version = k_ApiVersion, | |||
| name = gameObject.name, | |||
| name = "AcademySingleton", | |||
There was a problem hiding this comment.
name used to be given to Python at some point. we removed this (the Python environment is now nameless) so we can delete that field I think.
UnitySDK/Assets/ML-Agents/Examples/SharedAssets/Scripts/ProjectSettingsOverrides.cs
Outdated
Show resolved
Hide resolved
| var acaGo = new GameObject("TestAcademy"); | ||
| acaGo.AddComponent<Academy>(); | ||
| var aca = acaGo.GetComponent<Academy>(); | ||
| Assert.AreEqual(0, aca.GetStepCount()); |
There was a problem hiding this comment.
Some of these don't make sense anymore, because we never have an uninitialized Academy
| @@ -21,10 +22,16 @@ | |||
|
|
|||
| namespace MLAgents | |||
| { | |||
| public class AcademyStepper : MonoBehaviour | |||
| { | |||
| void FixedUpdate() | |||
There was a problem hiding this comment.
Why hide ?
Also, maybe rename AcademyDefaultStepper ? Or AcademyFixedUpdateStepper for clarity ?
There was a problem hiding this comment.
Renamed to AcademyFixedUpdateStepper.
I added m_StepperObject.hideFlags = HideFlags.HideInHierarchy in the setup code below, because I think it would be surprising to the user to see a GameObject that they didn't create there.
There was a problem hiding this comment.
Maybe only hide if the user is not in using a DEBUG flag ? Like what you did for sensor validators?
There was a problem hiding this comment.
There's really no reason for the user to interact with this object in the editor, or ever. I'm in favor of hiding it over showing it conditionally (which would only ever be in the editor in debug mode).
| } | ||
|
|
||
| m_Stepper = null; | ||
| UnityEngine.Object.Destroy(m_StepperObject); |
There was a problem hiding this comment.
@surfnerd Is this right, or do I need to Destroy the MonoBehaviour too?
There was a problem hiding this comment.
Should the AcademyStepper object destroy itself when the Academy disappears?
You should explicitly call using UnityEngine; on top of file so we know we rely on it in this script.
There was a problem hiding this comment.
Should the AcademyStepper object destroy itself when the Academy disappears?
Yeah, I was missing that. Changing so that the Academy deletes it in Dispose()
You should explicitly call using UnityEngine; on top of file so we know we rely on it in this script
using UnityEngine is already at the top. I think it's necessary here to disambiguate between https://docs.microsoft.com/en-us/dotnet/api/system.object?view=netframework-4.8
There was a problem hiding this comment.
Unit tests were complaining about using Destroy instead of DestroyImmediate in Editor mode. I changed things around a bit to use DestroyImmediate from Dispose (but will still use Destroy when disabling normally)
| @@ -21,10 +22,16 @@ | |||
|
|
|||
| namespace MLAgents | |||
| { | |||
| public class AcademyStepper : MonoBehaviour | |||
| { | |||
| void FixedUpdate() | |||
There was a problem hiding this comment.
Maybe only hide if the user is not in using a DEBUG flag ? Like what you did for sensor validators?
| } | ||
|
|
||
| // Clear out the actions so we're not keeping references to any old objects |
Co-Authored-By: Chris Goy <christopherg@unity3d.com>
Co-Authored-By: Chris Goy <christopherg@unity3d.com>
This is mostly ready, except for unit test fixes and documentation.