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

Rework the addition and removal of building influence #13797

Merged
merged 1 commit into from Aug 31, 2017

Conversation

Projects
None yet
5 participants
@abcdefg30
Member

abcdefg30 commented Aug 8, 2017

Pulled from #13796. This should improve performance a tiny bit as we don't do the extra trait query for BuildingInfo whenever an actor is added to or removed from the world.

@rob-v

lgtm, a nit/question.

@@ -274,6 +277,7 @@ public Building(ActorInitializer init, BuildingInfo info)
self = init.Self;
topLeft = init.Get<LocationInit, CPos>();
Info = info;
influence = self.World.WorldActor.Trait<BuildingInfluence>();

This comment has been minimized.

@rob-v

rob-v Aug 18, 2017

Contributor

Is BuildingInfluence mandatory for World actor?

@rob-v

rob-v Aug 18, 2017

Contributor

Is BuildingInfluence mandatory for World actor?

This comment has been minimized.

@abcdefg30

abcdefg30 Aug 23, 2017

Member

Looks like it, see IsCloseEnoughToBase (line 192 here).

@abcdefg30

abcdefg30 Aug 23, 2017

Member

Looks like it, see IsCloseEnoughToBase (line 192 here).

Show outdated Hide outdated OpenRA.Mods.Common/Traits/Buildings/BuildingInfluence.cs Outdated
@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Aug 23, 2017

Member

Updated.

Member

abcdefg30 commented Aug 23, 2017

Updated.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Aug 23, 2017

Contributor

I didn't check if it is standard practice or how much it would improve it, but can we move BuildingInfluence influence; to BuildingInfo to retrieve it only once for all Buildings?

Contributor

rob-v commented Aug 23, 2017

I didn't check if it is standard practice or how much it would improve it, but can we move BuildingInfluence influence; to BuildingInfo to retrieve it only once for all Buildings?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 23, 2017

Member

Not standard practice, and isn't likely to help enough to justify the extra complexity. Note that each actor type will have a different BuildingInfo instance.

Member

pchote commented Aug 23, 2017

Not standard practice, and isn't likely to help enough to justify the extra complexity. Note that each actor type will have a different BuildingInfo instance.

@rob-v

rob-v approved these changes Aug 23, 2017

👍

@Mailaender

Looking good 👍 and tested in-game.

@Mailaender Mailaender merged commit 5cfb5aa into OpenRA:bleed Aug 31, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender
Member

Mailaender commented Aug 31, 2017

@abcdefg30 abcdefg30 deleted the abcdefg30:unInfluence branch Sep 11, 2017

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