[MLA-12] update protobuf for vector observations #2862
Conversation
| var numFloats = sensor.Write(m_WriteAdapter); | ||
| var floatObs = new Observation | ||
| { | ||
| FloatData = new ArraySegment<float>(m_VectorSensorBuffer, floatsWritten, numFloats), |
| @@ -56,4 +56,24 @@ public interface ISensor { | |||
| string GetName(); | |||
| } | |||
|
|
|||
| public static class ISensorExtensions | |||
There was a problem hiding this comment.
I guess this is the equivalent of adding code in a header file. Nice.
There was a problem hiding this comment.
Yeah, I just wanted an implementation that used the interface method. https://stackoverflow.com/questions/2770333/can-extension-methods-be-applied-to-interfaces says it's OK.
| @@ -523,6 +518,17 @@ public void InitializeSensors() | |||
| Debug.Assert(!sensors[i].GetName().Equals(sensors[i + 1].GetName()), "Sensor names must be unique."); | |||
| } | |||
| #endif | |||
| // Create a buffer for writing vector sensor data too | |||
There was a problem hiding this comment.
Might need to discuss alternatives to this. We need to have somewhere for non-compressed sensors to write before passing this to the proto objects; this seemed like a good way to prevent repeated allocations. Another option would be to have the Communicator have a cache of Observations (indexed by size), and it manages the Write() calls before converting to proto.
| int floatsWritten = 0; | ||
| // Generate data for all Sensors | ||
| for (var i = 0; i < sensors.Count; i++) | ||
| { | ||
| var sensor = sensors[i]; | ||
| if (sensor.GetCompressionType() == SensorCompressionType.None) | ||
| { | ||
| m_WriteAdapter.SetTarget(m_Info.floatObservations, floatsWritten); | ||
| floatsWritten += sensor.Write(m_WriteAdapter); | ||
| // only handles 1D |
There was a problem hiding this comment.
There's currently an assumption that compressed observation <=> 3 dimensional. We should (later) add an option to write visual obs as floats, just to test all the code paths (and maybe a compressed vector obs too).
| /// </summary> | ||
| /// <param name="sensor"></param> | ||
| /// <returns></returns> | ||
| public static int ObservationSize(this ISensor sensor) |
There was a problem hiding this comment.
Needed this (product of the shape components) in a few places so added it as an extension to ISensor. Any objections?
| CompressionTypeProto, | ||
| from mlagents.envs.communicator_objects.observation_pb2 import ( | ||
| ObservationProto, | ||
| NONE as COMPRESSION_TYPE_NONE, |
There was a problem hiding this comment.
This is a bit awkward, but CompressionTypeProto.NONE doesn't work on protobuf<3.8
| @@ -8,8 +8,15 @@ enum CompressionTypeProto { | |||
| PNG = 1; | |||
There was a problem hiding this comment.
Maybe NONE and PNG should be changed to NO_COMPRESSION and PNG_COMPRESSION to avoid the bug you see with NONE ?
There was a problem hiding this comment.
The problem isn't with NONE, it looks like it's any enum in protobuf <3.8. See protocolbuffers/protobuf#6028.
That being said, if you think those are better enum names, we can change them.
There was a problem hiding this comment.
Alternatively, we can also update the min protobuf version to 3.8.0
|
I wrote a one-off script to update the demo files. I'll reconfirm that they work though. |
|
For posterity, here's the script https://gist.github.com/chriselion/3714d05255eea2f9132b96a182fbdcaa (it's not generally usable, so not adding to source control) |
Several changes to the proto objects:
Includes C# and python changes necessary to support these changes, and updated the existing demonstration files.