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

Miscellaneous trait look-up reductions #19166

Merged
merged 3 commits into from
Mar 14, 2021
Merged

Conversation

reaperrr
Copy link
Contributor

Some more trait look-up reductions in places that might not get called that often, but potentially several times at once.

I was too lazy to measure the performance impact (unlikely to be noteworthy, anyway), but those felt simple enough to pass as "can't hurt and might add up, without regressing or over-complicating code".

Copy link
Contributor

@anvilvapre anvilvapre left a comment

Choose a reason for hiding this comment

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

Mods won't be able to add multiple Mobile traits to an actor? In the past it would cause an exception during trait lookup. Now it would cause OccupiesSpace to be used?

OpenRA.Mods.Common/Activities/Move/MoveAdjacentTo.cs Outdated Show resolved Hide resolved
@reaperrr
Copy link
Contributor Author

Updated
Removed first commit less because of the comment, but more because attempted profiling showed MoveAdjacentTo to be far less common than I thought.

Didn't bother profiling the Mobile.Pathfinder changes since that's more polish than anything.

Perf improvements from the 2 CanAttack changes, from left to right (bleed, armament changes only, both), in stopwatch ticks:
canatk-perf

Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, just had a couple of small style nits.

OpenRA.Mods.Common/Traits/Attack/AttackBase.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Attack/AttackBase.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Attack/AttackBase.cs Outdated Show resolved Hide resolved
@reaperrr
Copy link
Contributor Author

Updated.

Copy link
Member

@abcdefg30 abcdefg30 left a comment

Choose a reason for hiding this comment

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

Code changes lgtm but untested

@pchote pchote merged commit 65c796d into OpenRA:bleed Mar 14, 2021
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

4 participants