-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add custom package settings #5027
Conversation
@@ -0,0 +1,74 @@ | |||
using System.Linq; |
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.
This might be a dumb question, but why do we have a BuildSettingsProvider if none of the settings are used in a build? (They are all Editor only settings?)
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.
they are used in builds, if you don't want to train or even try to connect to a trainer in a game that's being shipped you'd want to know that in your build.
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.
They are used in builds. If you don't want to train or even try to connect to a trainer in a game that's being shipped, you'd want to know that in your build.
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 the use case it to remove the command line argument listeners on the build? Because a build will not try to listen to a trainer unless a specific command line argument is passed.
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.
Good question. Originally I made a player training port but that doesn't seem to make much sense so I removed that.
But I still think there's chance that we'll add settings that is used in player (analytics? academy stepping?)
private static void ClearPlayerSettingsDirtyFlag() | ||
{ | ||
#if UNITY_2019_OR_NEWER | ||
var settings = Resources.FindObjectsOfTypeAll<PlayerSettings>(); |
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.
PlayerSettings? I thought this would be an MLAgentsSettings.
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.
We're injecting our settings into the preload assets in PlayerSettings and these method is to detect/clear the dirty flag of PlayerSettings before/after we put in our settings. So PlayerSettings here is correct.
#else | ||
internal static void InitializeInPlayer() | ||
{ | ||
s_Settings = Resources.FindObjectsOfTypeAll<MLAgentsSettings>().FirstOrDefault() ?? ScriptableObject.CreateInstance<MLAgentsSettings>(); |
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.
s_Settings = Resources.FindObjectsOfTypeAll<MLAgentsSettings>().FirstOrDefault() ?? ScriptableObject.CreateInstance<MLAgentsSettings>(); | |
Settings = Resources.FindObjectsOfTypeAll<MLAgentsSettings>().FirstOrDefault() ?? ScriptableObject.CreateInstance<MLAgentsSettings>(); |
?
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.
good catch. The callback in Settings setter is for refreshing in the editor so should not really matter though.
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.
Can you explain in comments why we have both Settings
and s_Settings
?
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 comments on the getter/setter
class MLAgentsSettings : ScriptableObject | ||
{ | ||
[SerializeField] | ||
private bool m_ConnectTrainer = true; |
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.
There were other settings that we talked about adding later - should we group/nest these now so that we don't have to go through hoops to move them around in the future?
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 can group them into Training Port or Trainer Settings.
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.
@chriselion by group/nest these do you mean nesting them in the code (like a subclass in MLAgentsSettings) or just grouping in editor?
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.
Either way - I think using a subclass in MLAgentsSettings makes the grouping in the editor more automatic.
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 guess if we always need to manually layout the fields in MLAgentsSettingsProvider.OnGUI
, it's less important. But it might let us just add the "top level" settings (like TrainerSettings) and not have to worry about new fields when we add them.
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.
Can you add some user-facing documentation on how to enable these, and how to have different settings for training/inference?
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 it looks good. I'd like to see some Edit mode test coverage on these files. After that we can
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.
(Pending other feedback)
…echnologies/ml-agents into develop-package-settings
Tests added. I placed it under DevProject to have a place to create settings asset on the fly during tests. |
2018 tests are failing and are expected since I removed the 2019_OR_NEWER flags. |
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.
Proposed change(s)
Add customizable ML-Agents settings in project settings.
Users can create asset for their own projects.
Settings are accessible in editor and player build.
Multiple settings assets are allowed (displayed in drop-down list).
Currently only contains
Connect to Trainer
andEditor Training Port
. More can be added later.Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
Types of change(s)
Checklist
Other comments