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

Refactor SupplyTruck/AcceptsSupplies to *Cash traits #13000

Merged
merged 2 commits into from Apr 18, 2017

Conversation

reaperrr
Copy link
Contributor

@reaperrr reaperrr commented Mar 19, 2017

It's only used by our RA mod and no more likely to be used by any 3rd-party mod than any other RA-specific logic (less likely, in my opinion), so I think it can be moved to Mods.Cnc.

Renamed to more generic DeliversCash/AcceptsDeliveredCash and added Type/Stance tags instead.

@MustaphaTR
Copy link
Member

Actually, i'm using Supply Truck logic on the D2K map i was working on which is currently on bleed, but will keep it on release when it happens.

I wouldn't mind much if this won't effect the Next but Next + 1. I don't think i would update the map anyway when that happens.

@reaperrr
Copy link
Contributor Author

@MustaphaTR D2k currently needs the Mods.Cnc.dll anyway due to ProductionAirdrop, and I don't see that changing soon (not before Next+1 in any case).

@reaperrr
Copy link
Contributor Author

Alternatively, we could keep it in Common and rename the trait to SupplyActor or SupplyDeliverer or DeliversSupplies.

@GraionDilach
Copy link
Contributor

GraionDilach commented Mar 19, 2017

Considering that these could be used for the YR Grinder even, I'd say it just needs a tag property.

@MustaphaTR
Copy link
Member

Yea, i keep forgetting that D2k read CnC dll.

But Graion is right, this is the closet thing we have to grinder. So keeping it on common, renaming it and adding a Type: support for AcceptsSupplies and SupplyTruck so multipile types can be used (both actual Supply Trucks and Grinder for example).

@reaperrr reaperrr changed the title Move SupplyTruck logic to Mods.Cnc Refactor SupplyTruck to DeliversSupplies Mar 20, 2017
@reaperrr
Copy link
Contributor Author

Updated as suggested.

@GraionDilach
Copy link
Contributor

Ah, I forgot this doesn't have a stance check. May you add that as well? 👍 after it.

Copy link
Contributor

@GraionDilach GraionDilach left a comment

Choose a reason for hiding this comment

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

👍

@Iamgoofball
Copy link
Contributor

#13000

pchote
pchote previously requested changes Apr 9, 2017
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.

"Supplies" is still a mod-specific name (what's a supply?). Can you please change these to DeliversCash and AcceptsDeliveredCash or something similar?

@pchote
Copy link
Member

pchote commented Apr 9, 2017

This PR supersedes #5925, so that one can be dropped from the Future milestone after this is merged. It also checks another box from #6336.

@reaperrr reaperrr changed the title Refactor SupplyTruck to DeliversSupplies Refactor SupplyTruck/AcceptsSupplies to *Cash traits Apr 15, 2017
@reaperrr
Copy link
Contributor Author

Updated.

Note: Removing AcceptsSupplies from TS instead of renaming is intentional, that looks like copy-pasta from the early days of the mod.

@atlimit8 atlimit8 dismissed pchote’s stale review April 17, 2017 15:59

Requested renaming has been applied.

@GraionDilach
Copy link
Contributor

Probably you should also rename the DonateSupplies activity as well.

@reaperrr
Copy link
Contributor Author

Probably you should also rename the DonateSupplies activity as well.

Done.

Copy link
Contributor

@GraionDilach GraionDilach left a comment

Choose a reason for hiding this comment

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

👍 still stands.

Copy link
Contributor

@obrakmann obrakmann left a comment

Choose a reason for hiding this comment

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

👍

@obrakmann obrakmann merged commit 843ac85 into OpenRA:bleed Apr 18, 2017
@obrakmann
Copy link
Contributor

Changelog

@reaperrr reaperrr deleted the supply-to-cnc branch July 23, 2017 07:52
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

6 participants