[REG-2168] Support a bot action to 'ResetForNextTest' .. and 'RestartGame' and 'QuitGame' #351

nAmKcAz · 2024-11-06T18:34:38Z

Adds support for 'RestartGame' and 'QuitGame' Bot Actions
- ResetForNextTest will run the registered restart impl, OR default to running the restart game action
- Restart game will restart the game in the editor, or will load a custom impl of what to do in a standalone runtime
- Quit game will stop the game in the editor, or exit the application in a standalone runtime
Also, refactors the package membership of criteria and actions to make them easier to find in the explorer .. this should be mostly contained to the first commit if you want to avoid the noise

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)

…ntations

nAmKcAz · 2024-11-06T18:42:30Z

...gression.unity.bots/Runtime/Scripts/StateRecorder/BotSegments/Models/IRGRestartGameAction.cs

@@ -0,0 +1,10 @@
+namespace StateRecorder.BotSegments.Models
+{
+    public interface IRGRestartGameAction


New interface for custom restart implementations

nAmKcAz · 2024-11-06T18:42:42Z

...bots/Runtime/Scripts/StateRecorder/BotSegments/Models/BotActions/RestartGameBotActionData.cs

+namespace RegressionGames.StateRecorder.BotSegments.Models.BotActions
+{
+    [Serializable]
+    public class RestartGameBotActionData : IBotActionData


New restart game bot action

nAmKcAz · 2024-11-06T18:42:51Z

...ty.bots/Runtime/Scripts/StateRecorder/BotSegments/Models/BotActions/QuitGameBotActionData.cs

+namespace RegressionGames.StateRecorder.BotSegments.Models.BotActions
+{
+    [Serializable]
+    public class QuitGameBotActionData : IBotActionData


New quit game bot action

nAmKcAz · 2024-11-06T18:43:50Z

src/gg.regression.unity.bots/Runtime/Scripts/StateRecorder/SdkApiVersion.cs

+        /**
+         * <summary> Add restart game bot action</summary>
+         */
+        public const int VERSION_25 = 25;


new sdk api version

nAmKcAz · 2024-11-06T18:44:13Z

...nity.bots/Runtime/Scripts/StateRecorder/BotSegments/JsonConverters/BotActionJsonConverter.cs

@@ -47,6 +49,12 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist
                    case BotActionType.Mouse_ObjectDetection:
                        data = jObject["data"].ToObject<CVObjectDetectionMouseActionData>(serializer);
                        break;
+                    case BotActionType.RestartGame:


handle the new actions when reading in bot segment json

addisonbgross

Looks good overall. Just 1 non-blocking Q

...bots/Runtime/Scripts/StateRecorder/BotSegments/Models/BotActions/RestartGameBotActionData.cs

batu

This looks good!

vontell

Very nice!! I only had one question about default behavior in the editor for the restart mode, but looks good otherwise. Will resolve that quickly once I get an answer.

...bots/Runtime/Scripts/StateRecorder/BotSegments/Models/BotActions/RestartGameBotActionData.cs

vontell · 2024-11-07T14:57:19Z

...bots/Runtime/Scripts/StateRecorder/BotSegments/Models/BotActions/RestartGameBotActionData.cs

+                // un-register our listener
+                EditorApplication.playModeStateChanged -= PlayGameInEditor;
+                // just finished playing ... start it back up
+                EditorApplication.isPlaying = true;


I'm glad this actually works!

vontell · 2024-11-07T14:59:12Z

...bots/Runtime/Scripts/StateRecorder/BotSegments/Models/BotActions/RestartGameBotActionData.cs

+                RGDebug.LogInfo($"Restarting the game in the editor...");
+                _error = null;
+                error = _error;
+                // register our hook so that the game will start right back up
+                EditorApplication.playModeStateChanged += PlayGameInEditor;
+                // stop the game in the editor
+                EditorApplication.isPlaying = false;
+                return true;


Does this mean in editor we only allow a restart that completely restarts the game? I might be misreading this but it seems like the _action is only considered outside of editor?

correct.. as of right now..

Editor - 'restart' actually restarts the game

External Runtime -

Default - 'restart' loads scene 0, which is as close as we can get to restarting the game without ending the process runtime; this restarts the game to the starting scene, but potentially without cleaning up many things as noted in the comments in the code... we could explore making this much harsher... like.. destroy all objects (event don't destroy on load) before loading scene 0 which would make it much closer to the editor behaviour... however, this still wouldn't kill background threads, or unity task running on entities/etc

Custom - 'restart' runs whatever commands are in your custom impl

I suppose that was fine for now, but I was thinking that even in editor it would run the custom impl, since my restart logic might just be "respawn at the respawn point and remove all enemies". But on second thought, it might be good to allow the custom action in the editor because this means the only way for you to test the custom logic is to keep rebuilding the game, which could take a long time.

Apparently we weren't on the same page at all when initially discussing this task.

It seems you wanted an action to define 'reset for next test', but I got 'restart game' out of the initial discussion.

I guess I'll start over .

Start over? I'm confused as I feel like all the code here aligns with exactly what I was thinking, including the default of loading the initial scene. From what I see here, this aligns almost exactly with my thinking but I had in my mind one small extension, but if that extension is not small I'd rather have this PR in.

nAmKcAz · 2024-11-07T18:16:30Z

@vontell I debated whether to start a new PR for this, but I re-opened the existing. There is now another new action ResetForNextTest which should fulfill what you were looking for, but I am leaving the RestartGame and QuitGame actions in here as they feel safer / more appropriate to me.

You can now do what you intended with a partial reset for the next test without restarting the game, but I don't really agree with this function being different than restart game as there is generally no 1 single action you can take to safely reset the game's state other than fully restarting it.

I feel that this is only going to add confusion for users on how to implement 'reset' safely for multiple test scenarios.

nAmKcAz · 2024-11-07T18:47:25Z

src/gg.regression.unity.bots/Runtime/Scripts/StateRecorder/BotSegments/Models/BotActionType.cs

@@ -11,6 +11,9 @@ public enum BotActionType
        Mouse_CVImage,
        Mouse_CVText,
        Mouse_ObjectDetection,
+        RestartGame,
+        QuitGame,
+        ResetForNextTest


I would rather name this ResetGame, but that is too close to RestartGame to be completely clear.

Ultimately, I don't like Reset existing separately from Restart, but that was the only way to fufill @vontell 's ask that the custom impl be testable in the editor if desired.

vontell · 2024-11-08T15:29:05Z

Approving but happy to discuss this further at some point, I have a feeling what I'm about to write out via text is better explained over a quick huddle at some point.

I think where the confusion came from is what we were assuming would go in this RestartGame custom impl, and I am fine going with whatever you were expecting inside of that... but anyhow, I was expecting the result of my message to not be an entirely new type, it was simply arguing that the custom impl would run in the editor - I guess I am missing some nuance around why we couldn't just delete that UNITY_EDITOR conditional in the ResetGame handler, that was what I thought would be the change (i.e. just making sure if I provide a custom impl, it uses that in editor so I can make sure that works before building). I suppose that means that the sentence "Ultimately, I don't like Reset existing separately from Restart, but that was the only way to fufill @vontell 's ask that the custom impl be testable in the editor if desired" is the part I don't understand, I'll dive a little deeper into the code though and see if I can figure that out for myself

nAmKcAz · 2024-11-08T15:33:43Z

@vontell I fundamentally dis-agree with using the custom impl in the editor. The BEST and only truly clean option is to restart the game. In the editor, this is trivial to achieve and thus is what should happen. I don't like there being a reset and restart at all. I want just restart, as doing anything other than fully restarting the game is less than ideal. Unfortunately in a built runtime, the options for restarting the game are limited. That is why I had the initial discussion with you about possibly starting a new process for the game runtime as our preferred runtime option, but that didn't work well in docker. I would still prefer that to what is here now though.

vontell · 2024-11-08T15:40:45Z

Hmmmm I think I see where the disconnect is, I suppose my question would be what exactly we imagine going into this custom impl for RestartGame.

In terms of custom impl in the editor, I think we are actually in agreement there, I am pretty ok with taking a stance that we should be doing the best possible restart to ensure things are not getting messed up. The only reason I was pushing for the above (this reasoning slightly shifted as we conversed) is that I really don't like the idea of code that I can't test until after the build is made, it just makes iterating for the developer tough. But then again, maybe that goes back to what kind of code is going to be inside this custom impl, maybe the code you are imagining is very simple and therefore that wouldn't be as much of a concern

Also just to demonstrate what I thought my comment was going to result in, here is the edit to the code that I thought would be made, I suppose I don't know why this change wouldn't work (this was a quick edit, small pieces of it may not be correct fully): 16e4823

nAmKcAz added 5 commits November 6, 2024 09:39

Refactor package location of actions and criteria

d3c2d5c

[REG-2168] Create a new RestartGame action that allows custom impleme…

e91cfe5

…ntations

Merge remote-tracking branch 'origin/main' into zack/restart-game-action

107b240

Implement better editor restart

b54751d

Document future todos

24b1ffa

nAmKcAz requested a review from vontell as a code owner November 6, 2024 18:34

nAmKcAz added 2 commits November 6, 2024 13:40

Fix usings for new packages in tests

fb12f63

Update IRGRestartGameAction.cs

a389b54

nAmKcAz commented Nov 6, 2024

View reviewed changes

Update SdkApiVersion.cs

d689e5b

nAmKcAz commented Nov 6, 2024

View reviewed changes

nAmKcAz requested review from batu and abeizer November 6, 2024 18:47

Update code docs

96db86b

nAmKcAz changed the title ~~Zack/restart game action~~ [REG-2168] Support a bot action to 'RestartGame' Nov 6, 2024

addisonbgross approved these changes Nov 6, 2024

View reviewed changes

...bots/Runtime/Scripts/StateRecorder/BotSegments/Models/BotActions/RestartGameBotActionData.cs Outdated Show resolved Hide resolved

Update RestartGameBotActionData.cs

aa817d5

batu approved these changes Nov 6, 2024

View reviewed changes

vontell requested changes Nov 7, 2024

View reviewed changes

nAmKcAz requested a review from vontell November 7, 2024 15:45

nAmKcAz closed this Nov 7, 2024

Add ResetForNextTest action + fix a few quirks in restart

f5cee87

nAmKcAz reopened this Nov 7, 2024

nAmKcAz changed the title ~~[REG-2168] Support a bot action to 'RestartGame'~~ [REG-2168] Support a bot action to 'ResetGame' .. and 'RestartGame' and 'QuitGame' Nov 7, 2024

nAmKcAz changed the title ~~[REG-2168] Support a bot action to 'ResetGame' .. and 'RestartGame' and 'QuitGame'~~ [REG-2168] Support a bot action to 'ResetGameForNextTest' .. and 'RestartGame' and 'QuitGame' Nov 7, 2024

nAmKcAz changed the title ~~[REG-2168] Support a bot action to 'ResetGameForNextTest' .. and 'RestartGame' and 'QuitGame'~~ [REG-2168] Support a bot action to 'ResetForNextTest' .. and 'RestartGame' and 'QuitGame' Nov 7, 2024

nAmKcAz requested review from vontell and removed request for vontell November 7, 2024 18:16

nAmKcAz commented Nov 7, 2024

View reviewed changes

vontell approved these changes Nov 8, 2024

View reviewed changes

Do it aaron's way ...

66317c2

nAmKcAz merged commit 68ebc00 into main Nov 8, 2024
2 checks passed

nAmKcAz deleted the zack/restart-game-action branch November 8, 2024 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REG-2168] Support a bot action to 'ResetForNextTest' .. and 'RestartGame' and 'QuitGame' #351

[REG-2168] Support a bot action to 'ResetForNextTest' .. and 'RestartGame' and 'QuitGame' #351

nAmKcAz commented Nov 6, 2024 •

edited

Loading

nAmKcAz Nov 6, 2024

nAmKcAz Nov 6, 2024

nAmKcAz Nov 6, 2024

nAmKcAz Nov 6, 2024

nAmKcAz Nov 6, 2024

addisonbgross left a comment

batu left a comment

vontell left a comment

vontell Nov 7, 2024

vontell Nov 7, 2024

nAmKcAz Nov 7, 2024

vontell Nov 7, 2024

nAmKcAz Nov 7, 2024

vontell Nov 7, 2024

nAmKcAz commented Nov 7, 2024

nAmKcAz Nov 7, 2024

vontell commented Nov 8, 2024

nAmKcAz commented Nov 8, 2024 •

edited

Loading

vontell commented Nov 8, 2024 •

edited

Loading

[REG-2168] Support a bot action to 'ResetForNextTest' .. and 'RestartGame' and 'QuitGame' #351

[REG-2168] Support a bot action to 'ResetForNextTest' .. and 'RestartGame' and 'QuitGame' #351

Conversation

nAmKcAz commented Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addisonbgross left a comment

Choose a reason for hiding this comment

batu left a comment

Choose a reason for hiding this comment

vontell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nAmKcAz commented Nov 7, 2024

Choose a reason for hiding this comment

vontell commented Nov 8, 2024

nAmKcAz commented Nov 8, 2024 • edited Loading

vontell commented Nov 8, 2024 • edited Loading

nAmKcAz commented Nov 6, 2024 •

edited

Loading

nAmKcAz commented Nov 8, 2024 •

edited

Loading

vontell commented Nov 8, 2024 •

edited

Loading