Make AI use the signaling agent and add more conditions for responding to commands#6736
Make AI use the signaling agent and add more conditions for responding to commands#6736
Conversation
AI_SIGNALING_CHANCE is so that it's not used a billion times per second (Thanks to Patryk26g for suggesting to use NextSingle instead of Next) This is only the very beginning of this.
| // Use signaling agent if I have any with a chance of 5% per think | ||
| if (organelles.HasSignalingAgent && random.NextSingle() < Constants.AI_SIGNALING_CHANCE) | ||
| { | ||
| UseSignalingAgent(ref organelles, speciesAggression, ref signaling, ref control, random); |
There was a problem hiding this comment.
It seems like there's only one case where the signalling command is set to None. That means that cells do not stop signalling! That's a very, very major bug in my opinion because eventually I think most cells will just be signalling all the time.
Also I think another key part still missing is that cells should decide to ignore an incoming signalling command in certain situations (like if become aggressive signal is too far away, or a move signal is far away and the cell is not that full on resources).
Without those two things I think the signals will quite effectively destroy any useful AI behaviours as they'll get permanently locked into following the signal of the first species member of them that triggered it.
There was a problem hiding this comment.
Do you have suggestions for when it should be set to no command?
There was a problem hiding this comment.
I'd say whenever the original condition to set a signal is gone, or like after 15-30 seconds of the cell not feeling like it should send the same signal again. Something like that, but I'm sure someone else more familiar with the AI could come up with a more advanced codnitions.
| else | ||
| { | ||
| signalExists = false; | ||
| } |
There was a problem hiding this comment.
is this "else" needed? can do without it, just make "bool signalExists = signaling.ReceivedCommand != MicrobeSignalCommand.None && entity.IsAliveAndHas()". Btw what is " entity.IsAliveAndHas()" used for? Shouldn't it be "signaling.ReceivedCommandFromEntity.IsAliveAndHas()"?
There was a problem hiding this comment.
Btw what is " entity.IsAliveAndHas()" used for?
It's so that the game doesn't crash when there isn't a signal.
There was a problem hiding this comment.
This else still should be removed... it literally if signal == false then signal = false
| private void UseSignalingAgent(ref OrganelleContainer organelles, float speciesAggression, | ||
| ref CommandSignaler signaling, Random random) | ||
| { | ||
| var willBeAggressiveThisTime = RollCheck(speciesAggression, Constants.MAX_SPECIES_AGGRESSION, random); |
There was a problem hiding this comment.
willBeAggressiveThisTime -> shouldBecomeAggresive
There was a problem hiding this comment.
Personally, I think that's an inaccurate name considering what the variable is being used for.
There was a problem hiding this comment.
As I said in other comment, future tense variables should be omitted.
| signaling.QueuedSignalingCommand = MicrobeSignalCommand.None; | ||
| } | ||
| } | ||
| else |
There was a problem hiding this comment.
I guess it is right now, but since I want to add more sub-conditions I'll keep it.
There was a problem hiding this comment.
Are you going to do this in this PR? If not, then you should remove it. YAGNI
There was a problem hiding this comment.
Yes, I'm gonna do it in this PR.
There was a problem hiding this comment.
So there was unimplemented stuff? Good thing that I didn't spend any more review energy yet...
|
|
||
| // Adjusted behaviour values (calculated here as these are needed by various methods) | ||
| var speciesBehaviour = ourSpecies.Species.Behaviour; | ||
| var willAdjustBehaviourValues = signaling.ReceivedCommand == MicrobeSignalCommand.BecomeAggressive && |
There was a problem hiding this comment.
I think we should omit future tense in variables... adjustBehaviourValues is fine. or event better "shouldBeAggresive"
There was a problem hiding this comment.
You know, I never thought about it, but you are right that future tense is not used in variable names customarily. So yeah, this sounds like a good naming change, and I would definitely have named this variable something along the lines of needsToAdjustBehaviour.
| bool signalExists = signaling.ReceivedCommand != MicrobeSignalCommand.None && | ||
| entity.IsAliveAndHas<WorldPosition>(); | ||
| Vector3 signalerPosition = default; | ||
| float squaredDistanceToSignaler = default; |
There was a problem hiding this comment.
squaredDistanceToSignaler -> signalerDistanceSquared
| ai.ATPThreshold = 0.0f; | ||
| } | ||
|
|
||
| // Use signaling agent if I have any with a small chance per think |
There was a problem hiding this comment.
Is this supposed to be "per tick"?
There was a problem hiding this comment.
I hope not because we do not have "ticks" in the game (the closest concept we have is a game simulation update)...
There was a problem hiding this comment.
I do mean "think", as in each time AIThink() is called. I guess it should say "per update", then?
There was a problem hiding this comment.
I think "per think" is perfectly understandable, and I have a feeling that there are existing comments that are of similar format. But for clarity it could be written as "per think method call".
| private void UseSignalingAgent(ref OrganelleContainer organelles, float speciesAggression, | ||
| ref CommandSignaler signaling, Random random) | ||
| { | ||
| var shouldBeAggressive = RollCheck(speciesAggression, Constants.MAX_SPECIES_AGGRESSION, random); |
There was a problem hiding this comment.
So this, combined with the section below is just deciding whether they should be trying to travel in a group? (Because this is outside specific responses, just passive, right?)
I think this would make sense not just for aggressive predators, but also for some Brave prey.
There was a problem hiding this comment.
This comment looks like it was not marked as solved?
There was a problem hiding this comment.
Patryk26g told me to not resolve (his) comments as solved myself.
There was a problem hiding this comment.
You should ask for a review again / comment again mentioning their name to ask for further feedback if a reviewer is not responding in a reasonable amount of time to your further changes (I do know from personal experience that it is easy to miss new changes that were related to a review comment I left).
The actual defense part should be covered by the shouldBeAggressive condition in UseSignalingAgent()
| var compounds = compoundStorage.Compounds; | ||
|
|
||
| bool signalExists = signaling.ReceivedCommand != MicrobeSignalCommand.None && | ||
| entity.IsAliveAndHas<WorldPosition>(); |
There was a problem hiding this comment.
Isn't entity wrong here? The system should not even run if the entity is dead, so shouldn't this code be checking if ReceivedCommandFromEntity is the one that is alive?
| var adjustBehaviourValues = signaling.ReceivedCommand == MicrobeSignalCommand.BecomeAggressive && | ||
| signalerDistanceSquared < Constants.AI_BECOME_AGGRESSIVE_DISTANCE_SQUARED; |
There was a problem hiding this comment.
I think this needs to check the signalExists safety flag as well because theoretically the received command might be left to aggressive command but then there signaller doesn't exist.
| { | ||
| ai.MoveToLocation(signaling.ReceivedCommandFromEntity.Get<WorldPosition>().Position, | ||
| ref control, entity); | ||
| if (signalerDistanceSquared < Constants.AI_MOVE_DISTANCE_SQUARED) |
There was a problem hiding this comment.
How small is this distance? The signal is literally meant to bump into other members to make using the binding agent easy for the player, so I don't really agree with this change...
|
|
||
| if (organelles.HasBindingAgent) | ||
| { | ||
| signaling.QueuedSignalingCommand = MicrobeSignalCommand.MoveToMe; |
There was a problem hiding this comment.
This line seems useless so should be deleted (there's no early exit so we always execute the line signaling.QueuedSignalingCommand = MicrobeSignalCommand.None; after this).
Or alternatively it could be use some logic to detect when cells would be good to group together and would then set this but only if already in binding mode / going into binding mode, as otherwise it is useless to trigger this signal.
There was a problem hiding this comment.
Doesn't it make more sense to remove the signaling.QueuedSignalingCommand = MicrobeSignalCommand.None; right at the end of the function?
There was a problem hiding this comment.
Well there has to be a some quite common condition to reset the signal as I don't want the AI to be entirely made so that it is assumed that there's almost always a signal. It is better for signals to only be sent occasionally so that the AI just doesn't become a total mess where it is always just forced to respond to other signals.
Also for safety I think most "non-urgent" signals should not be activated if the current cell is receiving another signal as that just adds to the signal noise.
| if (organelle.Definition.HasPilusComponent || organelles.AgentVacuoleCount > 0) | ||
| { | ||
| var membersNearEnough = 0; | ||
| var enoughMembers = (int)speciesAggression / 100; | ||
|
|
||
| foreach (var member in speciesMembers!) | ||
| { | ||
| if (position.Position.DistanceSquaredTo(member.Position) | ||
| < Constants.AI_BECOME_AGGRESSIVE_DISTANCE_SQUARED) | ||
| { | ||
| ++membersNearEnough; | ||
| } | ||
| } | ||
|
|
||
| if (membersNearEnough >= enoughMembers) | ||
| { | ||
| signaling.QueuedSignalingCommand = MicrobeSignalCommand.BecomeAggressive; | ||
| break; | ||
| } | ||
|
|
||
| signaling.QueuedSignalingCommand = MicrobeSignalCommand.FollowMe; | ||
| break; | ||
| } | ||
|
|
||
| signaling.QueuedSignalingCommand = MicrobeSignalCommand.None; |
There was a problem hiding this comment.
This loop looks very suspicious to me. I think instead what you actually want to do is first setup variables for pilus count, and then do a count of pili, and only then would you do the membersNearEnough calculation as that's an expensive calculation to do each loop iteration so I'm quite sure you didn't meant to put that logic inside this loop.
Brief Description of What This PR Does
This PR makes the microbe AI use the signaling agent and adds more conditions for responding to commands.
Related Issues
Closes #5118
Discussion: https://forum.revolutionarygamesstudio.com/t/how-should-ai-use-the-signaling-agent/1233
Progress Checklist
Note: before starting this checklist the PR should be marked as non-draft.
break existing features:
https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
(this is important as to not waste the time of Thrive team
members reviewing this PR)
styleguide.
Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.