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

Convert movement packets #492

Open
wants to merge 36 commits into
base: arena-vertical-slice
Choose a base branch
from

Conversation

Ciel1996
Copy link

@Ciel1996 Ciel1996 commented Jan 20, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

  • All new classes have class-level documentation comments, if there are any at all
  • Tests for the changes have been added (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation update
  • Other... Please describe:

What is the current behaviour?

The movement packets are currently actively polled and not rate limited.

What is the new behaviour?

Resolves #468
Subscribe MovementHandler to MessageBroker<Movement> events which then get handled instead of being polled.
In order to implement the rate-limit of 30 messages per second per agent, a Polly RateLimit-Policy has been introduced.
The desired amount of packets and the timespan can be adjusted via constant/static values at the top of the class.
Each AgentId is assigned their own policy, reason is given via code comment.

Other information

Writing a test proved difficult, as there is no way of getting a valid and initialized instance of Agent as far as I've seen. If there is a known way for this, please feel free to tag me and I will include tests.

@Ciel1996 Ciel1996 self-assigned this Jan 20, 2023
@Ciel1996
Copy link
Author

Ciel1996 commented Jan 20, 2023

@garrettluskey I'm not that happy with the IMemoryCache as means of rate-limiting. Do you have any experience with this? I'm only familiar with Polly's rate limit, but I'm not sure if this could be appropriate here.

I'm open for suggestions, I will try something to find a better solution.

EDIT: I rewrote part of the code and chose Polly.

@Ciel1996 Ciel1996 linked an issue Jan 20, 2023 that may be closed by this pull request
2 tasks
@garrettluskey
Copy link
Contributor

Also could you create some tests for rate limiting/testing each event

@Ciel1996
Copy link
Author

Ciel1996 commented Jan 30, 2023

Thanks for the feedback. Sorry for the delay so far, I gotta invest some more time into this.

- Unsure what to make of Polly with this, as it seems like the cleaner
  solution
- Implemented basic delta-management-logic
- refactored classes
- added missing comments
@EgardA
Copy link
Contributor

EgardA commented Feb 15, 2023

If you could make the MovementHandler use the new methods that have been created in the NetworkAgentRegistry that would be great :)

@Ciel1996
Copy link
Author

Will do while resolving the merge conflict 👍

source/ClientDebug/ClientDebug.csproj.user Outdated Show resolved Hide resolved
source/MissionTests/RunTimeHelper.cs Outdated Show resolved Hide resolved
source/Missions/Services/Agents/Packets/MovementHandler.cs Outdated Show resolved Hide resolved
Ciel1996 and others added 4 commits February 19, 2023 13:42
- Refactored AgentMovement.Apply()
- Refactored AgentMovement into struct
- Added operator overload to AgentEquipmentData due to default
  comparison in AgentMovement
@garrettluskey
Copy link
Contributor

To be able to close this PR a publisher of these events is still needed.

Create a publisher class under the Agent folder that only manages the PlayerControlled Agent (Agent.Main).

In this class poll the agents values that are in AgentMovement. Save those values in a AgentMovement variable. If these values change publish an event associated with that change.

@Ciel1996
Copy link
Author

Should be good to go

source/Missions/Services/Agents/AgentPublisher.cs Outdated Show resolved Hide resolved
source/Missions/Services/Agents/AgentPublisher.cs Outdated Show resolved Hide resolved
source/Missions/Services/Agents/AgentPublisher.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@garrettluskey garrettluskey left a comment

Choose a reason for hiding this comment

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

2 very small things to fix. Also check that this works in game. If needed I can help you get it working with some pair programming in the future. Just let me know

source/Missions/Services/Agents/AgentPublisher.cs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert Movement packets from per second to send on change
3 participants