-
Notifications
You must be signed in to change notification settings - Fork 3
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
[REG-2176] Skip graphics capture if graphics not available #354
Conversation
src/gg.regression.unity.bots/Runtime/Scripts/StateRecorder/ScreenshotCapture.cs
Outdated
Show resolved
Hide resolved
src/gg.regression.unity.bots/Runtime/Scripts/StateRecorder/ScreenshotCapture.cs
Outdated
Show resolved
Hide resolved
src/gg.regression.unity.bots/Runtime/Scripts/StateRecorder/GameFacePixelHashObserver.cs
Outdated
Show resolved
Hide resolved
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.
Nothing to add, pending Zack's comments
src/gg.regression.unity.bots/Runtime/Scripts/StateRecorder/ScreenshotCapture.cs
Outdated
Show resolved
Hide resolved
@vontell I see you re-tagged me for review, but that seems to conflict with your comments above about still testing this. I'd prefer to only re-review this one time so please request again once all testing/changes are done. |
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.
Commenting so slack bot stops pinging me until this is ready
Ok this should finally be ready for review. Took a while to get to the cleanest solution, but I believe this would be it... TLDR; my original check would stop both the screenshot read and onCompletion handlers from being called ever, meaning that the action handlers that write the ticks to disk also never get called. I've left that check there as a safety precaution, but the main change is that now the onCompletion handlers are called right away in the case when graphics are not available. |
src/gg.regression.unity.bots/Runtime/Scripts/StateRecorder/ScreenshotCapture.cs
Outdated
Show resolved
Hide resolved
src/gg.regression.unity.bots/Runtime/Scripts/StateRecorder/ScreenshotCapture.cs
Outdated
Show resolved
Hide resolved
@RG-nAmKcAz edits made 👍 |
Also just tested on Windows, this works there as well |
Sorry this took so long, took a while to iterate since building the game fully takes 15 minutes or so, and my approach of doing an early conditional break was acting up, so went with the more safe approach of just wrapping it in an
if
statement. Tested this in BossRoom and seems to work, and in our user's game, the game itself still doesn't work, but the graphics error is no longer thrown. There may be more changes to come, TBD, but this gets us partway there.