HomeWildfire Games

Units should only be able to build structures from their owner's civ, and not…
AuditedrP20521

Description

Units should only be able to build structures from their owner's civ, and not from their identity civ

Reviewed By: elexis

Trac Tickets: #4870

Differential Revision: https://code.wildfiregames.com/D1065

Event Timeline

bb raised a concern with this commit.Nov 25 2017, 7:55 PM
bb added a subscriber: bb.

Test fails due to a missing GetCiv mock

This commit now has outstanding concerns.Nov 25 2017, 7:55 PM
elexis added a subscriber: elexis.EditedNov 25 2017, 8:17 PM

I guess reading the Vulcan output helps. I started writing a patch, but I can go on with my other atlas bug inside the atlas bug revealed by the removed rmgen bug if you happen to write a builder fix as well

bb added a comment.Nov 25 2017, 8:21 PM

Didn't start writing a patch yet, could do though or continue reviewing some more patches at random

In rP20521#25227, @bb wrote:

Didn't start writing a patch yet, could do though or continue reviewing some more patches at random

Fix finished quicker than expected

bb added a comment.Nov 25 2017, 10:14 PM

Seems to be more like the problem that Vulcan says "successful build", while the tests fail

This now prevents C&C style capturing (which allows one to build everything from the other faction). It also somewhat breaks that one cheat.

Seems like a workaround for an AI issue.

(See differential and or ticket)

(See differential and or ticket)

Those just state "Yeah, that shouldn't be done and now we'll enshrine that after not checking any previous discussion after less than a day of discussion.".

mimo added a comment.Nov 26 2017, 12:05 AM

This now prevents C&C style capturing (which allows one to build everything from the other faction). It also somewhat breaks that one cheat.
Seems like a workaround for an AI issue.

No, what was done previously just had no sense. If we want to only allow to build structures from the Identity civ, then the unit template should just not have {civ} but the civ we want: It would be useless to have that {civ} replacement here,. The behaviour of the patch (replaced by the civ player, as is done in the ProductionQueue for units) is what has always been expected but was buggy.
This has nothing to do with AI.

No, what was done previously just had no sense. If we want to only allow to build structures from the Identity civ, then the unit template should just not have {civ} but the civ we want: It would be useless to have that {civ} replacement here,. The behaviour of the patch (replaced by the civ player, as is done in the ProductionQueue for units) is what has always been expected but was buggy.

Ah, I guess I was somewhat confused by the comment that wasn't fixed in rP16506 that still talks about the entity. Same for the schema here and in the production queue.

Also having to specify the civ explicitly for C&C style capturing seems strange, given that having {civ} is because we want to make that easier. Though I'm not sure where a flag to switch from one behaviour to the other would even fit.

This has nothing to do with AI.

As is apparently broke if it doesn't get what it expects, that seems strange.

Also having to specify the civ explicitly for C&C style capturing seems strange, given that having {civ} is because we want to make that easier. Though I'm not sure where a flag to switch from one behaviour to the other would even fit.

I do believe {civ} was added to make the identity civ replacement, but in general the way we've handled capturing meant that we needed to change that (which I've always found kind of annoying…).

Maybe we should just replace it with {id_civ} and {owner_civ} and be done with it, to be honest.

mimo added a comment.Nov 26 2017, 10:20 AM

Gameplay wise, with the change we now have both possibilities: pers_house will allow the pers units to always build their own house, and {civ}_house to build the player one when captured.

But i agree that there is a (minor) drawback when using templates inheritances. As wraitii, i would propose two replacements as the way to go, although a bit differently:
{civ or whatever the name} is replaced by the owner civ, done dynamically in the simu as now
[civ or whatever the name] is replaced by the identity civ, ideally already when building the template as it is something static and present only to ease inheritance and not related to gameplay.

test failure fixed in rP20527

In rP20521#25245, @mimo wrote:

{civ or whatever the name} is replaced by the owner civ, done dynamically in the simu as now
[civ or whatever the name] is replaced by the identity civ, ideally already when building the template as it is something static and present only to ease inheritance and not related to gameplay.

I'd rather go with {owner_civ} and [ID_CIV] then, as that somewhat clarifies what we're looking at and vaguely ressembles a C++ #define in all-caps. In fact I'd be fine with #CIV but I feel like that'd be less understood.
Agreed that the identity civ swapping should ideally be done in the template building step.

bb accepted this commit.Nov 27 2017, 12:11 PM

test failure gone, so my concern has vanished

All concerns with this commit have now been addressed.Nov 27 2017, 12:11 PM