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

Fix not all traits respecting multiple instances of 'Production' #14795

Merged
merged 10 commits into from Nov 3, 2018

Conversation

Projects
None yet
4 participants
@abcdefg30
Copy link
Member

abcdefg30 commented Feb 7, 2018

I'll update ClonesProducedUnits in a follow-up PR.

Note for clarity: Currently the last Harkonnen missions crash (and that's probably just one case; #14796 "fixed" this for the upcoming playtest.) We want to sort all those issues with multiple instances of Produces out for Next+1. (I.e. replace all occurrences of Trait<Produces> and TraitInfo<ProducesInfo>)

@pchote pchote added this to the Next release milestone Feb 7, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Feb 7, 2018

We decided to revert the commit introducing the issue in the meantime: #14796

@abcdefg30 abcdefg30 modified the milestones: Next release, Next + 1 Feb 7, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Feb 7, 2018

Note for clarity: Currently the last Harkonnen missions crash (and that's probably just one case; #14796 "fixed" this for the upcoming playtest.) We want to sort all those issues with multiple instances of Produces out for Next+1. (I.e. replace all occurrences of Trait<Produces> and TraitInfo<ProducesInfo>)

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Feb 7, 2018

Why is 9e2b55a needed?

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Feb 8, 2018

It was a test case commit that was not removed before merging. It makes the build-everything cheat more difficult to use because it shows as an exact duplicate of the Harkonnen conyard in the sidebar. It doesn’t make sense / offer any real benefit to allow the special palace to be built using cheats in MP, especially since we do not do the same for the other special factions.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Feb 8, 2018

Do you have a lua script to exercise the testcase? The lua integration is the main thing that needs testing here.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:traitProduces branch from a02bcf5 to ef61343 Feb 19, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Feb 19, 2018

Updated.

@pchote pchote modified the milestones: Next release, Next + 1 Mar 1, 2018

}

void ITick.Tick(Actor self)
{
var current = queue.CurrentItem();
var q = queue;

This comment has been minimized.

@GraionDilach

GraionDilach Apr 4, 2018

Contributor

Why do we need this variable?

This comment has been minimized.

@abcdefg30

abcdefg30 Apr 8, 2018

Member

How else do you suggest I handle the case when queue == null without reassigning the queue variable? (We could e.g. move the value assignment in if statements, I thought this is the cleaner approach though.)

void FindQueue()
{
var type = info.ProductionType ?? self.Info.TraitInfo<ProductionInfo>().Produces.First();
// Use all queues when no specific one is given

This comment has been minimized.

@GraionDilach

GraionDilach Apr 4, 2018

Contributor

Related to my other nit: wouldn't it make more sense to just FieldLoader.Require the queue type explicitly?

This comment has been minimized.

@abcdefg30

abcdefg30 Apr 8, 2018

Member

Maybe; that sounds like a good idea to simplify the code here. Will have a look.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:traitProduces branch from ef61343 to ae3da3a May 4, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented May 4, 2018

Updated.

get
{
return "The 'ProductionBar' trait now requires the 'ProductionType' to be set.\n" +
"The value be automatically set to the first value in 'Produces' of the first 'Production' trait.";

This comment has been minimized.

@pchote

pchote May 6, 2018

Member

"The value be"

@pchote pchote removed this from the Next release milestone Jul 1, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 1, 2018

While this would be good to have, I don't think it is critical so bumping off the milestone. If you can do the fixup/rebase in time then we should still try and get this in.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Jul 1, 2018

I'm not entirely sure what bleed looks like in that regard (since this PR hasn't been rebased in ages), but we'll want 97cb480 if it's not on bleed yet (iirc it was picked to prep-1801 directly).

@pchote pchote force-pushed the abcdefg30:traitProduces branch 2 times, most recently from 36244b8 to 0875c1c Oct 28, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 28, 2018

I rebased this (on top of #15743 to get the worst of the rebase conflicts out of the way at once), reordered a few things to simplify reviewing, and fixed a few bugs that I noticed.
This also ties in with #15045 which redefines (fixes) the behaviour of disabled Production traits.

The code looks mostly ok, but needs testing before giving a final review.

@pchote pchote force-pushed the abcdefg30:traitProduces branch from 0875c1c to b5e1003 Nov 3, 2018

abcdefg30 and others added some commits Feb 7, 2018

Overhaul ProductionBar:
- Is now a conditional trait
- Now respects multiple Production trait instances
- ProductionType is now required

@pchote pchote force-pushed the abcdefg30:traitProduces branch 3 times, most recently from d7eedd1 to b64daf7 Nov 3, 2018

abcdefg30 and others added some commits Nov 3, 2018

Improve ClonesProducedUnits logic:
- Supports multiple Production trait instances
- Clones the correct faction variant, if defined

@pchote pchote force-pushed the abcdefg30:traitProduces branch from b64daf7 to 40e6bce Nov 3, 2018

@pchote pchote force-pushed the abcdefg30:traitProduces branch 2 times, most recently from 01c587b to ef4f152 Nov 3, 2018

@pchote

pchote approved these changes Nov 3, 2018

Copy link
Member

pchote left a comment

Rebased again, split out specific commits for the different changes, and tested as well as I can.

LGTM, but would like a second review to cover my changes here before we merge.

@pchote pchote added the PR: Needs +2 label Nov 3, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Nov 3, 2018

Your changes look good to me. 👍 /

@abcdefg30 abcdefg30 merged commit f968b16 into OpenRA:bleed Nov 3, 2018

2 checks passed

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

@abcdefg30 abcdefg30 deleted the abcdefg30:traitProduces branch Nov 3, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Nov 3, 2018

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