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

HitShape, query trait via actor cached targetable positions. #20165

Merged
merged 1 commit into from Sep 10, 2022

Conversation

anvilvapre
Copy link
Contributor

@anvilvapre anvilvapre commented Aug 4, 2022

HitShape is the most looked up trait per per actor using TraitsImplementing.

Using this method will search the global TraitDictionary for all actors with the trait and after search for the actor on which the method was called.

Changes:

  • Rather than using TraitsImplementing use the already available enabledTargetablePositions to find HitShape traits.
  • Add Actor.GetEnabledTargetablePositions.
  • Because of this some Linq statements rewritten to foreach loops.

Optional suggestions:

  • Optionally rename Actor.GetTargetablePositions to Actor.GetEnabledTargetableWorldPositions
  • Move IHitShape to OpenRA.Game TraitInterfaces. Add ITargertablePositionsInterface.TargetableShapes() to return an enumeration of HitShapes.

Assumptions:

  • Casting ITargetablePositions to HitShape is faster than each time searching for an actor in the trait container.

Advantage:

  • Save 300k times searching for a actor in the trait container.

Statistics on traits being most often queried for an actor using TraitDictionary.WithInterface. Shelllmap + a short bot game,.

    391 OpenRA.Mods.Common.Traits.IGainsExperienceModifier
    429 OpenRA.Mods.Common.Traits.INotifyProduction
    458 OpenRA.Mods.Common.Traits.Tooltip
    607 OpenRA.Mods.Common.Traits.ISpeedModifier
    719 OpenRA.Mods.Common.Traits.ExternalCondition
    764 OpenRA.Mods.Common.Traits.Locomotor
    932 OpenRA.Mods.Common.Traits.IWallConnector
   1225 OpenRA.Mods.Common.Traits.SquadManagerBotModule
   1355 OpenRA.Mods.Common.Traits.Exit
   1384 OpenRA.Mods.Common.Traits.INotifyDamage
   1384 OpenRA.Mods.Common.Traits.INotifyKilled
   1400 OpenRA.Mods.Common.Traits.INotifyDocking
   1487 OpenRA.Mods.Common.Traits.SupportPower
   1489 OpenRA.Traits.INotifyAddedToWorld
   1661 OpenRA.Mods.Common.Traits.IDamageModifier
   1852 OpenRA.Mods.Common.Traits.Power
   1980 OpenRA.Traits.IProvideTooltipInfo
   1980 OpenRA.Traits.ITooltip
   2308 OpenRA.Mods.Common.Traits.AttackFrontal
   2363 OpenRA.Mods.Common.Traits.GivesBuildableArea
   2819 OpenRA.Traits.ISelectionBar
   6124 OpenRA.Mods.Common.Traits.SmudgeLayer
   6607 OpenRA.Mods.Common.Traits.AttackBase
   6763 OpenRA.Mods.Common.Traits.INotifyHarvesterAction
   8088 OpenRA.Mods.Common.Traits.RevealsShroud
   8374 OpenRA.Mods.Common.Traits.Armament
  16677 OpenRA.Mods.Common.Traits.Armor
  18434 OpenRA.Traits.IVisibilityModifier
  20703 OpenRA.Mods.Common.Traits.IBlocksProjectiles
  26319 OpenRA.Traits.IIssueOrder
  27066 OpenRA.Traits.IRenderAboveShroudWhenSelected
  27066 OpenRA.Traits.IRenderAnnotationsWhenSelected
  46052 OpenRA.Mods.Common.Traits.INotifyAppliedDamage
  57296 (not
  64625 OpenRA.Mods.Common.Traits.RejectsOrders
  77929 OpenRA.Traits.IRenderOverlay
  77931 OpenRA.Traits.IPaletteModifier
  93490 OpenRA.Mods.Common.Traits.IDisableEnemyAutoTarget
  98184 OpenRA.Mods.Common.Traits.ICrushable
 185125 OpenRA.Mods.Common.Traits.INotifyBlockingMove
 324904 OpenRA.Mods.Common.Traits.HitShape

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 the // PERF comment should be added everywhere we cast to HitShape, not just the few instances

OpenRA.Game/Actor.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Warheads/DamageWarhead.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Warheads/SpreadDamageWarhead.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Warheads/TargetDamageWarhead.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Warheads/SpreadDamageWarhead.cs Outdated Show resolved Hide resolved
@anvilvapre
Copy link
Contributor Author

I think the // PERF comment should be added everywhere we cast to HitShape, not just the few instances

Done.

PunkPun
PunkPun previously approved these changes Sep 1, 2022
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.

Untested, but code LGTM

@penev92
Copy link
Member

penev92 commented Sep 1, 2022

Reading around your changes, I noticed some minor housekeeping is in order. I feel it would be best done before the changes in this PR, but since it's small enough that could also be done by you cherry-picking my commit and rebasing yours on top of it.
Or if people would prefer a separate PR I can open one.
The idea is that a few redundant things are removed and you even get our public getter.
bleed...penev92:OpenRA:targetablePositionsCleanup

P.S.: Sorry, I see in the mean time you updated the PR to be closer to what I have, but I still think mine is even cleaner, so my request stands.

@RoosterDragon
Copy link
Member

The reason to cache an IEnumerable such as items.Where(x => x.Foo) is to avoid repeating the allocations that occur for the lambda and the Where machinery. Usually such allocations aren't worth worrying about. But the various helper methods on Actor are called often so it can be worth bearing in mind.

@penev92
Copy link
Member

penev92 commented Sep 4, 2022

Never mind my comment then. This does call for some explanatory comments though, but that's not related to this PR.

Mailaender
Mailaender previously approved these changes Sep 10, 2022
Copy link
Member

@Mailaender Mailaender left a comment

Choose a reason for hiding this comment

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

Hitshapes still function in-game.

@anvilvapre anvilvapre dismissed stale reviews from Mailaender and PunkPun via 48d425e September 10, 2022 16:18
@Mailaender Mailaender merged commit e3aa2dc into OpenRA:bleed Sep 10, 2022
@Mailaender
Copy link
Member

Changelog

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

6 participants