search

comp345 / risk-game

Warzone game on console implemented using C++
1 stars 0 forks source link

A2 command processor #16

Closed GD-Dheer closed 2 years ago

eyeshield2110 commented 2 years ago

Btw I don't think we should separate the State and Transition classes out of the GameEngine, since we're restricted in what files we are allowed to submit.

eyeshield2110 commented 2 years ago

The main thing that bothers me is that State objects are being handled by the CommandProcessor directly (ex. some function of CommandProcessor pass a State* as parameter). It violates encapsulation, and we're more likely to get errors from misuse of pointers in A3, when adding modification. We can discuss this more in a call later tonight.

eyeshield2110 commented 2 years ago

Another thing: we'll have to move the code from testGameEngine into a testCommandProcessor instead, to avoid merge conflict since Alexander wrote his part into the same function.

laila-chammaa commented 2 years ago

heya, sorry, a lot of the comments aren't addressed (or maybe im blind) are you still working on them?

laila-chammaa commented 2 years ago

im good in noah is good with state+transition being in commandprocessor, u can just hop in the call and ask, and merge if all good from him @GD-Dheer

laila-chammaa commented 2 years ago

altho not sure which branch he wants to merge this into, he might've said into a2-p4? not sure

laila-chammaa commented 2 years ago

@GD-Dheer can u make sure branch a2_archived works with your changes? so try to run the commandProcessorDriver in that branch and if all works as expected (it might not since the merge conflicts were huge), feel free to close this PR (altho maybe u wanna address some of the comments?) and delete the branch. thanks!

laila-chammaa commented 2 years ago

@GD-Dheer can u make sure branch a2_archived works with your changes? so try to run the commandProcessorDriver in that branch and if all works as expected (it might not since the merge conflicts were huge), feel free to close this PR (altho maybe u wanna address some of the comments?) and delete the branch. thanks!

heya @GD-Dheer since i remember during the demo, it wasn't working and u had to switch branches or smth? if im remembering wrong and it works, my bad, just close the PR and delete the branch