Skip to content

Removing Obsolete methods from the package #5024

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.

Sign up for GitHub

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 7 commits into from
Mar 8, 2021
Merged

Removing Obsolete methods from the package #5024

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b51f827
Removing Obsolete methods from the package
vincentpierre Mar 2, 2021
911696e
Missing depecration and modified changelog
vincentpierre Mar 3, 2021
3ace30a
Readding the obsolete BrainParameter methods, will need a larger disc…
vincentpierre Mar 3, 2021
7d9eb6d
Removing Action Masker, readding the warining when using a non-implem…
vincentpierre Mar 3, 2021
5fc2748
Merge branch 'v2-staging' into v-staging-remove-obsolete
vincentpierre Mar 4, 2021
b8001c6
removing documentation and some calls to deprecated methods in the ex…
vincentpierre Mar 4, 2021
47c4489
Editing the Changelog to put the unreleased on top
vincentpierre Mar 5, 2021
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
6 changes: 2 additions & 4 deletions Project/Assets/ML-Agents/Examples/Basic/Scripts/BasicActuatorComponent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,9 @@ public class BasicActuatorComponent : ActuatorComponent
/// Creates a BasicActuator.
/// </summary>
/// <returns></returns>
#pragma warning disable 672
public override IActuator CreateActuator()
#pragma warning restore 672
public override IActuator[] CreateActuators()
{
return new BasicActuator(basicController);
return new IActuator[] { new BasicActuator(basicController) };
}

public override ActionSpec ActionSpec
Expand Down
6 changes: 2 additions & 4 deletions Project/Assets/ML-Agents/Examples/Match3/Scripts/Match3ExampleActuatorComponent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,12 @@ namespace Unity.MLAgentsExamples
public class Match3ExampleActuatorComponent : Match3ActuatorComponent
{
/// <inheritdoc/>
#pragma warning disable 672
public override IActuator CreateActuator()
#pragma warning restore 672
public override IActuator[] CreateActuators()
{
var board = GetComponent<Match3Board>();
var agent = GetComponentInParent<Agent>();
var seed = RandomSeed == -1 ? gameObject.GetInstanceID() : RandomSeed + 1;
return new Match3ExampleActuator(board, ForceHeuristic, agent, ActuatorName, seed);
return new IActuator[] { new Match3ExampleActuator(board, ForceHeuristic, agent, ActuatorName, seed) };
}
}
}
6 changes: 2 additions & 4 deletions com.unity.ml-agents.extensions/Runtime/Match3/Match3ActuatorComponent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,12 @@ public class Match3ActuatorComponent : ActuatorComponent
public bool ForceHeuristic;

/// <inheritdoc/>
#pragma warning disable 672
public override IActuator CreateActuator()
#pragma warning restore 672
public override IActuator[] CreateActuators()
{
var board = GetComponent<AbstractBoard>();
var agent = GetComponentInParent<Agent>();
var seed = RandomSeed == -1 ? gameObject.GetInstanceID() : RandomSeed + 1;
return new Match3Actuator(board, ForceHeuristic, seed, agent, ActuatorName);
return new IActuator[] { new Match3Actuator(board, ForceHeuristic, seed, agent, ActuatorName) };
}

/// <inheritdoc/>
Expand Down
15 changes: 15 additions & 0 deletions com.unity.ml-agents/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,21 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
and this project adheres to
[Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## [2.0.0-preview]
Copy link
Contributor

Choose a reason for hiding this comment

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

Package verification will probably complain about this. You might need to mark the old Unreleased section as 1.9.0, and this as Unreleased

### Major Changes
#### com.unity.ml-agents (C#)
- Some methods previously marked as `Obsolete` have been removed. If you were using these methods, you need to replace them with their supported counterpart.
#### ml-agents / ml-agents-envs / gym-unity (Python)

### Minor Changes
#### com.unity.ml-agents / com.unity.ml-agents.extensions (C#)
#### ml-agents / ml-agents-envs / gym-unity (Python)

### Bug Fixes
#### com.unity.ml-agents (C#)
#### ml-agents / ml-agents-envs / gym-unity (Python)


## [Unreleased]
### Major Changes
#### com.unity.ml-agents (C#)
Expand Down
14 changes: 1 addition & 13 deletions com.unity.ml-agents/Runtime/Actuators/ActuatorComponent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,12 @@ namespace Unity.MLAgents.Actuators
/// </summary>
public abstract class ActuatorComponent : MonoBehaviour
{
/// <summary>
/// Create the IActuator. This is called by the Agent when it is initialized.
/// </summary>
/// <returns>Created IActuator object.</returns>
[Obsolete("Use CreateActuators instead.")]
public abstract IActuator CreateActuator();

/// <summary>
/// Create a collection of <see cref="IActuator"/>s. This is called by the <see cref="Agent"/> during
/// initialization.
/// </summary>
/// <returns>A collection of <see cref="IActuator"/>s</returns>
public virtual IActuator[] CreateActuators()
{
#pragma warning disable 618
return new[] { CreateActuator() };
#pragma warning restore 618
}
public abstract IActuator[] CreateActuators();

/// <summary>
/// The specification of the possible actions for this ActuatorComponent.
Expand Down
41 changes: 0 additions & 41 deletions com.unity.ml-agents/Runtime/Actuators/IActionReceiver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,47 +148,6 @@ public override int GetHashCode()
return (ContinuousActions.GetHashCode() * 397) ^ DiscreteActions.GetHashCode();
}
}

/// <summary>
/// Packs the continuous and discrete actions into one float array. The array passed into this method
/// must have a Length that is greater than or equal to the sum of the Lengths of
/// <see cref="ContinuousActions"/> and <see cref="DiscreteActions"/>.
/// </summary>
/// <param name="destination">A float array to pack actions into whose length is greater than or
/// equal to the addition of the Lengths of this objects <see cref="ContinuousActions"/> and
/// <see cref="DiscreteActions"/> segments.</param>
[Obsolete("PackActions has been deprecated.")]
public void PackActions(in float[] destination)
{
Debug.Assert(destination.Length >= ContinuousActions.Length + DiscreteActions.Length,
$"argument '{nameof(destination)}' is not large enough to pack the actions into.\n" +
$"{nameof(destination)}.Length: {destination.Length}\n" +
$"{nameof(ContinuousActions)}.Length + {nameof(DiscreteActions)}.Length: {ContinuousActions.Length + DiscreteActions.Length}");

var start = 0;
if (ContinuousActions.Length > 0)
{
Array.Copy(ContinuousActions.Array,
ContinuousActions.Offset,
destination,
start,
ContinuousActions.Length);
start = ContinuousActions.Length;
}
if (start >= destination.Length)
{
return;
}

if (DiscreteActions.Length > 0)
{
Array.Copy(DiscreteActions.Array,
DiscreteActions.Offset,
destination,
start,
DiscreteActions.Length);
}
}
}

/// <summary>
Expand Down
57 changes: 0 additions & 57 deletions com.unity.ml-agents/Runtime/Agent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,17 +338,6 @@ internal struct AgentParameters
/// </summary>
IActuator m_VectorActuator;

/// <summary>
/// This is used to avoid allocation of a float array every frame if users are still using the old
/// OnActionReceived method.
/// </summary>
float[] m_LegacyActionCache;

/// <summary>
/// This is used to avoid allocation of a float array during legacy calls to Heuristic.
/// </summary>
float[] m_LegacyHeuristicCache;

/// Currect MultiAgentGroup ID. Default to 0 (meaning no group)
int m_GroupId;

Expand Down Expand Up @@ -952,29 +941,7 @@ public virtual void Initialize() { }
/// <seealso cref="IActionReceiver.OnActionReceived"/>
public virtual void Heuristic(in ActionBuffers actionsOut)
{
// Disable deprecation warnings so we can call the legacy overload.
#pragma warning disable CS0618

// The default implementation of Heuristic calls the
// obsolete version for backward compatibility
switch (m_PolicyFactory.BrainParameters.VectorActionSpaceType)
{
case SpaceType.Continuous:
Heuristic(m_LegacyHeuristicCache);
Array.Copy(m_LegacyHeuristicCache, actionsOut.ContinuousActions.Array, m_LegacyActionCache.Length);
actionsOut.DiscreteActions.Clear();
break;
case SpaceType.Discrete:
Heuristic(m_LegacyHeuristicCache);
var discreteActionSegment = actionsOut.DiscreteActions;
for (var i = 0; i < actionsOut.DiscreteActions.Length; i++)
{
discreteActionSegment[i] = (int)m_LegacyHeuristicCache[i];
}
actionsOut.ContinuousActions.Clear();
break;
}
#pragma warning restore CS0618
}

/// <summary>
Expand Down Expand Up @@ -1064,8 +1031,6 @@ void InitializeActuators()
var param = m_PolicyFactory.BrainParameters;
m_VectorActuator = new AgentVectorActuator(this, this, param.ActionSpec);
m_ActuatorManager = new ActuatorManager(attachedActuators.Length + 1);
m_LegacyActionCache = new float[m_VectorActuator.TotalNumberOfActions()];
m_LegacyHeuristicCache = new float[m_VectorActuator.TotalNumberOfActions()];

m_ActuatorManager.Add(m_VectorActuator);

Expand Down Expand Up @@ -1229,10 +1194,6 @@ public virtual void WriteDiscreteActionMask(IDiscreteActionMask actionMask)
{
m_ActionMasker = new DiscreteActionMasker(actionMask);
}
// Disable deprecation warnings so we can call the legacy overload.
#pragma warning disable CS0618
CollectDiscreteActionMasks(m_ActionMasker);
#pragma warning restore CS0618
}

/// <summary>
Expand Down Expand Up @@ -1310,24 +1271,6 @@ public virtual void OnActionReceived(ActionBuffers actions)
// Nothing implemented.
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

OnActionReceived should be empty now (no reason to look at the actionSpec)

if (!actions.ContinuousActions.IsEmpty())
{
Array.Copy(actions.ContinuousActions.Array,
m_LegacyActionCache,
actionSpec.NumContinuousActions);
}
else
{
for (var i = 0; i < m_LegacyActionCache.Length; i++)
{
m_LegacyActionCache[i] = (float)actions.DiscreteActions[i];
}
}
// Disable deprecation warnings so we can call the legacy overload.
#pragma warning disable CS0618
OnActionReceived(m_LegacyActionCache);
#pragma warning restore CS0618
}

/// <summary>
Expand Down
63 changes: 0 additions & 63 deletions com.unity.ml-agents/Runtime/Agent.deprecated.cs
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions com.unity.ml-agents/Runtime/Agent.deprecated.cs.meta
View file
Open in desktop

This file was deleted.

49 changes: 0 additions & 49 deletions com.unity.ml-agents/Runtime/Policies/BrainParameters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,6 @@

namespace Unity.MLAgents.Policies
{
/// <summary>
/// This is deprecated. Agents can now use both continuous and discrete actions together.
/// </summary>
[Obsolete("Continuous and discrete actions on the same Agent are now supported; see ActionSpec.")]
public enum SpaceType
{
/// <summary>
/// Discrete action space: a fixed number of options are available.
/// </summary>
Discrete,

/// <summary>
/// Continuous action space: each action can take on a float value.
/// </summary>
Continuous
}

/// <summary>
/// Holds information about the brain. It defines what are the inputs and outputs of the
Expand Down Expand Up @@ -69,49 +53,16 @@ public ActionSpec ActionSpec
}
}

/// <summary>
/// (Deprecated) The number of possible actions.
/// </summary>
/// <remarks>The size specified is interpreted differently depending on whether
/// the agent uses the continuous or the discrete actions.</remarks>
/// <value>
/// For the continuous actions: the length of the float vector that represents
/// the action.
/// For the discrete actions: the number of branches.
/// </value>
[Obsolete("VectorActionSize has been deprecated, please use ActionSpec instead.")]
[FormerlySerializedAs("vectorActionSize")]
public int[] VectorActionSize = new[] { 1 };
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, I think we need to keep these around so that old assets can still be loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by old assets ? What functionality are we trying to preserve ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear, for v2, I was thinking refactoring BrainParameters and BehaviorParameters completely, so maybe it is best to set expectations early and tune down the scope I had in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you have a scene or prefab from v1.6.0 or earlier, the BrainParameters have VectorActionSize and VectorActionSpaceType fields on them. When you first open this in 1.7.0 or later, we have to read the values from the old field and fill in the ActionSpec (m_ActionSpec) with them. The work happens in the UpdateToActionSpec() method, which is called in OnBeforeSerialize() and OnAfterDeserialize().

So if we remove these and a user loads a scene from <=1.6.0, their Agent will have no actions. In my opinion, this is a big pain for upgrading and we shouldn't do it. I'm willing to be convinced otherwise, but I think we need a bigger discussion on it.

That being said, I don't love that we have to keep the fields around (and public!) just so that they can be read once. It would be worth following up with #devs-serialization to see if there's a better way we can do this while hiding the fields. If nothing else, we can rename them to e.g. DeprecatedDoNotUseVectorActionSize.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, these fields are used in a bunch of places that you didn't update, so this doesn't compile. I'll update yamato tests to run on PRs targeting the staging branch.


/// <summary>
/// The list of strings describing what the actions correspond to.
/// </summary>
[FormerlySerializedAs("vectorActionDescriptions")]
public string[] VectorActionDescriptions;

/// <summary>
/// (Deprecated) Defines if the action is discrete or continuous.
/// </summary>
[Obsolete("VectorActionSpaceType has been deprecated, please use ActionSpec instead.")]
[FormerlySerializedAs("vectorActionSpaceType")]
public SpaceType VectorActionSpaceType = SpaceType.Discrete;
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to stay too.


[SerializeField]
[HideInInspector]
internal bool hasUpgradedBrainParametersWithActionSpec;

/// <summary>
/// (Deprecated) The number of actions specified by this Brain.
/// </summary>
[Obsolete("NumActions has been deprecated, please use ActionSpec instead.")]
public int NumActions
{
get
{
return ActionSpec.NumContinuousActions > 0 ? ActionSpec.NumContinuousActions : ActionSpec.NumDiscreteActions;
}
}

/// <summary>
/// Deep clones the BrainParameter object.
/// </summary>
Expand Down
Loading