-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 ISensor.Update() #2852
Add ISensor.Update() #2852
Conversation
academyInitializeMethod?.Invoke(aca, new object[] { }); | ||
|
||
// Step the agent | ||
agent1.RequestDecision(); |
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.
Unfortunately this doesn't fail without the fix in place. Might need to discuss a better way to get the test Agent to act like it's in inference mode.
@@ -35,15 +36,16 @@ void Update() | |||
/// <summary> | |||
/// Creates demonstration store for use in recording. | |||
/// </summary> | |||
void InitializeDemoStore() | |||
public void InitializeDemoStore(IFileSystem fileSystem = null) |
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.
Allow for the mock file system to be injected. Could keep this private and do it by reflection, but not sure if that's necessary.
@@ -71,14 +73,22 @@ public void WriteExperience(AgentInfo info) | |||
m_DemoStore.Record(info); | |||
} | |||
|
|||
public void Close() |
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.
Should this happen on disable instead? Need it for testing to make sure the write stream is flushed,
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.
would you move the initialization to OnEnable?
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.
It's already done lazily in Update()
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.
should it be set to null
after it's closed? Is there a use case for stopping recording during play mode and resuming?
public DemonstrationStore() | ||
{ | ||
m_FileSystem = new FileSystem(); | ||
if (fileSystem != null) |
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.
Combined these.
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.
Overall looks good. Just unsure of when the demonstration recorder should be open/closed/set to null
This add an ISensor.Update() method and implements it for the cases where it makes sense. This prevents observations from being cleared before they can be sent to the DemonstrationRecorder.