-
Notifications
You must be signed in to change notification settings - Fork 3
[REG-2217] Send available segments to dashboard #368
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
nAmKcAz
left a comment
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.
Mostly looks good, a few small requests to rename some methods and do an error case check.
| SendAvailableSequences(); | ||
| } | ||
|
|
||
| private void LoadAvailableSegments() |
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.
based on the method naming, I don't like how these methods implicitly send the status as well, maybe rename them to better represent that they do that ?
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.
Updated the name to indicate that this does send segments after loading them
| { | ||
| botManager.OnBeginPlaying(); | ||
| } | ||
| var playbackController = FindObjectOfType<BotSegmentsPlaybackController>(); |
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 is fine for the initial POC, but we'll need to decide when we're doing away with the in game overlay
for now.. they could easily conflict
either way, we should probably at least check here that so segment/sequence is running in case the client UI is somehow out of sync
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.
Play now only works if something isn't already playing
| AvailableSegments, | ||
|
|
||
| // info about the currently-running sequence (or segment) | ||
| ActiveSequence, |
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.
Future (non-blocking), I have a task to create a status manager to better isolate the current status of which segment we're on, what the current error state is, whether exploration is active, and the current exploration error state. We'll want to be able to sync that to the client, but I wanted to put this comment here for visibility into what is coming
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.
Yeah, would love to adopt Addison's mockups into this dashboard. We have a lot more freedom here to show detailed information about the current segment, progression through a sequence, any errors, etc.
What has been done
Out of Scope
Interacting with segments outside of playing + stopping them. We will be getting more interaction with segments (creation and editing) in upcoming PRs.
Find the pull request instructions here
Every reviewer and the owner of the PR should consider these points in their request (feel free to copy this checklist so you can fill it out yourself in the overall PR comment)