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

Remove IExplodeModifier interface #15825

Merged
merged 4 commits into from Nov 22, 2018

Conversation

Projects
None yet
4 participants
@pchote
Copy link
Member

pchote commented Nov 17, 2018

This PR removes IExplodeModifier with the aim of unblocking #15453.

CC @IceReaper.

Show resolved Hide resolved OpenRA.Mods.Common/Traits/Conditions/GrantConditionOnPlayerResources.cs Outdated
@@ -70,13 +70,17 @@ public class HarvesterInfo : ITraitInfo, Requires<MobileInfo>
[Desc("The pathfinding cost penalty applied for each harvester waiting to unload at a refinery.")]
public readonly int UnloadQueueCostModifier = 12;

[GrantedConditionReference]
[Desc("Condition to grant while empty.")]
public readonly string EmptyCondition = null;

This comment has been minimized.

@MustaphaTR

MustaphaTR Nov 17, 2018

Member

Would be cool if we can define values here. We could drop whatever that handles the empty-half-full artwork to use conditions instead too.

There is what i used for Generals Alpha for supply logic: https://github.com/MustaphaTR/OpenRA/blob/generals-alpha-engine/OpenRA.Mods.Gen/Traits/Supply/SupplyCollector.cs#L61 https://github.com/MustaphaTR/OpenRA/blob/generals-alpha-engine/OpenRA.Mods.Gen/Traits/Supply/SupplyDock.cs#L37
Here we could use bale amount instead, since value depends on what is in.

This comment has been minimized.

@pchote

pchote Nov 17, 2018

Member

The whole bale system needs to be rethought as part of a wider player resource rework. I think it would be a mistake to further bake in what we already know are bad abstractions now.

@pchote pchote force-pushed the pchote:remove-explode-modifier branch from 9b92c9f to 770efcd Nov 18, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 18, 2018

Updated.

@obrakmann
Copy link
Contributor

obrakmann left a comment

lgtm, but needs a rebase due to conflicts

@pchote pchote removed the PR: Rebase me! label Nov 22, 2018

@pchote pchote dismissed stale reviews from obrakmann and reaperrr via 8d04d9a Nov 22, 2018

@pchote pchote force-pushed the pchote:remove-explode-modifier branch from 770efcd to 8d04d9a Nov 22, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 22, 2018

Rebased

@obrakmann obrakmann merged commit 807a40c into OpenRA:bleed Nov 22, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

obrakmann commented Nov 22, 2018

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