Skip to content

ISensor interface and use for visual observations #2731

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

Jump to bottom
Merged
merged 38 commits into from
Oct 22, 2019
Merged

ISensor interface and use for visual observations #2731

merged 38 commits into from
Oct 22, 2019

Conversation

chriselion
Copy link
Contributor

@chriselion chriselion commented Oct 15, 2019 •
edited
Loading

Adds an ISensor interface, and implements Camera and RenderTexture Sensors. This will be more extensible for vector observations in the future, but should only be used for visual observations at the moment.

Notable interface changes / upgrade issues:

  • Visual observations are now configured via Camera Sensor Component and RenderTexture Sensor Components. This will require changes to any scenes which were previously using visual observations.
  • AgentParameters no longer contains Cameras and RenderTexture.
  • BrainParameters no longer requires setting Camera Resolutions. The resolutions are now set on Camera Sensor Component and RenderTexture Sensor Components.

chriselion
chriselion commented Oct 15, 2019
View reviewed changes
chriselion
chriselion commented Oct 15, 2019
View reviewed changes
UnitySDK/Assets/ML-Agents/Scripts/Utilities.cs
@@ -28,31 +28,67 @@ public static class Utilities
var numTextures = textures.Count;
var width = textures[0].width;
var height = textures[0].height;
var data = tensorProxy.data;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old code wrote all the Textures to the tensor at once. Now we'll do it one texture at a time.

vincentpierre
vincentpierre approved these changes Oct 15, 2019
View reviewed changes
Copy link
Contributor

@vincentpierre vincentpierre left a comment

Choose a reason for hiding this comment

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

I know this is a WIP but I approve of the idea and implementation.

surfnerd
surfnerd reviewed Oct 15, 2019
View reviewed changes
UnitySDK/Assets/ML-Agents/Scripts/Agent.cs Outdated
param.cameraResolutions[camCount + i].height);
m_Info.visualObservations.Add(obsTexture);
// TODO separate this from inference time
var sensor = m_Sensors[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

that looks much nicer

surfnerd
surfnerd reviewed Oct 15, 2019
View reviewed changes
surfnerd
surfnerd reviewed Oct 15, 2019
View reviewed changes
surfnerd
surfnerd reviewed Oct 15, 2019
View reviewed changes
surfnerd
surfnerd reviewed Oct 15, 2019
View reviewed changes
surfnerd
surfnerd reviewed Oct 15, 2019
View reviewed changes
UnitySDK/Assets/ML-Agents/Scripts/Utilities.cs

var t = textureOffset;
var texturePixels = texture.GetPixels32();
for (var h = height - 1; h >= 0; h--)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a comment here about h--

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is the old code but probably unindented one level. Need to merge that other PR and update this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do this after merge

@vincentpierre vincentpierre mentioned this pull request Oct 15, 2019
Merged
chriselion
chriselion commented Oct 18, 2019
View reviewed changes
ml-agents/mlagents/trainers/tests/mock_brain.py
@@ -235,3 +235,26 @@ def create_mock_banana_brain():
vector_observation_space_size=0,
)
return mock_brain


def make_brain_parameters(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to try to combine this with the other methods in this file.

@vincentpierre vincentpierre self-requested a review October 21, 2019 16:55
chriselion
chriselion commented Oct 21, 2019
View reviewed changes
UnitySDK/Assets/ML-Agents/Scripts/Sensor/CameraSensorComponent.cs
class CameraSensorComponent : SensorComponent
{
public new Camera camera;
public string sensorName = "CameraSensor";
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 think we can revisit this and try to make a unique name if adding another sensor component of the same type. It's OK for now though.

chriselion
chriselion commented Oct 21, 2019
View reviewed changes
UnitySDK/Assets/ML-Agents/Scripts/Sensor/RenderTextureSensor.cs
m_Height = height;
m_Grayscale = grayscale;
m_Name = name;
m_Shape = new[] { width, height, grayscale ? 1 : 3 };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: shape is cached now, so no extra allocations at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(and same for CameraSensor)

@chriselion chriselion marked this pull request as ready for review October 21, 2019 18:43
vincentpierre
vincentpierre reviewed Oct 21, 2019
View reviewed changes
UnitySDK/Assets/ML-Agents/Scripts/Agent.cs
/// <summary>
/// Generate data for each sensor and store it on the Agent's AgentInfo
/// </summary>
public void GenerateSensorData()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason the Compressed data is in the AgentInfo ? Would it be easier to generate it in the Communicator and the recorder? Maybe this is good for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you document in the code what the future design should look like ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should talk about it some more. I think the Brain (and maybe the presence of the recorder) should determine whether we're in compressed, uncompressed, or write-to-tensor mode; this is something that could be passed as a parameter to CollectObservations, which could call GenerateSensorData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be easier to generate it in the Communicator and the recorder?

The recorder operates on AgentInfos, not Agents.

We could pass the ISensors instead of the CompressedObservations in AgentInfo, and let the AgentInfo consumer decide what to do with them...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some comments

UnitySDK/Assets/ML-Agents/Scripts/InferenceBrain/ModelRunner.cs Outdated
if (!m_visualObservationsInitialized)
{
// Just grab the first agent in the collection (any will suffice, really).
var firstAgent = agents.FirstOrDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for FirstOrDefault to return a default (an agent that does not exist) and mess up the Initialization of the TensorGenerator ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few lines above, we early out if agents.Count is 0, so we should always have a valid Agent to grab here. I'll change this to First() and a comment though, so that it's clearer.

@vincentpierre
Copy link
Contributor

The AreaRemderTexture Agent in the gridworld scene is using a Camera sensor but should be using a RenderTextureSensor.
You should also add the CameraSensor to all of the other Visual Environments such as Pyramids and Pushblock

@chriselion
Copy link
Contributor Author

@vincentpierre Updated scenes - everything uses CameraSensor Component (except for the RenderTexture one on GridWorld)

vincentpierre
vincentpierre approved these changes Oct 22, 2019
View reviewed changes
Copy link
Contributor

@vincentpierre vincentpierre left a comment

Choose a reason for hiding this comment

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

I go this warning in the unity console :

Assets/ML-Agents/Scripts/Sensor/CameraSensorComponent.cs(8,27): warning CS0649: Field `MLAgents.Sensor.CameraSensorComponent.camera' is never assigned to, and will always have its default value `null'

I had a few minor comments but nothing serious. Maybe wait on @surfnerd to give a thumbs up since this is a major change.

UnitySDK/Assets/ML-Agents/Scripts/InferenceBrain/BarracudaModelParamLoader.cs
}
}

// TODO reenable checks there are enough Visual Observation Placeholder in the model.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a way to do these checks ?

Copy link
Contributor

Choose a reason for hiding this comment

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

➕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not currently, at least not at model load time, since we don't have any way to get the observation sizes until there's an Agent.

UnitySDK/Assets/ML-Agents/Scripts/Sensor/CameraSensorComponent.cs
{
class CameraSensorComponent : SensorComponent
{
public new Camera camera;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the new keyword here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't this it's needed here

Copy link
Contributor Author

@chriselion chriselion Oct 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Without new:

Assets/ML-Agents/Scripts/Sensor/CameraSensorComponent.cs(8,23): warning CS0108: `MLAgents.Sensor.CameraSensorComponent.camera' hides inherited member `UnityEngine.Component.camera'. Use the new keyword if hiding was intended

Should I just use the base member? Not sure how that's normally used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make a new one. cAmErA ?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • SensingCamera?
  • SensorCamera?
  • AttachedCamera?
  • ObservationCamera?

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 like ObservationCamera. @surfnerd any preferences?

This means that I either need to fix up the scenes again or use a FormerlySerializedAs, so want to make sure I only do it once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also the base member is deprecated; I'd rather just leave the new here unless you feel strongly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wow, yeah that property was removed from Component in 2019.2. new is fine. I didn't realize there was a camera property.

@chriselion
Copy link
Contributor Author

Re: the warning: the field is assigned in the UI

@vincentpierre
Copy link
Contributor

Re: the warning: the field is assigned in the UI

There has to be a way to suppress this warning, we have a lot of fields that are only assigned in the inspector. I think there is a way around it.

@chriselion
Copy link
Contributor Author

Looks like marking the classes as public gets rid of the warnings (and is probably something we want anyway): https://stackoverflow.com/a/12618676/224264

surfnerd
surfnerd reviewed Oct 22, 2019
View reviewed changes
surfnerd
surfnerd approved these changes Oct 22, 2019
View reviewed changes
Copy link
Contributor

@surfnerd surfnerd left a comment

Choose a reason for hiding this comment

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

Looks good! I'll create a JIRA for the render texture followup.

@chriselion chriselion merged commit 4a56460 into develop Oct 22, 2019
@chriselion chriselion deleted the develop-sensor-interface branch October 23, 2019 17:00
@chriselion chriselion changed the title [WIP] ISensor interface and use for visual observations ISensor interface and use for visual observations Oct 23, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vincentpierre vincentpierre vincentpierre approved these changes

@surfnerd surfnerd surfnerd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chriselion @vincentpierre @surfnerd