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

src/microbe_stage/systems/MicrobeAISystem.cs

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

src/microbe_stage/systems/MicrobeAISystem.cs

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))

src/microbe_stage/systems/MicrobeAISystem.cs

hhyyrylainen · 2025-01-08T13:37:27Z

src/microbe_stage/systems/MicrobeAISystem.cs

-        control.SetStateColonyAware(entity, engulf ? MicrobeState.Engulf : MicrobeState.Normal);
+        var ourCompounds = compoundStorage.Compounds;
+
+        if (atpLevel <= Constants.ENGULF_NO_ATP_TRIGGER_THRESHOLD)


I see the EnterEngulfModeForcedState call now, but now I'm wondering why the ATP level is checked here? Because EnterEngulfModeForcedState ultimately calls ForceStateApplyIfRequired which only causes problems if ATP is not sufficient, so the AI code doesn't actually need this duplicate check, right?

So we just use EnterEngulfModeForcedState withour SetStateColonyAware?

Yes, I didn't fully read the PR again but from a quick look EnterEngulfModeForcedState already has colony handling and checking for the ATP levels. So my comment is just about reducing unnecessary code in the AI system in this spot.

hhyyrylainen

There's just a small change I'd like to see to make this conform to our styleguide. Then if the AI works fine in game this will be ready to merge.

src/microbe_stage/systems/MicrobeAISystem.cs

hhyyrylainen · 2025-01-31T10:29:18Z

src/microbe_stage/systems/MicrobeAISystem.cs

+        bool isHunting = CheckForHuntingConditions(ref ai, ref position, ref organelles, ref ourSpecies, ref engulfer,
+            ref cellProperties, ref control, ref health, ref compoundStorage, entity, speciesFocus, speciesAggression,
+            speciesActivity, speciesOpportunism, strain, random, false, atpLevel);
+        if (isHunting)


Wouldn't it be cleaner to not split out the isHunting variable here? There's already a very similar block of code above that doesn't use a temporary variable. (will also need to add braces here as the if is then so long)

hhyyrylainen · 2025-02-03T10:02:53Z

src/microbe_stage/systems/MicrobeAISystem.cs

        {
            // Keep the maximum at 95% full, as there is flickering when near full
            ai.ATPThreshold = 0.95f * speciesFocus / Constants.MAX_SPECIES_FOCUS;
        }

+        // Even if we are out of ATP and there is microbe nearby, engulf them.
+        // make sure engulfing doesn't kill the cell. The cell won't engulf if it would kill it


I think this comment line is pretty unclear and either should be removed or reworded.

hhyyrylainen · 2025-02-03T10:04:37Z

Other than that one hard to read comment, the code changes look good to me here. And as long as the AI does actually sometimes use this new functionality, this will be good to merge.

comment fix

hhyyrylainen

I'll trust that there's been enough playtesting to merge this.

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

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

hhyyrylainen reviewed Dec 9, 2024

View reviewed changes

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

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

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

dligr reviewed Dec 24, 2024

View reviewed changes

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

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

dligr reviewed Dec 24, 2024

View reviewed changes

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

dligr reviewed Dec 25, 2024

View reviewed changes

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

hhyyrylainen reviewed Jan 8, 2025

View reviewed changes

Patryk26g added 3 commits January 30, 2025 21:39

Fixes

d7b3e19

Merge branch 'master' into engulfEvenIfOutOfAtp

98aa729

Fix

1ec349a

hhyyrylainen requested changes Jan 31, 2025

View reviewed changes

Patryk26g added 3 commits January 31, 2025 20:57

Style fix

a558d50

Fixes

b032d5f

Fix

6241339

hhyyrylainen reviewed Feb 3, 2025

View reviewed changes

Update MicrobeAISystem.cs

aa72d08

comment fix

hhyyrylainen approved these changes Feb 3, 2025

View reviewed changes

hhyyrylainen merged commit 5cb19a9 into Revolutionary-Games:master Feb 3, 2025
4 checks passed

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 •

edited by hhyyrylainen

Loading

hhyyrylainen commented Dec 5, 2024

Patryk26g commented Dec 5, 2024

HexapodPhilosopher commented Dec 6, 2024

Patryk26g commented Dec 7, 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

hhyyrylainen commented Dec 25, 2024

hhyyrylainen Jan 8, 2025

Patryk26g Jan 30, 2025

hhyyrylainen Jan 31, 2025

hhyyrylainen left a comment

hhyyrylainen Jan 31, 2025

hhyyrylainen Feb 3, 2025

hhyyrylainen commented Feb 3, 2025

hhyyrylainen left a comment

Let AI engulf even if running out of atp #5729

Let AI engulf even if running out of atp #5729

Conversation

Patryk26g commented Dec 5, 2024 • edited by hhyyrylainen Loading

hhyyrylainen commented Dec 5, 2024

Patryk26g commented Dec 5, 2024

HexapodPhilosopher commented Dec 6, 2024

Patryk26g commented Dec 7, 2024

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

hhyyrylainen commented Dec 25, 2024

hhyyrylainen Jan 8, 2025

Choose a reason for hiding this comment

Patryk26g Jan 30, 2025

Choose a reason for hiding this comment

hhyyrylainen Jan 31, 2025

Choose a reason for hiding this comment

hhyyrylainen left a comment

Choose a reason for hiding this comment

hhyyrylainen Jan 31, 2025

Choose a reason for hiding this comment

hhyyrylainen Feb 3, 2025

Choose a reason for hiding this comment

hhyyrylainen commented Feb 3, 2025

hhyyrylainen left a comment

Choose a reason for hiding this comment

Patryk26g commented Dec 5, 2024 •

edited by hhyyrylainen

Loading

hhyyrylainen commented Dec 12, 2024 •

edited

Loading