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

Fly, use rotating buffer over list to store last 5 positions. #20140

Merged
merged 1 commit into from Aug 2, 2023

Conversation

anvilvapre
Copy link
Contributor

@anvilvapre anvilvapre commented Jul 23, 2022

Fly stores the last the position over the lats 5 ticks to determine the distance travelled and whether flight is stuck.

  • Replace position buffer List with an rotating buffer. To avoid removing the head element from a list array each tick. Avoiding a ArrayCopy/Move each tick.
  • Avoid addition of one WVec/Pos to Zero WVev/Pos. Trivial.
  • Tickfacing, 50% change of one less subtraction. Trivial.
  • RotatingBuffer can be reused in new contrail implementation. Could potentially also be used in input tap detection. (only used function tested)

Future possible improvements: Map.DistanceAboveTerrain is looked up more often by all flight activites/functions - during the same tick. For rectangular maps the operation is not that expensive. Functions already take many arguments - did not want to add it.

OpenRA.Game/Primitives/RotatingBuffer.cs Outdated Show resolved Hide resolved
OpenRA.Game/Primitives/RotatingBuffer.cs Outdated Show resolved Hide resolved
OpenRA.Game/Primitives/RotatingBuffer.cs Outdated Show resolved Hide resolved
OpenRA.Game/Primitives/RotatingBuffer.cs Outdated Show resolved Hide resolved
@anvilvapre anvilvapre force-pushed the 20220718_fly_history branch 2 times, most recently from 8d53513 to 328f30f Compare October 9, 2022 12:25
@anvilvapre
Copy link
Contributor Author

Requested changes addressed.

Copy link
Contributor

@AspectInteractive2 AspectInteractive2 left a comment

Choose a reason for hiding this comment

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

This looks fine to me. I am not sure how much of a difference these changes will make but they are reasonable changes.

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

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

I think buffer related changes should be separated into its own commit. This also needs a rebased as it conflicts with the current codestyle (I've suggested the fix).

OpenRA.Game/Primitives/RingBuffer.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Air/Fly.cs Outdated Show resolved Hide resolved
OpenRA.Game/Primitives/RingBuffer.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Air/Fly.cs Outdated Show resolved Hide resolved
OpenRA.Game/Primitives/RingBuffer.cs Outdated Show resolved Hide resolved
OpenRA.Game/Primitives/RingBuffer.cs Show resolved Hide resolved
OpenRA.Game/Primitives/RingBuffer.cs Show resolved Hide resolved
OpenRA.Game/Primitives/RingBuffer.cs Outdated Show resolved Hide resolved
OpenRA.Game/Primitives/RingBuffer.cs Outdated Show resolved Hide resolved
OpenRA.Game/Primitives/RingBuffer.cs Show resolved Hide resolved
set
{
if (pos >= Count)
throw new ArgumentException($"Index out of bounds: {pos}");
Copy link
Member

Choose a reason for hiding this comment

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

We don't check this for get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it seemed reasonable, to avoid the overhead.

@anvilvapre
Copy link
Contributor Author

ping. fixups were done, apart from the bounds check in the getter, which will be unlikely and obviously tracable by the resulting exception.

@anvilvapre
Copy link
Contributor Author

ping.

@PunkPun PunkPun merged commit 1ce9161 into OpenRA:bleed Aug 2, 2023
3 checks passed
@PunkPun
Copy link
Member

PunkPun commented Aug 2, 2023

Chnagelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants