Adds keyboard controls #16
Conversation
Arrow keys = Vince AWSD = Jerry JIKL = Bart
|
@Tsmith18256 Please code review! |
| @@ -0,0 +1,3 @@ | |||
| export default class GameEvent { | |||
|
|
|||
| } | |||
There was a problem hiding this comment.
Might of been thinking I needed to add stuff the base class. I will change this to an interface.
|
|
||
| get gameObject() { | ||
| return this._gameObject; | ||
| } |
There was a problem hiding this comment.
Is _gameObject ever expected to change internally after the constructor? If not, you can change line 6 to be readonly gameObject: GameObject; and not have to do a getter. It will let you do an initial assignment in the constructor.
There was a problem hiding this comment.
Good point read only is better. Will do this.
| this.gameObjects = []; | ||
| Game.inputControllers = []; |
There was a problem hiding this comment.
Every time we create a game it resets the static inputControllers?
There was a problem hiding this comment.
This is a bug will remove this.
We need to rethink input controllers in due time. Refactor as you see fit when you are working.
| if (twoDContext == null) { | ||
| throw new Error('Could not get the context from the canvas'); | ||
| } | ||
| this.context = twoDContext!; |
There was a problem hiding this comment.
What does this ! do? (I actually have no idea)
There was a problem hiding this comment.
It is unnecessary because of the null check. But if I hadn't checked for null it would force set context which does not allow null.
It can be removed.
There was a problem hiding this comment.
That is awesome! Thank you... I was trying to do that the other day when I first did the TypeScript migration and couldn't figure it out.
| return Game._viewPortWidth; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is confusing here. Having the getters and setters in a nested class but then accessing the private properties as part of the parent class.
There was a problem hiding this comment.
Going to leave this. But refactor as you see fit. Thinking is we need to give static access to variables that can only be set inside Game.
Thought it gave it nice name spacing having it accessed via inner class.
There was a problem hiding this comment.
We should make it less single operation than this. What if someone wanted two Game objects running?
There was a problem hiding this comment.
Yeah I was kinda thinking about that idea too. We probably only ever want one game, but that doesn't mean it should break if somebody makes 2
| // TODO: This needs improvement. Angle changes needs to be smoother. | ||
| private calculateAngle() { | ||
|
|
||
| var xFactor = 0; |
There was a problem hiding this comment.
Gotta bunch of vars in this commit. Just do a find all and make these let. It's hard getting used to using let, but it's a lot nicer
|
|
||
| static handleEvent(game: Game, event: GameEvent | undefined): void { | ||
|
|
||
| if ((<any>event).constructor.name == (<any>SpawnEvent).name) { |
There was a problem hiding this comment.
For casting, use as. I read on the difference one time and apparently there are issues that can arise from this notation but not as or something like that.
| static getByType <T> (type: any, array:Array<T>): T | null { | ||
| var object: T | null = null; | ||
| array.forEach((e) => { | ||
| if ((<any>e).constructor.name == type.name) object = e; |
There was a problem hiding this comment.
Same as above for casting
| } | ||
|
|
||
| static getByType <T> (type: any, array:Array<T>): T | null { | ||
| var object: T | null = null; |
There was a problem hiding this comment.
I thought we were going only undefined and no null
|
|
||
| if (this.x < 0 || this.y < 0 || | ||
| this.x > Game.Info.viewPortWidth || | ||
| this.y > Game.Info.viewPortHeight) { |
There was a problem hiding this comment.
This is pretty unclear at first glance with all these conditions, can we break it down to variables for readability? Maybe like:
let isWithinScreenX: boolean = this.x >= 0 && this.x <= Game.Info.viewPortWidth;
let isWithinScreenY: boolean = this.y >= 0 && this.y <= Game.Info.viewPortHeight;
if (!isWithinScreenX || !isWithinScreenY) {
this.alive = false;
}
There was a problem hiding this comment.
NumberTools.isInRect. Will change
|
Going to address comments before pull request is merged. |
Arrow keys = Vince
AWSD = Jerry
JIKL = Bart