Skip to content
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

[change] public SensorComponent fields to properties, add custom editor #3564

Merged
merged 4 commits into from
Mar 6, 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
1 change: 1 addition & 0 deletions com.unity.ml-agents/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- The method `GetStepCount()` on the Agent class has been replaced with the property getter `StepCount`
- `RayPerceptionSensorComponent` and related classes now display the debug gizmos whenever the Agent is selected (not just Play mode).
- Most fields on `RayPerceptionSensorComponent` can now be changed while the editor is in Play mode. The exceptions to this are fields that affect the number of observations.
- Most fields on `CameraSensorComponent` and `RenderTextureSensorComponent` were changed to private and replaced by properties with the same name.
- Unused static methods from the `Utilities` class (ShiftLeft, ReplaceRange, AddRangeNoAlloc, and GetSensorFloatObservationSize) were removed.
- The `Agent` class is no longer abstract.
- SensorBase was moved out of the package and into the Examples directory.
Expand Down
48 changes: 48 additions & 0 deletions com.unity.ml-agents/Editor/CameraSensorComponentEditor.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
using UnityEngine;
using UnityEditor;
using MLAgents.Sensors;

namespace MLAgents.Editor
{
[CustomEditor(typeof(CameraSensorComponent))]
[CanEditMultipleObjects]
internal class CameraSensorComponentEditor : UnityEditor.Editor
{
public override void OnInspectorGUI()
{
var so = serializedObject;
so.Update();

// Drawing the CameraSensorComponent
EditorGUI.BeginChangeCheck();

EditorGUILayout.PropertyField(so.FindProperty("m_Camera"), true);
EditorGUI.BeginDisabledGroup(Application.isPlaying);
{
// These fields affect the sensor order or observation size,
// So can't be changed at runtime.
EditorGUILayout.PropertyField(so.FindProperty("m_SensorName"), true);
EditorGUILayout.PropertyField(so.FindProperty("m_Width"), true);
EditorGUILayout.PropertyField(so.FindProperty("m_Height"), true);
EditorGUILayout.PropertyField(so.FindProperty("m_Grayscale"), true);
}
EditorGUI.EndDisabledGroup();
EditorGUILayout.PropertyField(so.FindProperty("m_Compression"), true);

var requireSensorUpdate = EditorGUI.EndChangeCheck();
so.ApplyModifiedProperties();

if (requireSensorUpdate)
{
UpdateSensor();
}
}

void UpdateSensor()
{
var sensorComponent = serializedObject.targetObject as CameraSensorComponent;
sensorComponent?.UpdateSensor();
}

}
}
11 changes: 11 additions & 0 deletions com.unity.ml-agents/Editor/CameraSensorComponentEditor.cs.meta

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ protected void OnRayPerceptionInspectorGUI(bool is3d)
EditorGUI.BeginChangeCheck();
EditorGUI.indentLevel++;

EditorGUILayout.PropertyField(so.FindProperty("m_SensorName"), true);

// Because the number of rays and the tags affect the observation shape,
// they are not editable during play mode.
// Don't allow certain fields to be modified during play mode.
// * SensorName affects the ordering of the Agent's observations
// * The number of tags and rays affects the size of the observations.
EditorGUI.BeginDisabledGroup(Application.isPlaying);
{
EditorGUILayout.PropertyField(so.FindProperty("m_SensorName"), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make more sense to store "m_SensorName" and such in private const in case they are used somewhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, not quite sure what you mean. The user needs to assign a unique name to each sensor so that we can sort them deterministically, so the name here can't be const.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think he means the string, make it a variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here or in the component? We currently only use them in one place, and it's kinda redundant because of reflection.

We could do something like this:
https://stackoverflow.com/a/33059272/224264
although to use nameof we'd need to make the fields internal.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we only use it here, I think it is fine.

EditorGUILayout.PropertyField(so.FindProperty("m_DetectableTags"), true);
EditorGUILayout.PropertyField(so.FindProperty("m_RaysPerDirection"), true);
}
Expand Down Expand Up @@ -56,8 +56,8 @@ protected void OnRayPerceptionInspectorGUI(bool is3d)
m_RequireSensorUpdate = true;
}

UpdateSensorIfDirty();
so.ApplyModifiedProperties();
UpdateSensorIfDirty();
}


Expand Down
43 changes: 43 additions & 0 deletions com.unity.ml-agents/Editor/RenderTextureSensorComponentEditor.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
using UnityEngine;
using UnityEditor;
using MLAgents.Sensors;
namespace MLAgents.Editor
{
[CustomEditor(typeof(RenderTextureSensorComponent))]
[CanEditMultipleObjects]
internal class RenderTextureSensorComponentEditor : UnityEditor.Editor
{
public override void OnInspectorGUI()
{
var so = serializedObject;
so.Update();

// Drawing the RenderTextureComponent
EditorGUI.BeginChangeCheck();

EditorGUI.BeginDisabledGroup(Application.isPlaying);
{
EditorGUILayout.PropertyField(so.FindProperty("m_RenderTexture"), true);
EditorGUILayout.PropertyField(so.FindProperty("m_SensorName"), true);
EditorGUILayout.PropertyField(so.FindProperty("m_Grayscale"), true);
}
EditorGUI.EndDisabledGroup();

EditorGUILayout.PropertyField(so.FindProperty("m_Compression"), true);

var requireSensorUpdate = EditorGUI.EndChangeCheck();
so.ApplyModifiedProperties();

if (requireSensorUpdate)
{
UpdateSensor();
}
}

void UpdateSensor()
{
var sensorComponent = serializedObject.targetObject as RenderTextureSensorComponent;
sensorComponent?.UpdateSensor();
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 1 addition & 3 deletions com.unity.ml-agents/Runtime/Policies/BehaviorParameters.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,7 @@ enum BehaviorType
/// <summary>
/// The team ID for this behavior.
/// </summary>
[HideInInspector]
[SerializeField]
[FormerlySerializedAs("m_TeamID")]
[HideInInspector, SerializeField, FormerlySerializedAs("m_TeamID")]
Copy link
Contributor

Choose a reason for hiding this comment

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

You can do that ?!
Should we try to be consistent and make it the same way everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a simple find-and-replace for the ones for HideInInspector+ SerializeField+ FormerlySerializedAs. That's as much effort as I want to put into it for this PR.

public int TeamId;

[FormerlySerializedAs("m_useChildSensors")]
Expand Down
19 changes: 19 additions & 0 deletions com.unity.ml-agents/Runtime/Sensors/CameraSensor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,25 @@ public class CameraSensor : ISensor
int[] m_Shape;
SensorCompressionType m_CompressionType;

/// <summary>
/// The Camera used for rendering the sensor observations.
/// </summary>
public Camera camera
{
get { return m_Camera; }
set { m_Camera = value; }
}

/// <summary>
/// The compression type used by the sensor.
/// </summary>
public SensorCompressionType compressionType
{
get { return m_CompressionType; }
set { m_CompressionType = value; }
}


/// <summary>
/// Creates and returns the camera sensor.
/// </summary>
Expand Down
78 changes: 68 additions & 10 deletions com.unity.ml-agents/Runtime/Sensors/CameraSensorComponent.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using UnityEngine;
using UnityEngine.Serialization;

namespace MLAgents.Sensors
{
Expand All @@ -8,43 +9,88 @@ namespace MLAgents.Sensors
[AddComponentMenu("ML Agents/Camera Sensor", (int)MenuGroup.Sensors)]
public class CameraSensorComponent : SensorComponent
{
[HideInInspector, SerializeField, FormerlySerializedAs("camera")]
Camera m_Camera;

CameraSensor m_Sensor;

/// <summary>
/// Camera object that provides the data to the sensor.
/// </summary>
public new Camera camera;
public new Camera camera
{
get { return m_Camera; }
set { m_Camera = value; UpdateSensor(); }
}

[HideInInspector, SerializeField, FormerlySerializedAs("sensorName")]
string m_SensorName = "CameraSensor";

/// <summary>
/// Name of the generated <see cref="CameraSensor"/> object.
/// </summary>
public string sensorName = "CameraSensor";
public string sensorName
{
get { return m_SensorName; }
internal set { m_SensorName = value; }
}

[HideInInspector, SerializeField, FormerlySerializedAs("width")]
int m_Width = 84;

/// <summary>
/// Width of the generated image.
/// Width of the generated observation.
/// </summary>
public int width = 84;
public int width
{
get { return m_Width; }
internal set { m_Width = value; }
}

[HideInInspector, SerializeField, FormerlySerializedAs("height")]
int m_Height = 84;

/// <summary>
/// Height of the generated image.
/// Height of the generated observation.
/// </summary>
public int height = 84;
public int height
{
get { return m_Height; }
internal set { m_Height = value; }
}

[HideInInspector, SerializeField, FormerlySerializedAs("grayscale")]
public bool m_Grayscale;

/// <summary>
/// Whether to generate grayscale images or color.
/// </summary>
public bool grayscale;
public bool grayscale
{
get { return m_Grayscale; }
internal set { m_Grayscale = value; }
}

[HideInInspector, SerializeField, FormerlySerializedAs("compression")]
SensorCompressionType m_Compression = SensorCompressionType.PNG;

/// <summary>
/// The compression type to use for the sensor.
/// </summary>
public SensorCompressionType compression = SensorCompressionType.PNG;
public SensorCompressionType compression
{
get { return m_Compression; }
set { m_Compression = value; UpdateSensor(); }
}

/// <summary>
/// Creates the <see cref="CameraSensor"/>
/// </summary>
/// <returns>The created <see cref="CameraSensor"/> object for this component.</returns>
public override ISensor CreateSensor()
{
return new CameraSensor(camera, width, height, grayscale, sensorName, compression);
m_Sensor = new CameraSensor(m_Camera, m_Width, m_Height, grayscale, m_SensorName, compression);
return m_Sensor;
}

/// <summary>
Expand All @@ -53,7 +99,19 @@ public override ISensor CreateSensor()
/// <returns>The observation shape of the associated <see cref="CameraSensor"/> object.</returns>
public override int[] GetObservationShape()
{
return CameraSensor.GenerateShape(width, height, grayscale);
return CameraSensor.GenerateShape(m_Width, m_Height, grayscale);
}

/// <summary>
/// Update fields that are safe to change on the Sensor at runtime.
/// </summary>
internal void UpdateSensor()
{
if (m_Sensor != null)
{
m_Sensor.camera = m_Camera;
m_Sensor.compressionType = m_Compression;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@ namespace MLAgents.Sensors
[AddComponentMenu("ML Agents/Ray Perception Sensor 3D", (int)MenuGroup.Sensors)]
public class RayPerceptionSensorComponent3D : RayPerceptionSensorComponentBase
{
[HideInInspector]
[SerializeField]
[FormerlySerializedAs("startVerticalOffset")]
[HideInInspector, SerializeField, FormerlySerializedAs("startVerticalOffset")]
[Range(-10f, 10f)]
[Tooltip("Ray start is offset up or down by this amount.")]
float m_StartVerticalOffset;
Expand All @@ -25,9 +23,7 @@ public float startVerticalOffset
set { m_StartVerticalOffset = value; UpdateSensor(); }
}

[HideInInspector]
[SerializeField]
[FormerlySerializedAs("endVerticalOffset")]
[HideInInspector, SerializeField, FormerlySerializedAs("endVerticalOffset")]
[Range(-10f, 10f)]
[Tooltip("Ray end is offset up or down by this amount.")]
float m_EndVerticalOffset;
Expand Down
Loading