Skip to content
This repository has been archived by the owner on Jan 17, 2024. It is now read-only.

Issue 07 create game struct #10

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

Evan-Honey
Copy link
Contributor

No description provided.

Copy link
Contributor

@mollyli-honey mollyli-honey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I have a couple of minor comment.

type Game struct {
players []Player
enemies []Enemy
goals []Goal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goals? not Crystals? Am I mistaken that the objective is to get all crystals?

Copy link
Contributor Author

@Evan-Honey Evan-Honey Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i suppose I was just trying to make the name generic. like, when we skin it, enemies might become "ghosts" or "monsters", in the same way that goals are crystals

@@ -0,0 +1,10 @@
package state

type Game struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the game also have some kind of boundaries for where the entities can be located? I figure that we should just display a static board size, so knowing what that size is will be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, will plan to add boundaries in the next PR

if (goty != 0.0) {
t.Errorf("Initialized enemy did not have default y velocity 0.0")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default geometry test? Also for your players tests.

On a related note too, should the default radius be 1 instead of the built in value of 0? It seems weird to have an entity with no size.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default geo test is a good idea, i will add

agreed that the default radius should be 1, but Go does not have a way to set a default for a struct member that isn't the same as that native type's default. There's no constructor or anything either.

To get non-0 defaults we will need to establish a pattern of our own "constructor" methods and make a habit of calling them. From what I've read, Init() is a common name for struct initializers. I think that's out of the scope of this PR, but would go well with the next one containing default boundaries

t.Errorf("X not retrieved correctly got %v, wanted %v ", loc.x, 1.0)
}
if (loc.y != -1.5) {
t.Errorf("Y not retrieved correctly got %v, wanted %v ", loc.x, 1.0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loc here is incorrect, should be -1.5

t.Errorf("X not retrieved correctly got %v, wanted %v ", vel.x, 1.0)
}
if (vel.y != -1.5) {
t.Errorf("Y not retrieved correctly got %v, wanted %v ", vel.x, 1.0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong loc here too

package state

type Player struct {
location Location
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need something here to represent the player's controllable actions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, will add in a another PR

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants