-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Fixes missing camera resolution info in demos #2523
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
Conversation
/// <summary> | ||
/// Converts Resolution array protobuf to C# Resolution array. | ||
/// </summary> | ||
static Resolution[] CameraProtoToNative(CommunicatorObjects.ResolutionProto[] resolutionProtos) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ResolutionProtoToNative
?
@@ -1,4 +1,5 @@ | |||
using System.Text; | |||
using System.Net.NetworkInformation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is unnecessary.
/// <summary> | ||
/// Constructs complex label for each CameraResolution object. | ||
/// </summary> | ||
static string BuildCameraResolutionLabel(SerializedProperty cameraArray) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make private.
static string BuildCameraResolutionLabel(SerializedProperty cameraArray) | ||
{ | ||
var numCameras = cameraArray.arraySize; | ||
StringBuilder cameraLabel = new StringBuilder("[ "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use var.
{ | ||
var numCameras = cameraArray.arraySize; | ||
StringBuilder cameraLabel = new StringBuilder("[ "); | ||
for (int i = 0; i < numCameras; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int --> var
|
||
cameraLabel.Append(" ]"); | ||
return cameraLabel.ToString(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
public BrainParameters(CommunicatorObjects.BrainParametersProto brainParametersProto) | ||
{ | ||
vectorObservationSize = brainParametersProto.VectorObservationSize; | ||
cameraResolutions = ResolutionProtoToNative(brainParametersProto.CameraResolutions.ToArray()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Limit to 100 chars
/// </summary> | ||
static Resolution[] ResolutionProtoToNative(CommunicatorObjects.ResolutionProto[] resolutionProtos) | ||
{ | ||
Resolution[] localCameraResolutions = new Resolution[resolutionProtos.Length]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var
static Resolution[] ResolutionProtoToNative(CommunicatorObjects.ResolutionProto[] resolutionProtos) | ||
{ | ||
Resolution[] localCameraResolutions = new Resolution[resolutionProtos.Length]; | ||
for (int i = 0; i < resolutionProtos.Length; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var
/// <summary> | ||
/// Converts Resolution protobuf array to C# Resolution array. | ||
/// </summary> | ||
static Resolution[] ResolutionProtoToNative(CommunicatorObjects.ResolutionProto[] resolutionProtos) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make private.
@mmattar Comments addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, @awjuliani. Added 2 tiny comments.
@@ -92,4 +122,4 @@ public override void OnInspectorGUI() | |||
MakeBrainParametersProperty(brainParameters); | |||
serializedObject.ApplyModifiedProperties(); | |||
} | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add new line
@@ -121,4 +144,4 @@ public BrainParameters Clone() | |||
}; | |||
} | |||
} | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add new line
We were not reporting camera resolution information in the demonstration file. This addresses this, and ensures it is printed in the inspector correctly as well.