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

Made PowerManager optional for traits who do not require it. #15365

Merged
merged 1 commit into from Jul 26, 2018

Conversation

Projects
None yet
5 participants
@IceReaper
Copy link
Contributor

IceReaper commented Jul 22, 2018

PowerManager is required when using power in a mod. If there should be no power at all, removing this trait will cause a crash due to hardcoded usages. This PR fixes the crash by making PowerManager optional for all traits, who do not need PowerManager to work.

@@ -121,7 +121,7 @@ public ProductionQueue(ActorInitializer init, Actor playerActor, ProductionQueue
self = init.Self;
Info = info;
playerResources = playerActor.Trait<PlayerResources>();
playerPower = playerActor.Trait<PowerManager>();
playerPower = playerActor.TraitOrDefault<PowerManager>();

This comment has been minimized.

@GraionDilach

GraionDilach Jul 22, 2018

Contributor

Carrying over from #15358.

You'll need to move this to INotifyCreated.Created(). Without Requires<> enforcing the classicprodqueue be only loaded after the powermanager, it could be that a classicprodqueue ends up without initializing the power trait within itself.

@IceReaper IceReaper force-pushed the IceReaper:optional_powermanager branch from a797540 to 26cb80f Jul 25, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor Author

IceReaper commented Jul 25, 2018

changes implemented

@IceReaper IceReaper force-pushed the IceReaper:optional_powermanager branch from 26cb80f to 608da0e Jul 25, 2018

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

Eyup, 👍 now.

@pchote
Copy link
Member

pchote left a comment

A couple of minor issues:

@@ -135,6 +136,7 @@ public ProductionQueue(ActorInitializer init, Actor playerActor, ProductionQueue

void INotifyCreated.Created(Actor self)
{
playerPower = playerActor.TraitOrDefault<PowerManager>();

This comment has been minimized.

@pchote

pchote Jul 25, 2018

Member

You should be able to use self.Owner.PlayerActor here and drop the playerActor variable.

This comment has been minimized.

@IceReaper

IceReaper Jul 25, 2018

Author Contributor

self can be a player actor if the queue is attached to a player, so using .Owner.PlayerActor will crash in this case.

This comment has been minimized.

@pchote

pchote Jul 25, 2018

Member

This can be solved as per

protected override void Created(Actor self)
{
// Special case handling is required for the Player actor.
// Created is called before Player.PlayerActor is assigned,
// so we must query other player traits from self, knowing that
// it refers to the same actor as self.Owner.PlayerActor
var playerActor = self.Info.Name == "player" ? self : self.Owner.PlayerActor;
techTree = playerActor.Trait<TechTree>();
Update();
base.Created(self);
}

@@ -152,7 +154,7 @@ void INotifyOwnerChanged.OnOwnerChanged(Actor self, Player oldOwner, Player newO
{
ClearQueue();

playerPower = newOwner.PlayerActor.Trait<PowerManager>();
playerPower = newOwner.PlayerActor.TraitOrDefault<PowerManager>();

This comment has been minimized.

@pchote

pchote Jul 25, 2018

Member

... otherwise you will also need to update playerActor to the new owner here (but i'd prefer we simply didn't define that).

This comment has been minimized.

@IceReaper

IceReaper Jul 26, 2018

Author Contributor

So.. do i need to change this here?

This comment has been minimized.

@pchote

pchote Jul 26, 2018

Member

This should be fine now, I think.

powerLabel.Visible = power != 0;
powerIcon.Visible = power != 0;
var powerSize = font.Measure(powerLabel.Text);
var powerSize = new int2(0, 0);

This comment has been minimized.

@pchote

pchote Jul 25, 2018

Member

This doesn't appear to be used?

This comment has been minimized.

@IceReaper

IceReaper Jul 25, 2018

Author Contributor

Line 122 and 129. it is used ;)

This comment has been minimized.

@pchote

pchote Jul 26, 2018

Member

Right, I see now. My bad!

@IceReaper IceReaper force-pushed the IceReaper:optional_powermanager branch from 608da0e to ece7f37 Jul 25, 2018

@@ -135,6 +134,7 @@ public ProductionQueue(ActorInitializer init, Actor playerActor, ProductionQueue

void INotifyCreated.Created(Actor self)
{
playerPower = (self.Info.Name == "player" ? self : self.Owner.PlayerActor).TraitOrDefault<PowerManager>();

This comment has been minimized.

@pchote

pchote Jul 26, 2018

Member

Can you please include the same comment from ProvidesPrerequisite here too?

@IceReaper IceReaper force-pushed the IceReaper:optional_powermanager branch from ece7f37 to 2edf888 Jul 26, 2018

@pchote

pchote approved these changes Jul 26, 2018

Copy link
Member

pchote left a comment

A better long-term fix would be to remove the hardcoded link between ProductionQueue and PowerManager by introducing a production speed modifier interface.

This looks like a good short term fix, however.

@pchote pchote merged commit 81e1b39 into OpenRA:bleed Jul 26, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@IceReaper IceReaper deleted the IceReaper:optional_powermanager branch Jul 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.