Newgame #41

etosch · 2021-04-29T23:30:54Z

mostly kicking off a pull request to get eyes on this.

looking for feedback on how "complete" we want the autogenerated starter code to be.

…ib.rs

… worthwhile though

kclary · 2021-04-30T12:25:43Z

lgtm. but not sure what to look out for.

I tested it and looks like nothing is breaking. pushed the result to this branch: bf04905

Looks like it:

creates the directory and contents for the base rust game code (game.rs, lib.rs, types.rs)
updates the root Cargo.toml and toybox/Cargo.toml
updates/adds Games.toml -- this is new to me, is this the replacement for inventory?
updates toybox/src/lib.rs

etosch · 2021-04-30T12:30:01Z

Yup! Games.toml is replacement for inventory. It can be written do if someone wants to add a game manually.

I also updated the getting started with a new game information in docs.

jjfiv · 2021-04-29T23:32:42Z

Games.toml

@@ -0,0 +1 @@
+games = [["amidar", "amidar", "Amidar"], ["breakout", "breakout", "Breakout"], ["gridworld", "gridworld", "GridWorld"], ["space_invaders", "space_invaders", "SpaceInvaders"], ["pong", "pong", "PongConfig"]]


Could this be a flat array of the capitalized one?

We could call .lowercase() to get the other two?

Will do, hoss.

etosch · 2021-05-05T23:40:08Z

Updated Games.toml format, code generation, and documentation.

…t with tool

etosch · 2021-05-06T22:17:39Z

Just added a --clear command, so if you want to play around with a new game and then undo that, you can nuke it.

etosch · 2021-05-06T22:18:59Z

@kclary you might want to check that this doesn't break any changes for you -- I had to update the module name in space invades, and the main game name in Pong (from PongConfig) to make the autogeneration with @jjfiv 's requested change work.

etosch · 2021-05-06T22:32:59Z

BTW I was going to try to set this up as a cargo subcommand but it's too much of a hassle to get that working. We'll just stick with the instructions for now. Will be ready to merge once the tests are passing.

jjfiv

I think rolling back the change to 'spaceinvaders' is the only blocker here.

jjfiv · 2021-05-07T19:37:37Z

toybox/src/lib.rs

@@ -16,10 +16,10 @@ pub fn get_simulation_by_name(name: &str) -> Result<Box<dyn Simulation>, String>
        "breakout" => Ok(Box::new(breakout::Breakout::default())),
        #[cfg(feature = "gridworld")]
        "gridworld" => Ok(Box::new(gridworld::GridWorld::default())),
-        #[cfg(feature = "space_invaders")]


I'd be OK if we ditched the cfg-feature per game situation. We'd initially envisioned compiling just the games you need, but the rust compiler has gotten faster and we're distributing on pip now...

jjfiv · 2021-05-07T19:38:24Z

ctoybox/tests/test_bad_json.py

@@ -29,7 +29,7 @@ def test_breakout(self):
            self._bad_json_is_recoverable(tb)

    def test_space_invaders(self):
-        with Toybox("space_invaders") as tb:


I vote to restore the tuples so we don't have to break this compat.

jjfiv · 2021-05-07T19:39:18Z

newgame/src/resources/cargo_deps.txt

+serde_json = "*"
+serde_derive = "*"
+lazy_static = "*"
+rand = { version = "0.6.3", default-features = false, features=["std"] }


Can we turn these version numbers into "*" and have it be OK? idk, but it would be nice to be less-specific here...

I copied the other Cargo.tomls here; presumably you chose a specific version for a reason?

Yeah, I got paranoid with rand because they don't consider the specific random number generator to be part of semver... and we really want to be as reproducible as possible with a given seed.

jjfiv · 2021-05-07T19:40:44Z

newgame/src/resources/types_template.txt

+// Put the rest of the structs for your game here
+
+#[derive(Clone, Serialize, Deserialize, JsonSchema)]
+pub struct StateCore {


This is a huge bikeshed, but definitely for new games, I think this will make more sense to new people as "FrameState" rather than the silly "StateCore" that's my fault. #12.

OK to punt on this since I have #12 open.

This should go into another pull request.

etosch · 2021-06-24T15:36:54Z

@kclary you might want to check that this doesn't break any changes for you -- I had to update the module name in space invades, and the main game name in Pong (from PongConfig) to make the autogeneration with @jjfiv 's requested change work.

@kclary -- did this break any of your builds?
@jjfiv -- I'm back working on this now; all checks have passed, so do you have any information on where the build breaks?

kclary · 2021-06-24T15:45:47Z

@etosch I made the branch/build just to check if the tool worked. Haven't made any changes, and I'd be fine to abandon & rerun the latest version. I no longer remember how to check the build -- is that helpful for debugging or more to make sure there are no lingering issues?

etosch · 2021-06-24T16:04:34Z

@etosch I made the branch/build just to check if the tool worked. Haven't made any changes, and I'd be fine to abandon & rerun the latest version. I no longer remember how to check the build -- is that helpful for debugging or more to make sure there are no lingering issues?

@jjfiv was concerned that there were breaking changes. I think the issue he pointed out is that every instance of Toybox("space_invaders") in the other repos will break. Personally, I think that since we aren't under the gun for deadlines right now, we should just push this change, push a new version of the executable to PyPI, and I will be sure to update the Toybox repo to comply with the new naming scheme.

jjfiv · 2021-06-24T16:16:22Z

It was actually trying to use 'inventory' that broke the build: #37

I think this is orthogonal.

jjfiv

Ok with spaceinvaders rename since nobody's really using it yet.

Emma Tosch added 8 commits April 28, 2021 17:30

added newgame to top level workspace

bff28a7

merge commit

741b53c

first steps in newgame utility

3a2d5aa

creating all the files; just need to modify game rust and top level l…

ccbb913

…ib.rs

adding a new game should now work. next step: removing a game

1217faf

updating tomls

9783b12

caught an error

2ec4f9b

still working to get auto-generated code to compile; not sure this is…

cb3699f

… worthwhile though

etosch requested review from jjfiv and kclary April 29, 2021 23:30

jjfiv reviewed Apr 30, 2021

View reviewed changes

Emma Tosch added 2 commits May 5, 2021 19:13

updated format of Games.toml

6d03e20

auto generated lib.rs now compiles

a54626e

Emma Tosch added 3 commits May 5, 2021 19:42

updated README

aba7a01

added remove capability, needed to update other games to be consisten…

3cc041e

…t with tool

included statement in docs about removing

b69c38f

updated tests to point to the right formatting of space invaders

3e629b9

missed one

b98e783

jjfiv requested changes May 7, 2021

View reviewed changes

jjfiv mentioned this pull request May 15, 2021

pyO3 #42

Merged

etosch mentioned this pull request Jun 24, 2021

update all rand verisons to have latest instead of being pinned. #43

Open

kclary approved these changes Jun 24, 2021

View reviewed changes

jjfiv approved these changes Jun 24, 2021

View reviewed changes

etosch merged commit 6ced4dc into main Jun 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Newgame #41

Newgame #41

etosch commented Apr 29, 2021

kclary commented Apr 30, 2021 •

edited

Loading

etosch commented Apr 30, 2021

jjfiv Apr 29, 2021

etosch May 5, 2021

etosch commented May 5, 2021

etosch commented May 6, 2021

etosch commented May 6, 2021

etosch commented May 6, 2021

jjfiv left a comment

jjfiv May 7, 2021

jjfiv May 7, 2021

jjfiv May 7, 2021

etosch Jun 24, 2021

jjfiv Jun 24, 2021

jjfiv May 7, 2021

etosch Jun 24, 2021

etosch commented Jun 24, 2021

kclary commented Jun 24, 2021

etosch commented Jun 24, 2021

jjfiv commented Jun 24, 2021

jjfiv left a comment

		@@ -0,0 +1 @@
		games = [["amidar", "amidar", "Amidar"], ["breakout", "breakout", "Breakout"], ["gridworld", "gridworld", "GridWorld"], ["space_invaders", "space_invaders", "SpaceInvaders"], ["pong", "pong", "PongConfig"]]

Newgame #41

Newgame #41

Conversation

etosch commented Apr 29, 2021

kclary commented Apr 30, 2021 • edited Loading

etosch commented Apr 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etosch commented May 5, 2021

etosch commented May 6, 2021

etosch commented May 6, 2021

etosch commented May 6, 2021

jjfiv 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

Choose a reason for hiding this comment

etosch commented Jun 24, 2021

kclary commented Jun 24, 2021

etosch commented Jun 24, 2021

jjfiv commented Jun 24, 2021

jjfiv left a comment

Choose a reason for hiding this comment

kclary commented Apr 30, 2021 •

edited

Loading