Python side changes to the SideChannel redesign #3826
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vincentpierre
changed the title
|
Python side changes to #3807 |
chriselion
reviewed
Apr 22, 2020
| """ | ||
|
|
||
| class ConfigurationType(IntEnum): | ||
| SCREEN = (0,) |
There was a problem hiding this comment.
no commas (these are 1-tuples, not ints)
There was a problem hiding this comment.
(still the same formatting bug I am seeing)
Will change and be more careful
chriselion
reviewed
Apr 22, 2020
| /// <summary> | ||
| /// Side channel that supports modifying attributes specific to the Unity Engine. | ||
| /// </summary> | ||
| internal class EngineConfigurationChannel : SideChannel | ||
| { | ||
| private enum ConfigurationType : int | ||
| { | ||
| Screen = 0, |
There was a problem hiding this comment.
"Resolution" or "ScreenResolution"?
There was a problem hiding this comment.
ScreenResolution it is
chriselion
reviewed
Apr 22, 2020
| * `height`: Defines the height of the display. (Must be set alongside width) | ||
| * `quality_level`: Defines the quality level of the simulation. | ||
| * `time_scale`: Defines the multiplier for the deltatime in the simulation. If set to a higher value, time will pass faster in the simulation but the physics may perform unpredictably. | ||
| * `target_frame_rate`: Instructs simulation to try to render at a specified frame rate. |
There was a problem hiding this comment.
Add "capture_frame_rate" too?
There was a problem hiding this comment.
Missed it. Thanks.
chriselion
reviewed
Apr 22, 2020
| """ | ||
|
|
||
| class ConfigurationType(IntEnum): | ||
| SCREEN = (0,) |
There was a problem hiding this comment.
Rename to "RESOLUTION" or "SCREEN_RESOLUTION"?
chriselion
reviewed
Apr 22, 2020
| var captureFrameRate = msg.ReadInt32(); | ||
| Time.captureFramerate = captureFrameRate; | ||
| break; | ||
| } |
There was a problem hiding this comment.
I left a comment in the other PR in EnvironmentParametersChannel.OnMessageReceived() - we should be consistent with how we handle unknown values. I don't think we should throw (otherwise it's not forwards compatible) but we should either do nothing (like here) or warn.
There was a problem hiding this comment.
I can send a warning.
chriselion
reviewed
Apr 22, 2020
ml-agents-envs/mlagents_envs/side_channel/engine_configuration_channel.py
Show resolved
Hide resolved
chriselion
reviewed
Apr 22, 2020
| msg.write_float32(time_scale) | ||
| msg.write_int32(target_frame_rate) | ||
| super().queue_message_to_send(msg) | ||
| if width is not None and height is not None: |
There was a problem hiding this comment.
Raise an exception is one is None and the other isn't.
vincentpierre
added a commit
that referenced
this pull request
Apr 23, 2020
* Make EnvironmentParameters a first-class citizen in the API Missing: Python conterparts and testing. * Minor comment fix to Engine Parameters * A second minor fix. * Make EngineConfigChannel Internal and add a singleton/sealed accessor * Make StatsSideChannel Internal and add a singleton/sealed accessor * Changes to SideChannelUtils - Disallow two sidechannels of the same type to be added - Remove GetSideChannels that return a list as that is now unnecessary - Make most methods except (register/unregister) internal to limit users impacting the “system-level” side channels - Add an improved comment to SideChannel.cs * Added Dispose methods to system-level sidechannel wrappers - Specifically to StatsRecorder, EnvironmentParameters and EngineParameters. - Updated Academy.Dispose to take advantage of these. - Updated Editor tests to cover all three “system-level” side channels. Kudos to Unit Tests (TestAcademy / TestAcademyDispose) for catching these. * Removed debub log. * Back-up commit. * Revert "Back-up commit." This reverts commit f81e835. * key changes to wrapper classes made the wrapper classes non-singleton (but internal constructors) made EngineParameters internal * Re-enabled the option to add multiple side channels of the same type * Fixed example env * Add an enum flag to the EnvParamsChannel * Adding .cs.meta files * Update engine config side channel Removed unnecessary accessors Made capture frame rate a parameter * Rename SideChannelUtils —> SideChannelsManager * PR feedback * Minor PR feedback. * Python side changes to the SideChannel redesign (#3826) * Modified the EngineConfig to send one message per field * Created the Python Environment Parameters Channel and hooked it in * Make OnMessageReceived protected * addressing comments * [Side Channels] Edited the documenation and renamed a few things (#3833) * Edited the documetation and renamed a few things * addressing comments * Update docs/Python-API.md Co-Authored-By: Chris Elion <[email protected]> * Update com.unity.ml-agents/CHANGELOG.md Co-Authored-By: Chris Elion <[email protected]> * Removing unecessary migrating line Co-authored-by: Chris Elion <[email protected]> * Addressing renaming comments * Removing the EngineParameters class Co-authored-by: Vincent-Pierre BERGES <[email protected]> Co-authored-by: Chris Elion <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed change(s)
Switch from using floatproperties to the new environment parameters side channel
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
Types of change(s)
Checklist
Other comments