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

Add ProductionCostMultiplier and ProductionTimeMultiplier Traits #15305

Merged
merged 1 commit into from Apr 22, 2019

Conversation

@MustaphaTR
Copy link
Member

commented Jun 28, 2018

I was wrong with that this solves duplicate D2k actors, as prerequisites are still a concern there (still could remove dupe Carryall and Harvester but i don't think it is a good idea to handle some units differently than others.).

Still this solves the duplicate actors on our RAClassic mod for Russia and Turkey and also should solve @SoScared's problem at #15301. Also allows stuff like Generals Tech Oil Refinery and YR Soviet Industrial Plant.

Selling, bounties etc. still use the valued trait for their calculations.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

I don't have a better idea at the moment but this looks too hacky to be approved. Is this really needed for RA Classic? Can't Russia just use a generic queue-based costmultiplier akin to YR Industrial Plant?

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

Can't Russia just use a generic queue-based costmultiplier akin to YR Industrial Plant?

Our intent is to keep the miscalculated wierd values for Russia so defining them manually for each unit is required.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

Then I vote for introducing a local hack within that mod for it. This is going against OpenRA's evolution itself - exception traits should only be provided for really necessary cases within the primary mods.

I also can't find any documentation about the miscalculation but even if that has a formulae (I presume it just does a wrong rounding) I'm fully behind introducing that formulae as an UseClassicModifierFormulae or something akin to BodyOrientation->UseClassicPredictionFudge than doing it this way.

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2018

I think the proper way of doing this would to support multipile Buildable: traits and making it conditional. A field to override production cost can be the part of Buildable:. This would also allow different prereqs per queue and actually solve D2k dupe starport actors. I think that would require a serious refactor tho.

Edit: It would require some custom code to continue production when switching between different Buildable traits of same actor on same queue to make Industrial Plant/Oil Refinery tho, otherwise it would cancel.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2018

If we're going that broad, then I'd say splitting Buildable to multiple subtraits, each providing a particular functionality would be the way to go, one trait providing cameo, one the cost, one the prerequisites etc..

Either way, I am still against this PR as-is and I don't know how to resolve the issues anyway at this point.

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2018

I think i'm getting multipile buildable traits to work but will need some help.

@pchote

This comment has been minimized.

Copy link
Member

commented Sep 30, 2018

Needs a rebase now

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:costom-production-cost-time branch from d80df4e to 0ebc870 Nov 21, 2018

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:costom-production-cost-time branch from 0ebc870 to 0572112 Nov 21, 2018

@MustaphaTR MustaphaTR changed the title Add CustomProductionCost and CustomProductionTime Traits Add ProductionCostMultiplier and ProductionTimeMultiplier Traits Nov 21, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

Rebased and changed this to be a modifier (with support of making it add additional direct value over the modifier for the stuff needed for RAClassic).

@pchote

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

After our discussion on IRC I am concerned about finding a way forward here that won't actively get in the way of solving the other usecases.

I am overwhelmed with the current glut of PRs introducing design changes across various systems, and don't have any mental bandwidth left to consider the problems here. I would prefer that we left this on the back burner until things quiet down and there is capacity to properly consider the design of our build-requirements plumbing.

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

I'm not expexting this to be reviewed and merged soon, that's why i seperately PRed this to AS.

Also seems like messed up trying to implement this with interfaces, so i'll take more into this tomorrow and actually make it use interfaces.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:costom-production-cost-time branch 4 times, most recently from 89ca131 to cf7e9bf Nov 22, 2018

@pchote
Copy link
Member

left a comment

The code here looks generally fine, and implements the plumbing we need to resolve at least two usecases in the default mods (AI difficulty modifiers and D2K starport prices).

The sticking point is the ExtraCost and ExtraBuildTime values, which I understand are added to be able to replicate the original RA's arbitrary faction cost modifiers.

This model breaks down as soon as you mix multiple traits. Consider adding an industrial-plant style building to RA classic - the ExtraCost won't see the IP modifier, so the cost reduction for Russian players will not be uniformly 25% and players would be quite right to complain that this is a bug. Applying the cost offsets at each step would help, but creates the problem that the final cost depends on the order modifiers are evaluated in.

Arbitrary price/time replacements should be specified as replacement values, and then any real modifiers should act on this replaced value as normal. Duplicating actor definitions in yaml is far from ideal, but this at least interacts cleanly with modifiers. I would prefer that we could keep it this way until someone can implement support for multiple Buildable traits on an actor.

In the mean time we do have use cases for multiplicative modifiers, and they won't be made redundant by multiple Buildable support. So my preference would be to drop those two fields and then move towards getting the rest of the PR merged rather than letting this sit forever.

This is an important part of the AI difficultly levels that I want to implement for the next release, so I can take this over if you'd prefer not to continue with this.

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2019

When i implemented the ExtraCost, ExtraBuildTime, i missed a really important point. I can just use multipile traits to do modifications that are more precise than Cost/100 or BuildTime/100. I agree that they can go and i complately forgot i implemented them at the first place.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:costom-production-cost-time branch from cf7e9bf to 1a08c58 Mar 30, 2019

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2019

Updated.

D2K starport prices This PR alone doesn't solve D2k startport prices btw because cost is not the only thing that needs to be changed, prerequisites too. Current implentation would allow us to only remove duplicates for Trike and Carryall which i didn't do in this PR because it would be an inconsistency.

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

By starport prices I mean the fluctuating costs, which can be added through a new trait that implements IProductionCostModifierInfo. Like I said above, I don't consider the yaml duplication to be an issue worth solving in the short term.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:costom-production-cost-time branch from 1a08c58 to 8640d33 Apr 1, 2019

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

Updated.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:costom-production-cost-time branch from 8640d33 to a57f646 Apr 14, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

This now crashes immediately on game start, because the ProductionQueue constructor calls CacheProducibles before techTree has been assigned.

Moving the CacheProducibles call to Created fixes the issue. The playerResources and developerMode should move there too for consistency.

LGTM after this has been fixed.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:costom-production-cost-time branch from a57f646 to db75702 Apr 21, 2019

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

Updated.

@pchote
pchote approved these changes Apr 22, 2019

@pchote pchote added the PR: Needs +2 label Apr 22, 2019

@reaperrr reaperrr merged commit de7706c into OpenRA:bleed Apr 22, 2019

2 checks passed

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

@MustaphaTR MustaphaTR deleted the MustaphaTR:costom-production-cost-time branch Apr 22, 2019

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