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

Implements all kind of resource modifiers. #15798

Merged
merged 4 commits into from May 22, 2019

Conversation

@MustaphaTR
Copy link
Member

commented Nov 7, 2018

Revives #15283.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:resourcepurifier branch 2 times, most recently from 56a5b39 to 7858b2d Nov 7, 2018

@GraionDilach

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2018

External is Ore Purifier logic - it is parallel to the Harvester/Refinery modifiers and is not cumulative to them.

I also don't intend to return to this project but I'm more than eager to defend my own code.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:resourcepurifier branch from 7858b2d to e1413ff Nov 7, 2018

@pchote

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

Adding the changes label based on the IRC discussion several days ago, which concluded with

[14:16:51] | <MustaphaTR> Lemme go make it use a Interface for now, we can discuss further later.
@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2018

I have already done that, btw.

@pchote

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

My understanding of that discussion was that you had agreed that ExternalResourceMultiplier didn't belong upstream, but it is still in this PR?

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:resourcepurifier branch from e1413ff to d59f5c8 Nov 17, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2018

Updated as discussed on IRC.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:resourcepurifier branch 2 times, most recently from 3c0f4e5 to 89d2f1d Mar 15, 2019

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

Rebased.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:resourcepurifier branch from 89d2f1d to d34506d Apr 14, 2019

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:resourcepurifier branch from d34506d to 80508cb May 4, 2019

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

Updated.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:resourcepurifier branch 2 times, most recently from 3d742c9 to 42b8610 May 4, 2019

@pchote
Copy link
Member

left a comment

LGTM, just two minor nits that I missed last time around.

OpenRA.Mods.Cnc/Traits/ResourcePurifier.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/TraitsInterfaces.cs Show resolved Hide resolved

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:resourcepurifier branch from 1a014e9 to 9a3f641 May 16, 2019

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Updated.

@abcdefg30
Copy link
Member

left a comment

This crashes for me when putting ResourcePurifier on a player actor:

Exception of type `System.NullReferenceException`: Der Objektverweis wurde nicht auf eine Objektinstanz festgelegt.
   at OpenRA.Mods.Cnc.Traits.ResourcePurifier.<>c__DisplayClass6_1.<OpenRA.Traits.ITick.Tick>b__0(World w)
   at OpenRA.World.Tick()
   at OpenRA.Game.InnerLogicTick(OrderManager orderManager)
   at OpenRA.Game.LogicTick()
   at OpenRA.Game.Loop()
   at OpenRA.Game.Run()
   at OpenRA.Game.InitializeAndRun(String[] args)
   at OpenRA.Program.Main(String[] args)
@pchote

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Moving the PlayerResources trait lookup from the constructor to INotifyCreated.Created should fix that.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:resourcepurifier branch from 9a3f641 to 85a86d3 May 16, 2019

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Updated.

@abcdefg30

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Still crashes because player actors don't have a CenterPosition. We should force-disable/omit the floating text in that case imho.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:resourcepurifier branch from 85a86d3 to 38736ff May 20, 2019

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Updated.

OpenRA.Mods.Cnc/Traits/ResourcePurifier.cs Outdated Show resolved Hide resolved

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:resourcepurifier branch from 38736ff to a93f2dd May 21, 2019

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

Updated.

@abcdefg30 abcdefg30 merged commit 3709380 into OpenRA:bleed May 22, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@MustaphaTR MustaphaTR deleted the MustaphaTR:resourcepurifier branch May 25, 2019

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