Let AI engulf even if running out of atp #5729

Patryk26g · 2024-12-05T19:54:34Z

Brief Description of What This PR Does

If there is a microbe nearby and the cell is not moving make it able to engulf the prey in order to survive

Related Issues

Progress Checklist

Note: before starting this checklist the PR should be marked as non-draft.

PR author has checked that this PR works as intended and doesn't
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)
Initial code review passed (this and further items should not be checked by the PR author)
Functionality is confirmed working by another person (see above checklist link)
Final code review is passed and code conforms to the
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.

hhyyrylainen · 2024-12-05T19:56:07Z

This is for the AI, right? That's unclear from just the title (so I had to open up this PR immediately when I received the notification to make sure this wasn't a fix for a critical regression in the game).

Patryk26g · 2024-12-05T20:53:05Z

This is for the AI, right? That's unclear from just the title (so I had to open up this PR immediately when I received the notification to make sure this wasn't a fix for a critical regression in the game).

Yes it is, sorry for not being clear

src/microbe_stage/systems/MicrobeAISystem.cs

HexapodPhilosopher · 2024-12-06T21:24:13Z

From testing this today, the AI still doesn't engulf while out of ATP.

Patryk26g · 2024-12-07T09:15:34Z

From testing this today, the AI still doesn't engulf while out of ATP.

It should work now, the "if statement" was built improperly

hhyyrylainen · 2024-12-09T07:56:44Z

src/microbe_stage/systems/MicrobeAISystem.cs

            {
+                if (cellHealth.CurrentHealth > 2 * Constants.ENGULF_NO_ATP_DAMAGE)
+                {
+                    // even if we are out of ATP and there is microbe nearby, engulf them.


Suggested change

// even if we are out of ATP and there is microbe nearby, engulf them.

// Even if we are out of ATP and there is microbe nearby, engulf them.

Comments should be started with a capital letter. I'll comment just this once but I saw this elsewhere as well).

hhyyrylainen · 2024-12-09T07:58:36Z

src/microbe_stage/systems/MicrobeAISystem.cs

+                {
+                    // even if we are out of ATP and there is microbe nearby, engulf them.
+                    // make sure engulfing doesn't kill the cell
+                    bool isMicrobeHunting = CheckForHuntingConditions(ref ai, ref position, ref organelles,


I would have asked to add a blank line for readability before if (isMicrobeHunting) but I think it makes more sense to just inline this method call in there.

hhyyrylainen

I'm not seeing any references to EnterEngulfModeForcedState so how does this actually force the engulf mode?

Patryk26g · 2024-12-12T18:40:25Z

Well, tried rebasing onto master but it seems to mess up my branch again

hhyyrylainen · 2024-12-12T19:34:43Z

Probably easiest if you just start a new fresh local branch and either cherry-pick the commits you want from here or just redo the changes manually. Then delete your local branch named engulfEvenIfOutOfAtp and make that branch again but starting from that new fix branch, then force push to overwrite all commits here.

Edit: also I wanted to mention that usually when duplicate commits get created like that you've either messed up a merge (with losing the info that the branch is actually merged) or an interactive rebase ended up rebasing remote commits (this is pretty easy to accidentally do if you do an interactive rebase onto a different branch and you have merge commits made in your branch, it's better to either rebase or squash first).

revolutionary-bot · 2024-12-20T12:00:05Z

We are currently in feature freeze until the next release.
If your PR is not just a simple fix, then it may take until the release to get reviewed and merged.

dligr

I have noticed some issues with the code, but the main functionality seems to work based on my testing

dligr · 2024-12-24T20:14:56Z

src/microbe_stage/systems/MicrobeAISystem.cs

-
-        // There is no reason to be engulfing at this stage
-        control.SetStateColonyAware(entity, MicrobeState.Normal);
+            // if out of ATP and the prey is out of reach to engulf, do nothing


This comment should begin with a capital letter too

dligr · 2024-12-24T20:22:26Z

src/microbe_stage/systems/MicrobeAISystem.cs

@@ -280,6 +280,8 @@ private void ChooseActions(in Entity entity, ref MicrobeAI ai, ref CompoundAbsor

        ref var control = ref entity.Get<MicrobeControl>();

+        ref var cellHealth = ref entity.Get<Health>();


Wouldn't it be better to use the health variable that is already passed into this function?

dligr · 2024-12-24T20:22:44Z

src/microbe_stage/systems/MicrobeAISystem.cs

@@ -467,6 +481,37 @@ private void ChooseActions(in Entity entity, ref MicrobeAI ai, ref CompoundAbsor
            }
        }

+        // check if species can hunt any prey and if so - engage in chase


This comment should too start with a capital letter

dligr · 2024-12-24T20:27:49Z

src/microbe_stage/systems/MicrobeAISystem.cs

+                {
+                    // Even if we are out of ATP and there is microbe nearby, engulf them.
+                    // make sure engulfing doesn't kill the cell
+                    if (CheckForHuntingConditions(ref ai, ref position, ref organelles, ref ourSpecies, ref engulfer,


Maybe it would be a good idea to move this check to after the if (!outOfSomething) { ... } code block? So that if the microbe has all the necessary compounds, but still lacks ATP because of some temporary conditions (like sprinting) it just waits to generate some more ATP instead of hunting

hhyyrylainen · 2024-12-25T14:10:53Z

but the main functionality seems to work based on my testing

How? I'm sorry that I'm commenting while on my break but I've read this PR like twice but I didn't see anything that could set the forced engulfing time variable above 0? So that means that there should be no way for this PR to trigger that functionality (refer to my comment: #5729 (review))

dligr · 2024-12-25T14:35:55Z

src/microbe_stage/systems/MicrobeAISystem.cs

+
+        if (atpLevel <= Constants.ENGULF_NO_ATP_TRIGGER_THRESHOLD)
+        {
+            control.EnterEngulfModeForcedState(ref health, ref compoundStorage, entity, Compound.ATP);


@hhyyrylainen Here's the line that calls EnterEngulfModeForcedState depending on ATP level.
I've also observed a cell enter engulf mode while being damaged by lack of ATP.

I see there's now this call so the PR had been updated since I last looked.

BTW this method is not necessarily the best one call as there is a method that enters engulf mode normally or in a forced manner if required (and doesn't enter the mode if the cell would die from the damage).

hhyyrylainen added this to the Release 0.8.0 milestone Dec 5, 2024

hhyyrylainen added the review label Dec 5, 2024

hhyyrylainen requested review from a team December 5, 2024 19:56

Patryk26g changed the title ~~Engulf even if running out of atp~~ Let AI engulf even if running out of atp Dec 5, 2024

CI09 reviewed Dec 6, 2024

View reviewed changes

src/microbe_stage/systems/MicrobeAISystem.cs Outdated Show resolved Hide resolved

hhyyrylainen reviewed Dec 9, 2024

View reviewed changes

hhyyrylainen requested changes Dec 9, 2024

View reviewed changes

Patryk26g marked this pull request as draft December 12, 2024 18:44

Patryk26g added 11 commits December 13, 2024 12:54

Engulf microbe even if out of ATP

5f0afe5

Fix

7a68e2e

Fix 2

4d70c95

Fix

208ff3c

Fix

4ba706d

Engulf microbe even if out of ATP

1b5d95d

Fix

502ea9b

Fix 2

c9eb9cb

rebase

e698eea

Fix

10ce6e6

Fix

fecacc8

Patryk26g force-pushed the engulfEvenIfOutOfAtp branch from 1d8f921 to fecacc8 Compare December 13, 2024 12:00

Patryk26g marked this pull request as ready for review December 13, 2024 12:06

hhyyrylainen removed this from the Release 0.8.0 milestone Dec 20, 2024

hhyyrylainen added this to the Release 0.8.1 milestone Dec 20, 2024

dligr reviewed Dec 24, 2024

View reviewed changes

dligr reviewed Dec 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let AI engulf even if running out of atp #5729

Let AI engulf even if running out of atp #5729

Patryk26g commented Dec 5, 2024

hhyyrylainen commented Dec 5, 2024

Patryk26g commented Dec 5, 2024

HexapodPhilosopher commented Dec 6, 2024

Patryk26g commented Dec 7, 2024

hhyyrylainen Dec 9, 2024

hhyyrylainen Dec 9, 2024

hhyyrylainen left a comment

Patryk26g commented Dec 12, 2024

hhyyrylainen commented Dec 12, 2024 •

edited

Loading

revolutionary-bot commented Dec 20, 2024

dligr left a comment

dligr Dec 24, 2024 •

edited

Loading

dligr Dec 24, 2024

dligr Dec 24, 2024

dligr Dec 24, 2024

hhyyrylainen commented Dec 25, 2024

dligr Dec 25, 2024

hhyyrylainen Dec 25, 2024

	// even if we are out of ATP and there is microbe nearby, engulf them.
	// Even if we are out of ATP and there is microbe nearby, engulf them.

		@@ -280,6 +280,8 @@ private void ChooseActions(in Entity entity, ref MicrobeAI ai, ref CompoundAbsor

		ref var control = ref entity.Get<MicrobeControl>();

		ref var cellHealth = ref entity.Get<Health>();

Let AI engulf even if running out of atp #5729

Are you sure you want to change the base?

Let AI engulf even if running out of atp #5729

Conversation

Patryk26g commented Dec 5, 2024

hhyyrylainen commented Dec 5, 2024

Patryk26g commented Dec 5, 2024

HexapodPhilosopher commented Dec 6, 2024

Patryk26g commented Dec 7, 2024

hhyyrylainen Dec 9, 2024

Choose a reason for hiding this comment

hhyyrylainen Dec 9, 2024

Choose a reason for hiding this comment

hhyyrylainen left a comment

Choose a reason for hiding this comment

Patryk26g commented Dec 12, 2024

hhyyrylainen commented Dec 12, 2024 • edited Loading

revolutionary-bot commented Dec 20, 2024

dligr left a comment

Choose a reason for hiding this comment

dligr Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

dligr Dec 24, 2024

Choose a reason for hiding this comment

dligr Dec 24, 2024

Choose a reason for hiding this comment

dligr Dec 24, 2024

Choose a reason for hiding this comment

hhyyrylainen commented Dec 25, 2024

dligr Dec 25, 2024

Choose a reason for hiding this comment

hhyyrylainen Dec 25, 2024

Choose a reason for hiding this comment

hhyyrylainen commented Dec 12, 2024 •

edited

Loading

dligr Dec 24, 2024 •

edited

Loading