Skip to content

[bugfix] Fix MLA-793 Make Unity lifecycle methods protected. Added tests for changes #3590

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

Merged
merged 3 commits into from
Mar 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Project/Project.sln.DotSettings
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was added automatically by Rider. 🤷‍♂

<s:Boolean x:Key="/Default/UserDictionary/Words/=Dont/@EntryIndexedValue">True</s:Boolean></wpf:ResourceDictionary>
1 change: 1 addition & 0 deletions com.unity.ml-agents/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- `AgentInfo.actionMasks` has been renamed to `AgentInfo.discreteActionMasks`.
- `DecisionRequester` has been made internal (you can still use the DecisionRequesterComponent from the inspector). `RepeatAction` was renamed `TakeActionsBetweenDecisions` for clarity. (#3555)
- The `IFloatProperties` interface has been removed.
- Fix #3579.
Copy link

Choose a reason for hiding this comment

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

Lets expand with a 1-liner and then have a link to the Issue


## [0.14.1-preview] - 2020-02-25

Expand Down
17 changes: 15 additions & 2 deletions com.unity.ml-agents/Runtime/Agent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,10 @@ public string BehaviorName {
/// </summary>
internal VectorSensor collectObservationsSensor;

void OnEnable()
/// <summary>
/// Called when the attached <see cref="GameObject"/> becomes enabled and active.
/// </summary>
protected virtual void OnEnable()
{
LazyInitialize();
}
Expand Down Expand Up @@ -299,7 +302,10 @@ enum DoneReason
Disabled,
}

void OnDisable()
/// <summary>
/// Called when the attached <see cref="GameObject"/> becomes disabled and inactive.
/// </summary>
protected virtual void OnDisable()
{
DemonstrationWriters.Clear();

Expand Down Expand Up @@ -570,6 +576,13 @@ internal void InitializeSensors()
/// </summary>
void SendInfoToBrain()
{
if (!m_Initialized)
{
throw new UnityAgentsException("Call to SendInfoToBrain when Agent hasn't been initialized." +
"Please ensure that you are calling 'base.OnEnable()' if you have overridden OnEnable.");

Copy link

Choose a reason for hiding this comment

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

remove empty line

}

if (m_Brain == null)
{
return;
Expand Down
58 changes: 58 additions & 0 deletions com.unity.ml-agents/Tests/Editor/MLAgentsEditModeTest.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.CodeDom;
using UnityEngine;
using NUnit.Framework;
using System.Reflection;
Expand Down Expand Up @@ -602,4 +603,61 @@ public void TestHeuristicPolicyStepsSensors()
Assert.AreEqual(numSteps, agent1.sensor2.numCompressedCalls);
}
}

[TestFixture]
public class TestOnEnableOverride
{
public class OnEnableAgent : Agent
{
public bool callBase;

protected override void OnEnable()
{
if (callBase)
Copy link

Choose a reason for hiding this comment

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

missing { }

base.OnEnable();
}
}

static void _InnerAgentTestOnEnableOverride(bool callBase = false)
{
var go = new GameObject();
var agent = go.AddComponent<OnEnableAgent>();
agent.callBase = callBase;
var onEnable = typeof(OnEnableAgent).GetMethod("OnEnable", BindingFlags.NonPublic | BindingFlags.Instance);
var sendInfo = typeof(Agent).GetMethod("SendInfoToBrain", BindingFlags.NonPublic | BindingFlags.Instance);
Assert.NotNull(onEnable);
onEnable.Invoke(agent, null);
Assert.NotNull(sendInfo);
if (agent.callBase)
{
Assert.DoesNotThrow(() => sendInfo.Invoke(agent, null));
}
else
{
Assert.Throws<UnityAgentsException>(() =>
{
try
{
sendInfo.Invoke(agent, null);
}
catch (TargetInvocationException e)
{
throw e.GetBaseException();
}
});
}
}

[Test]
public void TestAgentCallBaseOnEnable()
{
_InnerAgentTestOnEnableOverride(true);
}

[Test]
public void TestAgentDontCallBaseOnEnable()
{
_InnerAgentTestOnEnableOverride();
}
}
}