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

Exit Types #14098

Merged
merged 1 commit into from Dec 13, 2017

Conversation

Projects
None yet
5 participants
@GSonderling
Contributor

GSonderling commented Oct 1, 2017

Most work was done by @atlimit8. I just solved merge conflicts, changed few tags in yaml files and got rid of issue with blocked exit point.

In the linked issue you can find more details about the whole process. Also I know that my solution for issue with blocked exit is not exactly the best. So if anyone knows about better way, I am open to suggestions.

I tested the code in ra, d2k, cnc and ts. I have been running AI only (8-12 bots) skirmishes, normal offline skirmish with one human player (me) and on a makeshift testing map with me and sometimes one bot.

I used default settings for everything apart from money and time, which I set to highest and fastest respectively.

In response to issue:
#14065

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Oct 1, 2017

Contributor

I will squash the commits after everything checks out. Sorry for inconvenience.

Contributor

GSonderling commented Oct 1, 2017

I will squash the commits after everything checks out. Sorry for inconvenience.

@pchote

Thanks for taking this on! Here are a couple of issues to start with:

Show outdated Hide outdated OpenRA.Mods.Common/Traits/Buildings/Exit.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Cnc/Traits/Buildings/ClonesProducedUnits.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Cnc/Traits/Buildings/ProductionAirdrop.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Mobile.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Production.cs Outdated
Show outdated Hide outdated mods/ra/rules/defaults.yaml Outdated
@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Oct 3, 2017

Contributor

Anything else I should fix?

Contributor

GSonderling commented Oct 3, 2017

Anything else I should fix?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 14, 2017

Member

Could you please squash your code changes into the original code commit, and your yaml changes into the original yaml commits for each mod? This is something that's easy to do if you know how, but very intimidating if you don't -- find me in IRC if you want some tips.

Member

pchote commented Oct 14, 2017

Could you please squash your code changes into the original code commit, and your yaml changes into the original yaml commits for each mod? This is something that's easy to do if you know how, but very intimidating if you don't -- find me in IRC if you want some tips.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 14, 2017

Member

I think we can drop all the explicit exits in TD/D2K/TS and all but the naval yards and barracks / kennel in RA?

Member

pchote commented Oct 14, 2017

I think we can drop all the explicit exits in TD/D2K/TS and all but the naval yards and barracks / kennel in RA?

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Oct 14, 2017

Contributor

I am almost done with the fixes. Don't worry I should be able to squash commits, I was just waiting until everything is as you need it too. No point in doing something something twice.

Anyway about dropping those exits: I don't think we should do that. First reason, obviously, is that it causes no harm and does not impact performance. It also causes no anomalous behavior or bugs. As far as I can see at least.
Second reason is that those exits, while unnecessary from functional perspective, are quite useful from modder perspective and for debugging. Their existence makes clear to modders that there can be more of them and that exittypes are not feature exclusive to specific buildings or queues.

As for debugging: if there are issues with exits in the future, it would save us some time if we can easily decide that the problem lies in specified exit, instead of walking trough the code and looking for problems with default value.

Finally, the code will, in my perspective, look better if it is uniform.

Update: Testing showed no problems so far.
Tested: TS, RA, CNC, D2K each for one hour.

Contributor

GSonderling commented Oct 14, 2017

I am almost done with the fixes. Don't worry I should be able to squash commits, I was just waiting until everything is as you need it too. No point in doing something something twice.

Anyway about dropping those exits: I don't think we should do that. First reason, obviously, is that it causes no harm and does not impact performance. It also causes no anomalous behavior or bugs. As far as I can see at least.
Second reason is that those exits, while unnecessary from functional perspective, are quite useful from modder perspective and for debugging. Their existence makes clear to modders that there can be more of them and that exittypes are not feature exclusive to specific buildings or queues.

As for debugging: if there are issues with exits in the future, it would save us some time if we can easily decide that the problem lies in specified exit, instead of walking trough the code and looking for problems with default value.

Finally, the code will, in my perspective, look better if it is uniform.

Update: Testing showed no problems so far.
Tested: TS, RA, CNC, D2K each for one hour.

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Oct 14, 2017

Contributor

Ok, Testing is finished and now just waiting for checks. If the code turns out to be of sufficient quality, I will squash the commits.

Contributor

GSonderling commented Oct 14, 2017

Ok, Testing is finished and now just waiting for checks. If the code turns out to be of sufficient quality, I will squash the commits.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 15, 2017

Member

Anyway about dropping those exits: I don't think we should do that.

My motivation for not wanting them is:

  1. It adds unnecessary complexity to the yaml, which invariably causes bugs... especially in missions and maps that override the default ruleset. It also sets a bad precedent for other mods, which look at our default mods for examples.
  2. It makes testing harder: we now need to check every production structure in every mod instead of the default case and the exceptions.
  3. It hides any potential bugs in the default case.

Don't worry I should be able to squash commits, I was just waiting until everything is as you need it too. No point in doing something something twice.

The contents and structure of the commits are part of the code review, so not squashing them forces us to review them twice (which is a lot more work than squashing).

Member

pchote commented Oct 15, 2017

Anyway about dropping those exits: I don't think we should do that.

My motivation for not wanting them is:

  1. It adds unnecessary complexity to the yaml, which invariably causes bugs... especially in missions and maps that override the default ruleset. It also sets a bad precedent for other mods, which look at our default mods for examples.
  2. It makes testing harder: we now need to check every production structure in every mod instead of the default case and the exceptions.
  3. It hides any potential bugs in the default case.

Don't worry I should be able to squash commits, I was just waiting until everything is as you need it too. No point in doing something something twice.

The contents and structure of the commits are part of the code review, so not squashing them forces us to review them twice (which is a lot more work than squashing).

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Oct 15, 2017

Contributor

Why would warfactory-explicit exits be dropped? How would the dooranims function otherwise?

Contributor

GraionDilach commented Oct 15, 2017

Why would warfactory-explicit exits be dropped? How would the dooranims function otherwise?

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Oct 15, 2017

Contributor

It is my understanding that, without specific exit defined, engine switches to default behavior.
Anyway, I'm going to remove those traits tomorrow. They are in lot of actors and I will have to do it by hand. No more relying on automation in this, I learned my lesson.

Contributor

GSonderling commented Oct 15, 2017

It is my understanding that, without specific exit defined, engine switches to default behavior.
Anyway, I'm going to remove those traits tomorrow. They are in lot of actors and I will have to do it by hand. No more relying on automation in this, I learned my lesson.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 15, 2017

Member

You can unstage the changed lines using your git client, it should only takes a few clicks or keystrokes! Let me know in IRC if you'd like some pointers.

Member

pchote commented Oct 15, 2017

You can unstage the changed lines using your git client, it should only takes a few clicks or keystrokes! Let me know in IRC if you'd like some pointers.

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Oct 15, 2017

Contributor

I decided to give it a shot and, well results are not great. After removing default exit traits the games started crashing. It seems that some buildings have trouble finding any exit.

Edit: And other stuff is broken too, War factories can't build units and I wouldn't be surprised if other structures were experiencing similar issues. So far I have only tested ra and cnc mods.
Edit2: Yep, it affects every structure without exits defined in YAML.

Contributor

GSonderling commented Oct 15, 2017

I decided to give it a shot and, well results are not great. After removing default exit traits the games started crashing. It seems that some buildings have trouble finding any exit.

Edit: And other stuff is broken too, War factories can't build units and I wouldn't be surprised if other structures were experiencing similar issues. So far I have only tested ra and cnc mods.
Edit2: Yep, it affects every structure without exits defined in YAML.

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Oct 17, 2017

Contributor

Just to make it completely clear, so that there is no chance of me messing up.
Which part of YAML code you wanted to remove @pchote ?

Contributor

GSonderling commented Oct 17, 2017

Just to make it completely clear, so that there is no chance of me messing up.
Which part of YAML code you wanted to remove @pchote ?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 22, 2017

Member

All the ProductionTypes lines that don't change anything compared with specifying nothing (everything but spen, I think?).

Sorry, but #14227 has gone and caused a bunch of rebase conflicts here. It should be straightforward to resolve, but can you please make sure that you add the new productionType before inits in the argument lists?

Member

pchote commented Oct 22, 2017

All the ProductionTypes lines that don't change anything compared with specifying nothing (everything but spen, I think?).

Sorry, but #14227 has gone and caused a bunch of rebase conflicts here. It should be straightforward to resolve, but can you please make sure that you add the new productionType before inits in the argument lists?

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Oct 22, 2017

Contributor

It might take a while, but I'm on it.

Contributor

GSonderling commented Oct 22, 2017

It might take a while, but I'm on it.

@AndriiYukhymchak

This comment has been minimized.

Show comment
Hide comment
@AndriiYukhymchak

AndriiYukhymchak Oct 22, 2017

Contributor

rebase?

Contributor

AndriiYukhymchak commented Oct 22, 2017

rebase?

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Oct 23, 2017

Contributor

Yeah, it kind of messed up the commit history.

Contributor

GSonderling commented Oct 23, 2017

Yeah, it kind of messed up the commit history.

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Oct 23, 2017

Contributor

Commits are squashed and merge conflicts (hopefully) resolved.

Contributor

GSonderling commented Oct 23, 2017

Commits are squashed and merge conflicts (hopefully) resolved.

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Oct 26, 2017

Contributor

Guys please take a look at this before we get more conflicts.

Contributor

GSonderling commented Oct 26, 2017

Guys please take a look at this before we get more conflicts.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 26, 2017

Member

Thanks for squashing. At a quick look this is looking more reasonable, except for the Mobile blocking thing which I mentioned in a now-outdated comment thread above:

This check is hiding the real bug, which is at

static bool CanUseExit(Actor self, ActorInfo producee, ExitInfo s)
{
var mobileInfo = producee.TraitInfoOrDefault<MobileInfo>();
self.NotifyBlocker(self.Location + s.ExitCell);
return mobileInfo == null ||
mobileInfo.CanEnterCell(self.World, self, self.Location + s.ExitCell, self);
}

Somewhere down the call chain from self.NotifyBlocker it is assuming that the caller is Mobile, and somehow bypassing the normal trait system to call this method despite not having the required trait.

Fixing this might be worth a PR of its own.

What i'd really like to know is why it has only become a problem now, with the exit filtering

Unfortunately i'm going to be travelling for the next couple of weeks, and am unlikely to have the time to meaningfully review PRs.

Member

pchote commented Oct 26, 2017

Thanks for squashing. At a quick look this is looking more reasonable, except for the Mobile blocking thing which I mentioned in a now-outdated comment thread above:

This check is hiding the real bug, which is at

static bool CanUseExit(Actor self, ActorInfo producee, ExitInfo s)
{
var mobileInfo = producee.TraitInfoOrDefault<MobileInfo>();
self.NotifyBlocker(self.Location + s.ExitCell);
return mobileInfo == null ||
mobileInfo.CanEnterCell(self.World, self, self.Location + s.ExitCell, self);
}

Somewhere down the call chain from self.NotifyBlocker it is assuming that the caller is Mobile, and somehow bypassing the normal trait system to call this method despite not having the required trait.

Fixing this might be worth a PR of its own.

What i'd really like to know is why it has only become a problem now, with the exit filtering

Unfortunately i'm going to be travelling for the next couple of weeks, and am unlikely to have the time to meaningfully review PRs.

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Oct 27, 2017

Contributor

Well as long as no more conflicts crop up this PR can wait.

About the Mobile bug:

The trail from that place is pretty long. As far as I can tell the problem is not in
self.NotifyBlocker(self.Location + s.ExitCell); but in mobileInfo.CanEnterCell(self.World, self, self.Location + s.ExitCell, self);

The thing is, the Mobile trait belongs to the unit being produced, not to the building. Unfortunately the CanEnterCell() function doesn't care about the arguments.

The result obviously makes no sense.

But fixing it there would require a more extensive rewrite and this PR is already dragging for too long.

Contributor

GSonderling commented Oct 27, 2017

Well as long as no more conflicts crop up this PR can wait.

About the Mobile bug:

The trail from that place is pretty long. As far as I can tell the problem is not in
self.NotifyBlocker(self.Location + s.ExitCell); but in mobileInfo.CanEnterCell(self.World, self, self.Location + s.ExitCell, self);

The thing is, the Mobile trait belongs to the unit being produced, not to the building. Unfortunately the CanEnterCell() function doesn't care about the arguments.

The result obviously makes no sense.

But fixing it there would require a more extensive rewrite and this PR is already dragging for too long.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 15, 2017

Member

What i'd really like to know is why it has only become a problem now, with the exit filtering

I'd like to reask/reiterate/reinforce this point, because I can't justify spending an unknown number of hours trying to get to the bottom of this while we have > 70 other open PRs waiting on review, and it seems that everybody else is going to ignore this PR until I give it a 👍 😞

If the exit blocking bug already exists in the current game then could you please drop the workaround from this PR so that we can merge the type filtering? If that really is a new regression from the other changes here, then where/why? The CanUseExit method is very old code, and I don't see it being used in any obviously different way in this PR. If you can point me in the right direction then it would save a lot of time and hopefully get progress on merging this moving again.

Member

pchote commented Nov 15, 2017

What i'd really like to know is why it has only become a problem now, with the exit filtering

I'd like to reask/reiterate/reinforce this point, because I can't justify spending an unknown number of hours trying to get to the bottom of this while we have > 70 other open PRs waiting on review, and it seems that everybody else is going to ignore this PR until I give it a 👍 😞

If the exit blocking bug already exists in the current game then could you please drop the workaround from this PR so that we can merge the type filtering? If that really is a new regression from the other changes here, then where/why? The CanUseExit method is very old code, and I don't see it being used in any obviously different way in this PR. If you can point me in the right direction then it would save a lot of time and hopefully get progress on merging this moving again.

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Nov 16, 2017

Contributor

Ok here is what I know at this point:

  1. The problem appears only in this PR. The bleed branch doesn't have this bug.

  2. The problem is related to way exits are handled and is almost exclusive to airstrip in CNC.

  3. In bleed the CanUseExit is never called with airstrip Actor as parameter. But other buildings are.

  4. When the it reaches DoProduction method. It is already too late, exitInfo is null.

  5. CanMoveFreelyInto method of MobileInfo calls IsBlockedBy method. To check if unit can enter the cell. As long as IsBlockedBy returns false, nothing bad happens.

  6. IsBlockedBy works normally, as long as the otherActor is the building itself. Because in those cases it's ignored. However, if it's not we have an issue. The method returns true, the we are blocked, and since airstrip has only one exit we end up with exception.

  7. The position of the airstrip seems irrelevant. In one case it was the edge of the map, in others middle. The cell checked for blockage was, for example, {28, 75} on "Blood and Sand".

I will get back to this.

Contributor

GSonderling commented Nov 16, 2017

Ok here is what I know at this point:

  1. The problem appears only in this PR. The bleed branch doesn't have this bug.

  2. The problem is related to way exits are handled and is almost exclusive to airstrip in CNC.

  3. In bleed the CanUseExit is never called with airstrip Actor as parameter. But other buildings are.

  4. When the it reaches DoProduction method. It is already too late, exitInfo is null.

  5. CanMoveFreelyInto method of MobileInfo calls IsBlockedBy method. To check if unit can enter the cell. As long as IsBlockedBy returns false, nothing bad happens.

  6. IsBlockedBy works normally, as long as the otherActor is the building itself. Because in those cases it's ignored. However, if it's not we have an issue. The method returns true, the we are blocked, and since airstrip has only one exit we end up with exception.

  7. The position of the airstrip seems irrelevant. In one case it was the edge of the map, in others middle. The cell checked for blockage was, for example, {28, 75} on "Blood and Sand".

I will get back to this.

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Nov 17, 2017

Contributor
  1. The Produce method in ProductionAirdrop is where the things start to go wrong.
    Instead of just picking first exit as it used to it checks all available exits. And it does so using the CanUseExit, which itself uses CanEnterCell, CanEnterCell and finally IsBlockedBy.

  2. The crux of the issue is: CanUseExit does not cover the case of no exit being available! If that happens we get exception.

In conclusion:

We can default to picking random, or first, exit if no exit seems to be available. Otherwise we can check if the building isn't passed to IsBlockedBy as self, which is what I have implemented now.

Edit: Finally, we could implement feature blocking production if exits are blocked, but that seems to be way more trouble than it's worth.

Pick which option you want. Personally, I think sooner we stop this the better. But we should also make sure that it doesn't come up in other context.

Contributor

GSonderling commented Nov 17, 2017

  1. The Produce method in ProductionAirdrop is where the things start to go wrong.
    Instead of just picking first exit as it used to it checks all available exits. And it does so using the CanUseExit, which itself uses CanEnterCell, CanEnterCell and finally IsBlockedBy.

  2. The crux of the issue is: CanUseExit does not cover the case of no exit being available! If that happens we get exception.

In conclusion:

We can default to picking random, or first, exit if no exit seems to be available. Otherwise we can check if the building isn't passed to IsBlockedBy as self, which is what I have implemented now.

Edit: Finally, we could implement feature blocking production if exits are blocked, but that seems to be way more trouble than it's worth.

Pick which option you want. Personally, I think sooner we stop this the better. But we should also make sure that it doesn't come up in other context.

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Nov 18, 2017

Contributor

If you want the first option, I propose following change to SelectExit method in Production.cs (line 156) :

protected ExitInfo SelectExit(Actor self, ActorInfo producee, string productionType)
		{
			return SelectExit(self, producee, productionType, e => CanUseExit(self, producee, e)) ?? self.Info.TraitInfos<ExitInfo>().First();
		}

Otherwise we could put similar change in further downstream but this one seems rather elegant.

Contributor

GSonderling commented Nov 18, 2017

If you want the first option, I propose following change to SelectExit method in Production.cs (line 156) :

protected ExitInfo SelectExit(Actor self, ActorInfo producee, string productionType)
		{
			return SelectExit(self, producee, productionType, e => CanUseExit(self, producee, e)) ?? self.Info.TraitInfos<ExitInfo>().First();
		}

Otherwise we could put similar change in further downstream but this one seems rather elegant.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 18, 2017

Member

Thanks for the detailed steps to reproduce.

The problem here is that ProductionAirdrop doesn't (and, with the current implementation, can't) have any logic to handle the case where all exits are blocked. The other production types are able to pause the production (or in the case of ProductionParadrop continue the drop and let the unit die), and so this problem really is isolated to that one hacky trait.

The original code acknowledged this limitation with

// Assume a single exit point for simplicity
var exit = self.Info.TraitInfos<ExitInfo>().First();

which this PR replaces with a "normal" exit query. I managed to miss that change the first time around 😄

The simplest and my preferred fix is to revert that change so that ProductionAirdrop continues to ignore the exit status and use always the first one. This avoids pushing workarounds for mod-specific design limitations into the shared trait code.

Member

pchote commented Nov 18, 2017

Thanks for the detailed steps to reproduce.

The problem here is that ProductionAirdrop doesn't (and, with the current implementation, can't) have any logic to handle the case where all exits are blocked. The other production types are able to pause the production (or in the case of ProductionParadrop continue the drop and let the unit die), and so this problem really is isolated to that one hacky trait.

The original code acknowledged this limitation with

// Assume a single exit point for simplicity
var exit = self.Info.TraitInfos<ExitInfo>().First();

which this PR replaces with a "normal" exit query. I managed to miss that change the first time around 😄

The simplest and my preferred fix is to revert that change so that ProductionAirdrop continues to ignore the exit status and use always the first one. This avoids pushing workarounds for mod-specific design limitations into the shared trait code.

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Nov 18, 2017

Contributor

Ok, if this is what you want I will revert that change. Although I would prefer, to append
?? self.Info.TraitInfos<ExitInfo>().First(); to existing line instead, so that we still have at least some aspects of Exit mechanics preserved.

Edit: Running one final test before push.

Contributor

GSonderling commented Nov 18, 2017

Ok, if this is what you want I will revert that change. Although I would prefer, to append
?? self.Info.TraitInfos<ExitInfo>().First(); to existing line instead, so that we still have at least some aspects of Exit mechanics preserved.

Edit: Running one final test before push.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 18, 2017

Member

I guess I wouldn't object to that (assuming you mean adding it in ProductionAirdrop). In practice it won't change anything though, as these structures only have one exit anyway.

Member

pchote commented Nov 18, 2017

I guess I wouldn't object to that (assuming you mean adding it in ProductionAirdrop). In practice it won't change anything though, as these structures only have one exit anyway.

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Nov 18, 2017

Contributor

The code is pushed and I will wait for all checks. It is possible that some line endings got messed up.

Contributor

GSonderling commented Nov 18, 2017

The code is pushed and I will wait for all checks. It is possible that some line endings got messed up.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 18, 2017

Member

Travis raises a style issue:

Checking for code style violations in OpenRA.Mods.Common...

OpenRA.Mods.Common/Traits/Mobile.cs:L310: [SP2000] Invalid spacing at the end of the line.

make: *** [check] Error 1
Member

pchote commented Nov 18, 2017

Travis raises a style issue:

Checking for code style violations in OpenRA.Mods.Common...

OpenRA.Mods.Common/Traits/Mobile.cs:L310: [SP2000] Invalid spacing at the end of the line.

make: *** [check] Error 1
@pchote

A couple of minor style nits, and unfortunately a regression:

Show outdated Hide outdated OpenRA.Mods.Cnc/Traits/Buildings/ProductionAirdrop.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Mobile.cs Outdated
Show outdated Hide outdated mods/ra/rules/defaults.yaml Outdated
Show outdated Hide outdated mods/ra/rules/defaults.yaml Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Production.cs Outdated
SpawnOffset: -1024,1024,0
Facing: 160
ExitCell: 0,2
ProductionTypes: Ship

This comment has been minimized.

@pchote

pchote Nov 18, 2017

Member

I'm not able to get ships to produce from the sub pen at all, even after fixing SelectExit to return null when blocked. Not sure what the problem is without digging deeper.

@pchote

pchote Nov 18, 2017

Member

I'm not able to get ships to produce from the sub pen at all, even after fixing SelectExit to return null when blocked. Not sure what the problem is without digging deeper.

This comment has been minimized.

@GSonderling

GSonderling Nov 18, 2017

Contributor

You can't produce transports? Or you can't produce other ships? Because I thought subpens should only make submarines and transports.

@GSonderling

GSonderling Nov 18, 2017

Contributor

You can't produce transports? Or you can't produce other ships? Because I thought subpens should only make submarines and transports.

This comment has been minimized.

@pchote

pchote Nov 18, 2017

Member

You're right, my bad. I was confusing the behaviour with cheats enabled (primary subpens being able to produce ships) as a general feature. But it isn't.

@pchote

pchote Nov 18, 2017

Member

You're right, my bad. I was confusing the behaviour with cheats enabled (primary subpens being able to produce ships) as a general feature. But it isn't.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 22, 2017

Member

I just noticed that this has been updated (please be aware that github doesn't notify us automatically). This is now back in my to-review queue, but I have more work travelling coming up, so my time is again constrained.

@GraionDilach: I would be interested in seeing your updated opinion on these changes after all the changes from above.

Member

pchote commented Nov 22, 2017

I just noticed that this has been updated (please be aware that github doesn't notify us automatically). This is now back in my to-review queue, but I have more work travelling coming up, so my time is again constrained.

@GraionDilach: I would be interested in seeing your updated opinion on these changes after all the changes from above.

@GSonderling

This comment has been minimized.

Show comment
Hide comment
@GSonderling

GSonderling Nov 23, 2017

Contributor

@pchote It's ok, you do this for free and in your free time. As long as the relevant code doesn't diverge, this PR can wait.

Contributor

GSonderling commented Nov 23, 2017

@pchote It's ok, you do this for free and in your free time. As long as the relevant code doesn't diverge, this PR can wait.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Nov 23, 2017

Contributor

This proposal looks better now than it was the last time I looked at. However, I don't like that empty fallback exits can only be used by explicitly no productiontypes.

TBH, I would define productiontypes for all actors regardless of mod, and then allow empty exits as valid for any productiontype.

Contributor

GraionDilach commented Nov 23, 2017

This proposal looks better now than it was the last time I looked at. However, I don't like that empty fallback exits can only be used by explicitly no productiontypes.

TBH, I would define productiontypes for all actors regardless of mod, and then allow empty exits as valid for any productiontype.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 23, 2017

Member

allow empty exits as valid for any productiontype.

The last set of changes that I suggested and which appear to have been included should have fixed this..?

Member

pchote commented Nov 23, 2017

allow empty exits as valid for any productiontype.

The last set of changes that I suggested and which appear to have been included should have fixed this..?

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Nov 23, 2017

Contributor

Ah yes, sorry, I misread the empty-productiontype case (where empty productiontypes can't use any exit with a defined productiontype) as such. Personally I would allow that limit be lifted in alltech mode.

Also still in favor of official mods using explicit exit types.

Contributor

GraionDilach commented Nov 23, 2017

Ah yes, sorry, I misread the empty-productiontype case (where empty productiontypes can't use any exit with a defined productiontype) as such. Personally I would allow that limit be lifted in alltech mode.

Also still in favor of official mods using explicit exit types.

@pchote

With the immediate firefighting of #14482 out of the way, I finally had some time to look at this again. Sorry for the unreasonably long delay (although any of the @OpenRA/engine-hackers could have reviewed this in the mean time...).

I have pushed a couple of amendments to your branch to address @GraionDilach's requests and a couple of other minor issues.

With these fixes in place, LGTM 👍.

@pchote pchote added the PR: Needs +2 label Dec 9, 2017

@pchote pchote dismissed their stale review via 819c906 Dec 9, 2017

@pchote

pchote approved these changes Dec 9, 2017

(pushed again to set the author information to @GSonderling and rebase against bleed)

@pchote pchote added this to the Next release milestone Dec 9, 2017

@reaperrr reaperrr merged commit 488cec6 into OpenRA:bleed Dec 13, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@reaperrr
Contributor

reaperrr commented Dec 13, 2017

@GSonderling GSonderling deleted the GSonderling:ExitTypes branch Jan 2, 2018

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