[PATCH] Limit heroes to one living per player

[historic_bruno]

Heroes are too easily exploited now and because they are so important to our plans, they should be limited this way. Perhaps we could generalize BuildLimits to EntityLimits, there seems a lot of overlap between building limits and the proposed unit/hero limits.

Would fit nicely with the visual feedback of #921. The AI needs to know about the limits, too, qBot gets carried away with heroes :)

[fcxSanya]

I had worked on this some time ago (I think somewhere in February while waiting for trading patch review) and had some partially working patch: it allowed to train only one hero, but I implemented this is as a separate code rather than generalizing BuildLimits and I did not implemented proper gui indication (graying of icons/tooltip notes); at this state I moved back to the trade patch and then had the lack of free time, so finally stuck on this.

I will look at evening at my patch and at least will post it here for reference. I can proceed work on this task by myself, but this probably will be slow, so if someone more active is interested in it, I don't mind if he will work on this ticket.

[fcxSanya]

my old unfinished patch

[fcxSanya]

I attached my old WIP patch for r11045. I will update it to the current revision today-tomorrow.

There is a related discussion in IRC logs from 2012-02-24 (​http://irclogs.wildfiregames.com/2012-02-24-QuakeNet-%230ad-dev.log):

17:04 < fcxSanya> I probably don't announced this explicitly, but I implemented 'one hero at a time' restriction, it is spread across Player, TrainingQueue components and the Commands helper file, but there is not much code indeed; now I work on gui part, i.e. displaying gray icons, when hero can't be trained and probably at the same time will do the same for buildings, because BuildLimits currently don't have such indication

<...>

17:12 < fcxSanya> initially I intended to modify BuildLimits, but then found out, that there is more changes than similarities and that most logic anyway will be in the TrainingQueue and Commands, so I did it there, without other components

Later in those logs Michael and Jonathan discuss that we need such restrictions for other units and this should be customizable via templates and restrictions can be unified for building/units and maybe also techs, i.e. what Ben wrote in the ticket description, so patch definitely needs more work.

[fcxSanya]

the same old functionality, but updated to r11909

[fcxSanya]

There is some unfinished gui changes inside the patch. I will try to finish them in near few days.

[fcxSanya]

greying of hero buttons on the training panel

[fcxSanya]

I still work on this ticket. I rewrote my patch to support limits for any units:

• renamed BuildLimits to EntityLimits (Ben did all the hard work of name selection in the ticket description :) )
• created TrainingRestrictions component by analogy with BuildRestrictions, but it contains only category; it is probably possible to generalize BuildRestrictions (like BuildLimits), but everything except category still will be not relevant to units and other schema elements will need to be made optional; I will look at this more carefully

What is implemented in GUI at this moment - is only icons greying when count of units is greater than limit. Need to adjust batch-training gui functionality too.

Also in quick testing I noticed that when unit is killed it is still counted in EntityLimits component, this is probably something minor, but I did not had time to check this yet (I made the patch at today's morning before going to work).

[fcxSanya]

Replying to fcxSanya:

Also in quick testing I noticed that when unit is killed it is still counted in EntityLimits component, this is probably something minor, but I did not had time to check this yet (I made the patch at today's morning before going to work).

Actually it was bug in my changes in unit training. Fixed in one_hero_2012_07_12_v2.diff​.

Current todo:

• batch-training gui changes
• code cleaning
• testing

[fcxSanya]

Working on batch training/training from multiple buildings.

Currently changed only single-unit-in-multiple-buildings case: for example if you train some unit and can train only 3 more units of this type, but 5 buildings which can train it are selected, train order will be sent only to first 3 of them.

[Maegereg]

Ah crap. I looked at this a couple weeks ago and thought you'd given up on this. I've been working on a solution, which I suppose I'll post here. Glancing through your latest patch, it seems roughly comparable, though I'm not as far along as you are (I have yet to tie into the AIs, for example, or remove the code for BuildLimits). The system I've set up is mildly different than yours, the main change being that limits are stored in individual entity templates, as suggested in the chatlog you posted. Anyhow, sorry to duplicate effort.

[fcxSanya]

Replying to Maegereg:

the main change being that limits are stored in individual entity templates, as suggested in the chatlog you posted.

I believe the point of suggestion in the logs was to use templates rather than hardcode limit only for heroes.

Your patch doesn't include file with EntityLimits class (probably it is untracked so you need to 'svn add' it), so I don't know how that part of logic works.

I don't see any advantage in duplicating the limit number in each template, in this case it looks like each template can have different limit regardless of the category. If you going to change some limit you should search/replace the value in all templates. If you will miss something, then it looks like you can have the situation where you e.g. have one hero and can train one more of one type (since it have limit of 2 in the template) and can't train hero of another type (since it have limit of 1 in the template).

Another thing: I think it is better to send EntityLimits inside simulation state and check them in gui side rather than perform GuiInterfaceCall to AllowedToBuild for each template, since later is more expensive in performance terms: simulation is executed in different context and this function will be called each frame for each template that can be trained inside selected building(s).

Anyhow, sorry to duplicate effort.

If it is not demotivating for you, then not much problem, surely it is more productive to work on different tasks, but more eyes to review code and fresh ideas to consider is good too :)

[Maegereg]

Includes added components

[Maegereg]

Replying to fcxSanya:

I believe the point of suggestion in the logs was to use templates rather than hardcode limit only for heroes.

Actually, I'm pretty sure Mythos wanted the numbers to be in individual templates. He said:

17:25 < Mythos_Ruler> This is in the House template: 17:25 < Mythos_Ruler> <BuildRestrictions> <Category>House</Category> </BuildRestrictions> 17:26 < Mythos_Ruler> This tells me nothing.

leper actually suggested pretty much the exact template I use in that conversation.

Your patch doesn't include file with EntityLimits class (probably it is untracked so you need to 'svn add' it), so I don't know how that part of logic works.

Fixed. You can take a look now.

I don't see any advantage in duplicating the limit number in each template, in this case it looks like each template can have different limit regardless of the category. If you going to change some limit you should search/replace the value in all templates. If you will miss something, then it looks like you can have the situation where you e.g. have one hero and can train one more of one type (since it have limit of 2 in the template) and can't train hero of another type (since it have limit of 1 in the template).

The work required to create a new limit is exactly the same as under your solution. You require editing the player template plus the template of every single limited unit to add the limit class, I require editing the template of every single unit (so 1 less template, actually, but not a significant difference). In either case if you miss something you end up with difficulties. The behavior is slightly different (your patch you end up with one unit unlimited and the rest following the limit, my patch you end up with one unit unlimited by still counting toward the limit for all the other classes).

Also, I believe my system allows for more powerful behavior. You can have a unit count towards a limit without being limited, as you pointed out (this might be desired behavior under some circumstances, and would be impossible to create under your system). Also, it's a lot easier and more intuitive to limit a single unit type under my system- you just add a limit to the template file, whereas for yours, you have to create a class, limit that class, and then give the unit that class.

Another thing: I think it is better to send EntityLimits inside simulation state and check them in gui side rather than perform GuiInterfaceCall to AllowedToBuild for each template, since later is more expensive in performance terms: simulation is executed in different context and this function will be called each frame for each template that can be trained inside selected building(s).

I see your point, but at the same time, it strikes me as better to abstract away the process of deciding whether a unit has exceeded its limits. I think my solution involves a little more logic than yours, and it struck me as much better to simple have the graphics perform a single method call rather than having to worry about how to pull data out of the template and assess whether the limit was exceeded. The abstraction seems especially good since we're almost certainly going to want to use it in more than one place (production queues should probably check every unti they add to make sure it's legal under limits) and it helps limit duplicate code. Also, I did make sure to only call AllowedToBuild if the unit actually had a limit, so that helps a lot with the performance cost. That was my logic, at any rate- I'm not really equipped to tell whether the performance cost is worth it.

[fcxSanya]

The work required to create a new limit is exactly the same as under your solution. You require editing the player template plus the template of every single limited unit to add the limit class, I require editing the template of every single unit (so 1 less template, actually, but not a significant difference).

Your solution is more comfortable in short term (as you say below in the message - if you quickly want to set the limit for the single template). But mine looks more correct from architecture point of view - you define limit with its settings (single number currently) and reference it from specific templates. With my solution you can see/adjust all limits in the single place.

In either case if you miss something you end up with difficulties. The behavior is slightly different (your patch you end up with one unit unlimited and the rest following the limit, my patch you end up with one unit unlimited by still counting toward the limit for all the other classes).

I mean miss when changing the limit (not when limiting more/less templates), e.g. if limit was 1 and you set it to 2 for the same templates. With my solution you can't do anything inconsistent since you change only one number in the player template.

Also, I believe my system allows for more powerful behavior. You can have a unit count towards a limit without being limited, as you pointed out (this might be desired behavior under some circumstances, and would be impossible to create under your system)

This sounds like a hack, if we will need such behaviour, better to implement it explicitly.

Re AllowedToBuild: note that you will need more gui logic to check limits for batch training/multiple building training anyway (or again delegate it into simulation).

[michael]

For what it's worth, I no longer have a strong opinion on this, other than it should not be hard coded somewhere, but easily edited either in the templates or some script. Looks like the disagreement is on the last part--templates or script. All I can say is that cases such as different factions having different limits for the same type of entity should be considered (if done in a script, then a new category can just be created). It's a tough call, so I'd say go with what is more elegant in implementation.

[fcxSanya]

Replying to michael:

Looks like the disagreement is on the last part--templates or script.

No, both solutions are in templates, mine is like current BuildLimits/BuildRestrictions:

templates/special/player.xml:

    <Limits>
      <CivilCentre/>
      <DefenseTower>25</DefenseTower>
      <Fortress>10</Fortress>
      <FemaleCitizen>6</FemaleCitizen>
      <Hero>1</Hero>
    </Limits>

templates/template_unit_hero_cavalry_archer.xml (and other specific templates are the same):

  <TrainingRestrictions>
    <Category>Hero</Category>
  </TrainingRestrictions>

Maegereg's implementation: no common definition in player.xml, but number stated in each file:

templates/template_unit_hero_cavalry_archer.xml (and other specific templates are the same):

	<EntityLimit>
		<ConcurrentLimit>
			<Limit>1</Limit>
			<Classes>Hero</Classes>
		</ConcurrentLimit>
	</EntityLimit>

For heroes we have 12 templates (with different parent templates). Some time ago (about three months ago) Philip mentioned in IRC that mixins can be used for templates:

22:48 <@Philip`> vtsj: I've thought about multiple inheritance for entity templates - maybe it'd be like <Entity parent="foo" mixins="bar baz"/> where bar and baz are not complete templates, they're just fragments that can provide some common behaviour (e.g. to import some civ-specific data, or to import some rank-specific data, cutting across the inheritance hierarchy).

This will allow to remove text duplication, but my solution ("my solution" in sense that "solution which I use", I just copied how BuildLimits/BuildRestrictions are implemented) still looks more robust for me.

All I can say is that cases such as different factions having different limits for the same type of entity should be considered (if done in a script, then a new category can just be created).

Different factions have different templates for each unit type, right? We can just define different limits (limit categories) in those specific templates.

[Maegereg]

Replying to fcxSanya:

Your solution is more comfortable in short term (as you say below in the message - if you quickly want to set the limit for the single template). But mine looks more correct from architecture point of view - you define limit with its settings (single number currently) and reference it from specific templates. With my solution you can see/adjust all limits in the single place.

You have a point in that use case.

I mean miss when changing the limit (not when limiting more/less templates), e.g. if limit was 1 and you set it to 2 for the same templates. With my solution you can't do anything inconsistent since you change only one number in the player template.

My point was that in creating a limit, both methods have the same risk of missing a template (mine for the actual limit, yours for the class). However, you're right that editing is much less likely to produce error under your model.

This will allow to remove text duplication, but my solution ("my solution" in sense that "solution which I use", I just copied how BuildLimits/BuildRestrictions are implemented) still looks more robust for me.

It's really a case of whether you consider the limits as data more closely relating to the unit, or data more closely relating to the player. Given mixins (do you know if they're functional yet- the quote implied that they didn't exist?), it sounds like it could be made to work about equally efficiently either way in terms of editing difficulty. I'm still more inclined to think of limits as unit information than player information, but this honestly might be something that deserves input from the people who actually edit those files the most. They're the ones who're best equipped to tell which solution would be easier to use and understand.

This sounds like a hack, if we will need such behaviour, better to implement it explicitly.

Actually, I've realized that the system I've come up with doesn't quite meet the functionality I was looking for. What would be more useful would be to allow both individual limits on the number of a particular unit that can be built (best defined in the unit's own template) and class limits. Under that system, you could define something like "You are allowed 5 siege units and one of them can be a siege tower." That's the functionality I was originally aiming for, but my current system allows only a close approximation of that.

Re AllowedToBuild: note that you will need more gui logic to check limits for batch training/multiple building training anyway (or again delegate it into simulation).

The logic isn't all that different, as far as checking whether you're allowed to build something. Allowing/disallowing batches and distributing over multiple buildings is gui logic, and belongs in the gui code, but determining whether you can build a unit (or how many of them you can build) is going to need to be referenced from multiple places, and design-wise belongs in the limit component of the simulation. On a more concrete note, the AllowedToBuild function can be trivially modified to return how many of a unit you're allowed to build, which quite easily addresses the other functions you were suggesting- and the code can be modified so it doesn't have to fetch a reference to the limits component quite as often, which should help with performance somewhat. The actual implementation details aren't that important here- it can be made to work without significant repeated code and without spreading limits logic over too many code areas, which I think would be a good outcome.

[fcxSanya]

Replying to Maegereg:

Given mixins (do you know if they're functional yet- the quote implied that they didn't exist?) <...>

They are not implemented and there is even no ticket for them. I mentioned them, because theoretically we can have them, and so text duplication is not the biggest problem.

I'm still more inclined to think of limits as unit information than player information, but this honestly might be something that deserves input from the people who actually edit those files the most. They're the ones who're best equipped to tell which solution would be easier to use and understand.

Man who perform most of templates changes is Michael aka Mythos_Ruler (see comment #13).

On a more concrete note, the AllowedToBuild function can be trivially modified to return how many of a unit you're allowed to build, which quite easily addresses the other functions you were suggesting <...>

Why not just pass those numbers (counts/limits) to gui with all other simulation state then? Anyway this is relative minor difference.

Since we both still prefer our own approaches, we need more opinions (third approach?) :)

[Maegereg]

Replying to fcxSanya:

They are not implemented and there is even no ticket for them. I mentioned them, because theoretically we can have them, and so text duplication is not the biggest problem.

Ah. Yes, that does eliminate some of the problem then.

Man who perform most of templates changes is Michael aka Mythos_Ruler (see comment #13).

Ah, I see. Well, if he doesn't have a strong opinion, then perhaps either would work. Though I tend to think we should try to get him to nail down which sort of a solution he would prefer.

Why not just pass those numbers (counts/limits) to gui with all other simulation state then? Anyway this is relative minor difference.

Since we both still prefer our own approaches, we need more opinions (third approach?) :)

I'm not terribly convinced on the template issue- both approaches have merit, I think, and my opinion doesn't count for very much as to which we should pick.

The passing numbers/calling functions issue, however, is something that I still am convinced I'm right about. I'll take another shot at convincing you on that count.

Basically, we're going to want to check whether you can legally build a given unit because of limits in a number of places. To name a couple- the gui, the production queue module (queues should probably not add anything that exceeds production limits, and we probably shouldn't rely on the gui to prevent the limits from being exceeded) and the builder module at a minimum (for the same reason as production queues- to prevent a builder working on something that exceeds a limit). The AIs also need to know, but we'll probably want to expose more information to them to aid with planning, so I'll ignore them for now.

If we simply pass in the current numbers at the limits to all of these places, we'll have duplicate code for evaluating whether units can be build under limits in three places. That in itself is annoying and sloppy, but it also raises two other problems: if we want to change how limits work (say, the way classes interact with each other or something like that, or if we find a bug) there are three different places we'd have to change code. That severely impacts maintainability. Second, anyone who's trying to understand the GUI code, or the production queue code, or the builder code will have to understand about limits. Simply calling cmpEntityLimit.AllowedToBuild (for instance) is pretty clear- it's obvious what it's checking, even if you know nothing about the code, and you don't have to bother yourself with what exactly is being checked. That will make the code easier to understand, and therefore easier to edit.

[historic_bruno]

I tried applying fcxSanya's most recent patch but it doesn't apply cleanly anymore. I tried resolving the conflicts and the unit limits appeared to work, but it interfered with recent GUI changes and broke some other things :(

[fcxSanya]

Replying to historic_bruno:

I tried applying fcxSanya's most recent patch but it doesn't apply cleanly anymore. I tried resolving the conflicts and the unit limits appeared to work, but it interfered with recent GUI changes and broke some other things :(

Oh, I will try to upload a new version for current SVN revision today. I'm still slowly working on this and I'm updating working copy time to time, so this should not be a problem.

[fcxSanya]

Current WIP version

[fcxSanya]

Attached the current version, the patch should apply on current revision (r12726). It is somewhere in the middle of multiple-buildings-batch-training-tooltips changes, but should work without errors. There is some strange lines in diff like:

-		var affordableMask = getGUIObjectByName("unit"+guiName+"Unaffordable["+i+"]");
-		var affordableMask1 = getGUIObjectByName("unit"+guiName+"Unaffordable["+(i+rowLength)+"]");
+		var affordableMask = getGUIObjectByName("unit"+guiName+"Unaffordable["+i+"]");
+		var affordableMask1 = getGUIObjectByName("unit"+guiName+"Unaffordable["+(i+rowLength)+"]");

As I understand this is merge side-effects. Those lines look identically if I move them into different files and do diff. Replying to historic_bruno:

<...> but it interfered with recent GUI changes and broke some other things :(

I expect that there is some changes needed in my patch to work properly with changes from r12404. What else?

[historic_bruno]

I applied that patch but now I get errors when selecting a building that trains heroes:

ERROR: JavaScript error: gui/session/input.js line 1614 ReferenceError: batchTrainingEntityAllowedCount is not defined getTrainingBatchStatus([object Object],67,"units/athen_hero_themistocles",[object Array])@gui/session/input.js:1614 setupUnitPanel("Training",[object Object],[object Object],[object Object],[object Array],(function (trainEntType) {addTrainingToQueue(selection, trainEntType, playerState);}))@gui/session/unit_commands.js:377 updateUnitCommands([object Object],[object GUIObject],[object GUIObject],[object Array])@gui/session/unit_commands.js:961 updateSelectionDetails()@gui/session/selection_details.js:299 onSimulationUpdate()@gui/session/session.js:375 onTick()@gui/session/session.js:259 __eventhandler0 (tick)([object Object])@sn tick:0

[fcxSanya]

Replying to historic_bruno:

I applied that patch but now I get errors when selecting a building that trains heroes:

There should be canBeTrainedCount instead of batchTrainingEntityAllowedCount on that line, copy-paste issue, probably. Fixed in one_hero_2012_10_04.diff​ (and also removed 'no_heroes' tech).

[fcxSanya]

New version: I think I finished the batch training tooltip changes. I moved them into a separate function and added some comments.

Still in todo list:

• Check/fix the needed resources functionality (from r12404);
• Code clean-up.

[fcxSanya]

Replying to fcxSanya:

• Check/fix the needed resources functionality (from r12404);

I just moved one block of code into another place and now both features look working together without issues.

[fcxSanya]

Attached the latest version with minor code clean-up. I still did not solve strange exclude/include of the same code (see comment #22) and still want to check/test it more myself, but reviewing/testing by others is appreciated too.

[fcxSanya]

Replying to fcxSanya:

I still did not solve strange exclude/include of the same code (see comment #22) <...>

Ah, it looks like there are inconsistent newline characters in that file in SVN. If I just open/save it with gedit (which fixes inconsistency), svn adds those lines into diff.

[fcxSanya]

In 12779:

Fix some inconsistent newline characters from r12404 in unit_commands.js via unix2dos. Refs #1432

[fcxSanya]

Trac displays no changes for r12779 ('svn diff -c 12779' displays them though), but it actually solved the issue, so I attached the new patch.

[fcxSanya]

A couple of notes:

• Current count/limit aren't displayed in GUI, the only indication is: when limit is reached the icon on the training panel is greyed out. I'm planning to display numbers also, but this can be added later after committing current version of the patch.
• There is the females limit in the patch - just for testing, I'm going to remove it before committing.

[fcxSanya]

• Current count/limit aren't displayed in GUI, the only indication is: when limit is reached the icon on the training panel is greyed out. I'm planning to display numbers also, but this can be added later after committing current version of the patch.

In one_hero_2012_10_27.diff​ I added "Current count: <number>, limit: <number>." string into the tooltip. If there is no limit, the string isn't shown at all, if the limit is reached it is red, otherwise it is white.

[Kieran P]

Your patch is quite big. Please don't commit until someone else has peer reviewed and is happy that there are no additional changes to make.

[fcxSanya]

Replying to k776:

Your patch is quite big. Please don't commit until someone else has peer reviewed and is happy that there are no additional changes to make.

Yes, I'm planning to wait for some reviews before committing.

[historic_bruno]

Looks good! I tested and didn't really notice any bugs. I only found a few minor issues with how the code looks:

• In unit_commands.js, formatBatchTrainingString() is a really messy function that's not easy to understand (especially lines 197-202). I would say even if it adds more lines of code, it should be written for clarity.
• The if blocks in EntityLimits.js are slightly unconventional in that they use braces {} when there's only one statement. It's an old issue from BuildLimits but we can fix it now :) Commands.js line 192 adds an if statement like that, and there may be others.

I think you should commit it whenever you get the chance.

[fcxSanya]

Replying to historic_bruno:

Thanks for the review :) I will fix the code formatting/style and commit today/tomorrow.

[fcxSanya]

In 12832:

Training limits. Limit heroes to one living per player. Allow heroes to be trained again. Closes #1432

[fcxSanya]

Replying to fcxSanya:

I will fix the code formatting/style and commit today/tomorrow.

In committed version I rewrote part of formatBatchTrainingString in hopefully better readable way, removed unconventional curly brackets and split some very long lines.

Also I discovered that starting from one_hero_2012_10_02_v4.diff​ I broke batch training for non-restricted by limits units, fixed it too.

[historic_bruno]

See http://www.wildfiregames.com/forum/index.php?showtopic=16726.

[fcxSanya]

In 12835:

Fix setupUnitPanel usage broken in r12832. Refs #1432

[fcxSanya]

In 12897:

Simplify formatBatchTrainingString function a bit: remove always true condition/unreachable code. Refs #1432

[elexis]

In 19329:

Rename the Female identity class to FemaleCitizen, because it is intended for female support units only.

Thus prevent template editors from adding that tag to female military units and
allow modders to add a Female class for wider purposes.
Remove the FemaleCitizen training restriction category that was added for debug purposes but forgotton to be removed in rP12832.

Differential Revision: ​https://code.wildfiregames.com/D244
Reviewed By: fatherbushido
Refs: #1432 #4490

[elexis]

In 23192:

Fix warning about attempted empty batch train occurring if g_BatchTrainingEntityAllowedCount % batchedSize == 0, refs #1432, #4059, rP12832, rP18421, rP19826, fixes #5602, reported by nani, reproduced by faction02.

[Freagarach]

In 24496:

[Gameplay A24] - Permadeath for heroes.

Part of: ​https://wildfiregames.com/forum/index.php?/topic/27214-borg-expansion-pack-mod-implementation-in-0ad-alpha-24-release/.
Refs. #1432

Differential revision: D3265
Reviewed by: @borg-, @Nescio