-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Release v0.7 disable gRPC on non supported platforms #1743
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
@@ -2,6 +2,9 @@ | |||
// Generated by the protocol buffer compiler. DO NOT EDIT! | |||
// source: mlagents/envs/communicator_objects/unity_to_external.proto | |||
// </auto-generated> | |||
|
|||
# if UNITY_EDITOR || UNITY_STANDALONE_WINDOWSWINDOWS || UNITY_STANDALONE_OSX || UNITY_STANDALONE_LINUX |
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.
UNITY_STANDALONE_WINDOWSWINDOWS looks like a typo
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.
Will fix
protobuf-definitions/README.md
Outdated
@@ -26,3 +26,14 @@ On Windows & Linux: [See here](https://github.com/google/protobuf/blob/master/sr | |||
1. Install pre-requisites. | |||
2. Un-comment line 4 in `make.bat`, and set to correct Grpc.Tools sub-directory. | |||
3. Run `make.bat` | |||
4. In the generated `UnityToExternalGrpc.cs` file in the `CommunicatorObjects` folder, you will need to add on top of the file the line |
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.
Say where CommunicatorObjects
folder is (i.e. in UnitySDK).
Also change "to add on top of the file the line"
to
"to add the following to the beginning of the file"
protobuf-definitions/README.md
Outdated
```csharp | ||
# if UNITY_EDITOR || UNITY_STANDALONE_WIN || UNITY_STANDALONE_OSX || UNITY_STANDALONE_LINUX | ||
``` | ||
and at the bottom of the file the line |
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.
"and the following line to the end"
protobuf-definitions/README.md
Outdated
```csharp | ||
#endif | ||
``` | ||
This is to make sure the Grpc library is not used on platforms that do not support training. |
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.
Explain why this is needed.
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.
You mean more than the current explanation ? What is missing in it ? I say It is to make sure the Grpc library is not used on platforms that do not support training
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.
Added a few comments.
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.
looks good now
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.
Looks good to me.
…ies#1743) * Fix for GRPC, need documentation * Edits * typo * Fixes * Missing typo * Modified the documentation * Updated the documentation
This would be a quick fix for disabling training on unsupported platforms.
Might still need to document that if the user wants to recompile the protobuf objects, they will need to edit the generated files.
I tested it on UWP