Reduce network burden of the hunger system #32986

Centronias · 2024-10-24T21:15:31Z

About the PR

Replace HungerComponent's CurrentHunger with HungerSystem.GetHunger, which calculates the current hunger value from an initial value, a time when that initial value was set, and a constant decay rate.

Why / Balance

#28070

Technical details

HungerComponent no longer has CurrentHunger, and instead has LastAuthoritativeHungerValue and LastAuthoritativeHungerChangeTime
Replaced all usages of HungerComponent.CurrentHunger with HungerSystem.GetHunger, which calculates the current hunger value by predicting the decay since the last authoritative update:
- LastAuthoritativeHungerValue - (IGameTiming.CurrTime - LastAuthoritativeHungerChangeTime) * ActualDecayRate
HungerSystem.Update now only looks to update the current hunger threshold (because there's no value to update)
Added HungerSystem.SetAuthoritativeHungerValue, which sets LastAuthoritativeHungerValue and LastAuthoritativeHungerChangeTime and dirties the component, causing those values to get pushed to the client, resetting the "current" calculation's basepoint.
HungerSystem.SetHunger and anything that modifies the current threshold call HungerSystem.SetAuthoritativeHungerValue to ensure any unpredictable changes are replicated to the client.

Media

Before:
(With non-game-logic-altering modifications made to raise an event on HungerComponent replication + popup creation when getting that event)

hunger-before.mp4

After:
(With non-game-logic-altering modifications made to raise an event on HungerComponent replication + popup creation when getting that event; and create a popup on every HungerSystem update tick)

hunger.mp4

Requirements

I have read and am following the Pull Request and Changelog Guidelines.
I have added media to this PR or it does not require an ingame showcase.

Breaking changes

HungerComponent's CurrentHunger no longer exists and must be replaced by a call to HungerSystem.GetHunger

Changelog
My understanding is that this is a totally under-the-hood change, so no CL.

Centronias · 2024-10-24T21:15:49Z

I'll do the same thing to thirst if this PR doesn't get obliterated for my implementation.

slarticodefast · 2024-10-26T10:51:46Z

I'll do the same thing to thirst if this PR doesn't get obliterated for my implementation.

Ideally someone finishes this derelict PR to combine the hunger and thirst systems into satiation #27481
But until then it should be fine to improve things step by step 👍

PJB3005

This is absolutely not the right way to do this. The issue you linked literally explains the correct way to fix it instead. Do that.

And do not do random style changes/refactors in unrelated PRs.

add clamp helper

Centronias · 2024-10-28T21:20:08Z

Content.Server/EntityEffects/EffectConditions/TotalHunger.cs

@@ -15,9 +15,10 @@ public sealed partial class Hunger : EntityEffectCondition

    public override bool Condition(EntityEffectBaseArgs args)
    {
-        if (args.EntityManager.TryGetComponent(args.TargetEntity, out HungerComponent? hunger))
+        if (args.EntityManager.TryGetComponent(args.TargetEntity, out HungerComponent? hunger) &&
+            args.EntityManager.SystemOrNull<HungerSystem>() is { } hungerSystem)


I don't know if args.EntityManager.SystemOrNull<HungerSystem>() is the right thing to do here, but I am not sure how else to access the system.

Just use GetEntitySystem?

The only GetEntitySystem I could find is on IEntitySystemManager which, I guess, would need to be injected, but I assume [Dependency]s can't be injected into non-systems like this.

Instead, I've changed it to just do args.EntityManager.System<...>(), based on

space-station-14/Content.Server/EntityEffects/EffectConditions/HasTagCondition.cs

Line 21 in fdd713a

return args.EntityManager.System<TagSystem>().HasTag(tag, Tag) ^ Invert;

doing that same thing.

If this is still not the right thing, do you mind providing me with a bit more guidance?

…ing to check the yaml

Centronias · 2024-10-28T21:29:03Z

Resources/Prototypes/Entities/Mobs/NPCs/animals.yml

@@ -1686,7 +1686,7 @@
      Dead: 0
    baseDecayRate: 0.04
  - type: Hunger
-    currentHunger: 25   # spawn with Okay hunger state


This name change definitely sucks, but I don't know a way around it.

It would be good for currentHunger or startingHunger to be the field in the yaml, but it would, I think, not be good to have either of those names show up in-game in something like ViewVariables.

This name change definitely sucks, but I don't know a way around it.

It would be good for currentHunger or startingHunger to be the field in the yaml, but it would, I think, not be good to have either of those names show up in-game in something like ViewVariables.

[ViewVariables(VVAccess.ReadOnly)] it.

Also I'm looking at the code, does setting this data field from YAML even work? It looks like it would always get replaced by the randomized amount selected in OnMapInit. (Note that this doesn't appear to be an issue with your PR, just an observation).

It looks like this was changed with the last rewrite of all of this:
https://github.com/space-wizards/space-station-14/pull/14939/files#diff-7ff03c169eb454aed40cb0979bd1d81f65ce0db09590d25191682c783e7ac344L131
(PR line links never work for me, but it's this:

space-station-14/Content.Server/Nutrition/Components/HungerComponent.cs

Line 131 in 19277a2

if (_currentHunger < 0)

)

Should I get rid of the fields in the yaml, since they don't do anything now?

Your choice I suppose

PJB3005 · 2024-10-30T01:53:20Z

Content.Server/EntityEffects/EffectConditions/TotalHunger.cs

@@ -15,9 +15,10 @@ public sealed partial class Hunger : EntityEffectCondition

    public override bool Condition(EntityEffectBaseArgs args)
    {
-        if (args.EntityManager.TryGetComponent(args.TargetEntity, out HungerComponent? hunger))
+        if (args.EntityManager.TryGetComponent(args.TargetEntity, out HungerComponent? hunger) &&
+            args.EntityManager.SystemOrNull<HungerSystem>() is { } hungerSystem)


Just use GetEntitySystem?

Content.Shared/Nutrition/EntitySystems/HungerSystem.cs

PJB3005 · 2024-10-30T02:00:36Z

Content.Shared/Nutrition/EntitySystems/HungerSystem.cs

@@ -120,7 +151,7 @@ private void UpdateCurrentThreshold(EntityUid uid, HungerComponent? component =
            return;
        component.CurrentThreshold = calculatedHungerThreshold;
        DoHungerThresholdEffects(uid, component);
-        Dirty(uid, component);
+        SetAuthoritativeHungerValue((uid, component), GetHunger(component));


Why is this necessary?

My thinking is any time the threshold changes, the "start point" values should be updated as well.
I suppose that's not strictly true as the only change that necessitates an update is a change to the decay rate. So I'll move this call down to exactly where the decay rate is modified.

PJB3005 · 2024-10-30T02:04:05Z

Resources/Prototypes/Entities/Mobs/NPCs/animals.yml

@@ -1686,7 +1686,7 @@
      Dead: 0
    baseDecayRate: 0.04
  - type: Hunger
-    currentHunger: 25   # spawn with Okay hunger state


This name change definitely sucks, but I don't know a way around it.

It would be good for currentHunger or startingHunger to be the field in the yaml, but it would, I think, not be good to have either of those names show up in-game in something like ViewVariables.

[ViewVariables(VVAccess.ReadOnly)] it.

Also I'm looking at the code, does setting this data field from YAML even work? It looks like it would always get replaced by the randomized amount selected in OnMapInit. (Note that this doesn't appear to be an issue with your PR, just an observation).

PJB3005 · 2024-11-12T21:50:49Z

Content.Shared/Nutrition/EntitySystems/HungerSystem.cs

@@ -23,6 +24,7 @@ public sealed class HungerSystem : EntitySystem
    [Dependency] private readonly MobStateSystem _mobState = default!;
    [Dependency] private readonly MovementSpeedModifierSystem _movementSpeedModifier = default!;
    [Dependency] private readonly SharedJetpackSystem _jetpack = default!;
+    [Dependency] private readonly INetManager _net = default!;


This is unused.

PJB3005 · 2024-11-12T21:52:18Z

Resources/Prototypes/Entities/Mobs/NPCs/animals.yml

@@ -1686,7 +1686,7 @@
      Dead: 0
    baseDecayRate: 0.04
  - type: Hunger
-    currentHunger: 25   # spawn with Okay hunger state


Your choice I suppose

reduce network burden of the hunger system

0b24eb8

PJB3005 self-assigned this Oct 28, 2024

PJB3005 requested changes Oct 28, 2024

View reviewed changes

Centronias added 3 commits October 28, 2024 13:27

Merge remote-tracking branch 'origin/master' into hunger-thirst

7098e57

explicit start + last updated

41c9cb8

remove auto reformat changes to otherwise untouched code

c1b6bb7

add clamp helper

Centronias force-pushed the hunger-thirst branch from 377c010 to c1b6bb7 Compare October 28, 2024 21:17

Centronias commented Oct 28, 2024

View reviewed changes

Centronias requested a review from PJB3005 October 28, 2024 21:20

github-actions bot added the Status: Needs Review This PR requires new reviews before it can be merged. label Oct 28, 2024

imagine making breaking changes, documenting them, and then not think…

ec498bb

…ing to check the yaml

Centronias commented Oct 28, 2024

View reviewed changes

PJB3005 requested changes Oct 30, 2024

View reviewed changes

comments

ca8d7cd

Centronias requested a review from PJB3005 October 30, 2024 18:39

PJB3005 requested changes Nov 12, 2024

View reviewed changes

PJB3005 added Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. and removed Status: Needs Review This PR requires new reviews before it can be merged. labels Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce network burden of the hunger system #32986

Reduce network burden of the hunger system #32986

Centronias commented Oct 24, 2024 •

edited

Loading

Centronias commented Oct 24, 2024

slarticodefast commented Oct 26, 2024

PJB3005 left a comment

Centronias Oct 28, 2024

PJB3005 Oct 30, 2024

Centronias Oct 30, 2024

Centronias Oct 28, 2024

PJB3005 Oct 30, 2024

Centronias Oct 30, 2024 •

edited

Loading

PJB3005 Nov 12, 2024

PJB3005 Oct 30, 2024

PJB3005 Oct 30, 2024

Centronias Oct 30, 2024

PJB3005 Oct 30, 2024

PJB3005 Nov 12, 2024

PJB3005 Nov 12, 2024

Reduce network burden of the hunger system #32986

Are you sure you want to change the base?

Reduce network burden of the hunger system #32986

Conversation

Centronias commented Oct 24, 2024 • edited Loading

About the PR

Why / Balance

Technical details

Media

Requirements

Breaking changes

Centronias commented Oct 24, 2024

slarticodefast commented Oct 26, 2024

PJB3005 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centronias Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centronias commented Oct 24, 2024 •

edited

Loading

Centronias Oct 30, 2024 •

edited

Loading