Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce network burden of thirst system #33403

Closed
wants to merge 4 commits into from

Conversation

Centronias
Copy link
Contributor

@Centronias Centronias commented Nov 18, 2024

About the PR

Replace ThirstComponent's CurrentThirst with ThirstSystem.GetThirst, which calculates the current thirst value from an initial value, a time when that initial value was set, and a constant decay rate.

See also #32986

Why / Balance

#28070

Technical details

  • ThirstComponent no longer has CurrentThirst, and instead has LastAuthoritativeThirstValue and LastAuthoritativeThirstChangeTime
  • Replaced all usages of ThirstComponent.CurrentThirst with ThirstSystem.GetThirst, which calculates the current thirst value by predicting the decay since the last authoritative update:
    • LastAuthoritativeThirstValue - (IGameTiming.CurrTime - LastAuthoritativeThirstChangeTime) * ActualDecayRate
  • ThirstSystem.Update now only looks to update the current thirst threshold (because there's no value to update)
  • Added ThirstSystem.SetAuthoritativeThirstValue, which sets LastAuthoritativeThirstValue and LastAuthoritativeThirstChangeTime and dirties the component, causing those values to get pushed to the client, resetting the "current" calculation's basepoint.
  • ThirstSystem.SetThirst and anything that modifies the current threshold call ThirstSystem.SetAuthoritativeThirstValue to ensure any unpredictable changes are replicated to the client.

Media

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

thirst.mp4

Requirements

Breaking changes

  • ThirstComponent's CurrentThirst is gone. It needs to be replaced by:
    • ThirstSystem.GetThirst in C#
    • lastAuthoritativeThirstValue in YAML (eg. for initial thirst values on prototypes)

Changelog
Technical implementation changes, no CL.

@github-actions github-actions bot added size/L Denotes a PR that changes 1000-4999 lines. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Nov 18, 2024
@ScarKy0 ScarKy0 added P1: High Priority: Higher priority than other items, but isn't an emergency. T: Performance Type: Performance impacting changes or bugs T: Refactor Type: Refactor of notable amount of codebase D2: Medium Difficulty: A good amount of codebase knowledge required. S: Needs Review Status: Requires additional reviews before being fully accepted A: General Interactions Area: General in-game interactions that don't relate to another area. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Nov 18, 2024
@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Dec 21, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

/// This value should be updated relatively infrequently. To get the current thirst, which changes with each update,
/// use <see cref="ThirstSystem.GetThirst"/>.
/// </summary>
[DataField, ViewVariables(VVAccess.ReadOnly)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare attributes on the same line, remove ViewVariables

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, ViewVariables is necessary because it explicitly prevents this field from being edited.

@github-actions github-actions bot added size/M Denotes a PR that changes 100-999 lines. S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted and removed size/L Denotes a PR that changes 1000-4999 lines. S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted labels Jan 20, 2025
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@Centronias
Copy link
Contributor Author

Closing in favor of #34166

@Centronias Centronias closed this Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: General Interactions Area: General in-game interactions that don't relate to another area. D2: Medium Difficulty: A good amount of codebase knowledge required. P1: High Priority: Higher priority than other items, but isn't an emergency. S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted S: Needs Review Status: Requires additional reviews before being fully accepted size/M Denotes a PR that changes 100-999 lines. T: Performance Type: Performance impacting changes or bugs T: Refactor Type: Refactor of notable amount of codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants