Implement GameInteractor & move CrowdControl and console commands to it #2358
Conversation
Chore/game interactor
|
@aMannus although I left you quite a number of comments -- I want to say this is looking pretty cool overall! I can tell a lot of work and thought went into it and I think its leading to a nicer way of doing CC but also allowing future things to be built on top. |
State, Effects & Console Improvements
|
@dcvz has done a lot to address what came up in the last review round. As far as I could see, any comments should now be addressed. That said, a lot has changed, so it's probably best to look at this completely fresh again. Also, I have not done another round of thorough testing after all these changes. Feeling a bit under the weather right now, but will try to get to that ASAP. I think it's safe to review right now though, we just shouldn't merge this before that testing is done. |
There was a problem hiding this comment.
This is beautiful!
I left a couple comments with questions in there, most are actually about things this PR hasn't changed, so @dcvz might be the one with the best answers to those.
I also left a couple tiny cleanup comments (like a place where a hardcoded string is still being checked)
Assuming testing goes well, there's nothing here that makes me think we shouldn't 
!
| EmitMessage(tcpsock, incomingEffect->id, incomingEffect->timeRemaining, result); | ||
| } else { | ||
| // check if a conflicting event is already active | ||
| // If another timed effect is already active that conflicts with the | ||
| // incoming effect, let CC retry later. | ||
| bool isConflictingEffectActive = false; | ||
| for (Effect* pack : activeEffects) { |
There was a problem hiding this comment.
I know this isn't a new name, but why pack?
There was a problem hiding this comment.
historical from Melon's implementation -- I can do a separate PR to clean up these things to take it out of aMannus' hands.
| bool isConflictingEffectActive = false; | ||
| for (Effect* pack : activeEffects) { | ||
| if (pack != incomingEffect && pack->category == incomingEffect->category && | ||
| pack->id < incomingEffect->id) { | ||
| isConflictingEffectActive = true; | ||
| EmitMessage(tcpsock, incomingEffect->id, incomingEffect->timeRemaining, | ||
| EffectResult::Retry); | ||
|
|
||
| break; |
There was a problem hiding this comment.
again, not something that's new to this PR, but i'm not fully understanding the logic here. when we break out of this for (Effect* pack : activeEffects) loop, we still make it down to L172 where incomingEffect is executed
if the idea that the checks within ExecuteEffect will also catch the incoming/active time effect conflict? if so do we need this loop here?
There was a problem hiding this comment.
No. This seems like a bug -- before it used to be a run with a dryRun = true. it should've been replaced with a CanApply() instead of an execute.
There might be a second issue that we shouldn't be emitting another event anyway, if isConflictingEffectActive
aMannus
added
do not merge
|
Changing it to do not merge until it's been properly tested. |
| @@ -95,6 +95,7 @@ class CrowdControl { | |||
| Effect* ParseMessage(char payload[512]); | |||
| EffectResult ExecuteEffect(Effect* effect); | |||
| EffectResult CanApplyEffect(Effect *effect); | |||
| EffectResult TranslateGiEnum(GameInteractionEffectQueryResult giResult); | |||
There was a problem hiding this comment.
Open for a better name
|
Apart from the name for the function in my own review comment, I think this is good to go now (for realsies this time maybe hopefully). |
Follow-up of #2195
The following are why this PR exists:
To do:
Build Artifacts