[PATCH] Extend Visible Garrisoning Schema

[Stan]

For now it's not possible to restrict the classes that appears on the walls, I would be nice to be able to do so.

That would prevent elephants from being visible on the walls for instance if you mod the fortress see Wowgetoffyourcellphone's comment.

Discussion here ​http://wildfiregames.com/forum/index.php?showtopic=19898&page=5#entry309635

[Stan]

You can now add a tag inside the garison point to restrict a certain type of unit. For instance if you type "Archer" no archer will garison on that point.

[Stan]

Make them allowed classes instead, on the input of WowGetOffYourCellPhone. The unit has to have all the types in order to be allowed to garrison. Fix a bug where the garrison point was unvalidated if a unit had not the good requirements but entered before the first one.

[Stan]

A test case with the mauryan fortress.

[mimo]

Thanks for the patch stan. I've not tested it, just had a look at the code.

• tabs seems to be a bit random, for example one is missing in line 236 while there is an extra tab in 252, 253 and 254, ...
• you should use the function MatchesClassList (from globalscripts/Templates.js) to check if an entity is allowed to garrison, see how it is done in AllowedToGarrison.
• Check the logic of the OnGlobalEntityRenamed function: for a standard garrison order, vgpEntity is undefined, so you should check the matching only in the block lines 241 to 247, and not at line 256. vgpEntity is only defined in some peculiar case where we want to reuse the same visibleGarrisonPoint, in case of promotion for example, and thus should always be allowed. Maybe while you are at it, adding a comment in the Garrison function about that point would be useful.
• the number of visibleGarrisoned units should be counted, and substracted from the BuildingAI ArrowCount if any.
• line 288 is useless, just replace 290 by if (this.AllowedToVisibleGarrisoning)
• line 299 should be removed, it is because you have not treated the case not allowed to garrison in lines 241 to 247.

[elexis]

Backlogging due to lack of progress.

[Stan]

Not sure if it asserts all the above, could someone check ?

[Stan]

Current Version

[Stan]

Little testcase if needed, you can only add an onager on the nearest tower, and all the other are for archers.

[elexis]

Style things:

• IsVisiblyGarrisonned should use the array-function some.
• the false if not is not part is not really needed (should be obvious)
• garrisonned -> garrisoned
• if( -> if (
• Spaces in objects: { "a": b, "c": d }
• //foo -> // Foo

(There were some discussions between sanderd17 and mimo on irc about the logic)

[fatherbushido]

L746 : I don't understand what is sent in the Message. What is entity in this.IsVisiblyGarrisonned(entity) ?

[Stan]

It sends true if the unit is visible false otherwise =) Entity is the entity's id. Same one that are sent in the other.

[fatherbushido]

Replying to stanislas69:

It sends true if the unit is visible false otherwise =) Entity is the entity's id. Same one that are sent in the other.

That's not the same one, here we send the list of killed entities (see the loop on entity above this line). this.entity is the entity of the buiding, killedEntities is the list of the killed entities but entity is nothing

[Stan]

Should fix the above, thanks fatherbushido for the insight.

[Stan]

Same testcase

[elexis]

Style:

• still one if(
• if (visibleGarrisonPoint) and if (this.AllowedToVisibleGarrisoning(visibleGarrisonPoint, entity)) can be merged to a single if-else
• Those PostMessage calls use like 150 characters per line. You could split the object that is given by argument into several lines (one property per line):

  Engine.PostMessage(this.entity, MT_GarrisonedUnitsChanged, {
      "added": [entity],
      "removed": [],
      "visible": this.IsVisiblyGarrisoned(entity)
  }); 
  

[Stan]

Should fix those.

[Stan]

Remove the multiple warning sendings

[Stan]

Fix dumb typo

[Stan]

Fix style and another typo

[Stan]

Should fix most of the issues and be a little more clean.

[Stan]

Use mimo's way of sending messages.

[Stan]

New version this time creating temporary objects as the game would replace the value of the variable by the name.

[mimo]

why is VisibleGarrisonAllowedClasses optional in the schema ? Wouldn't it be safer to have it mandatory, defined by default to "Unit" or "Ranged" (lines 222 to 224 could then be removed). lines 270, 367 and 669 why defining this key ? and not simply "visible[entity] = ..." and why do you need 644 and following ? seems useless

Then concerning the design: when we garrison, we start by filling the visible spots and then do non-visible garrison. But when ungarrisoning a single unit, it is the first in the array with the right template which is ungarrisoned, independantly of it being visible or not. That's not consistent. I think that when a visibly garrisoned unit is killed or ungarrisoned, it should be replaced by any non-visible garrisoned which satisfy the requirements (and a message be thrown such that BuildingAI takes it into account), and when ungarrisoning a single unit, we should give priority to nonvisble ones (visible ones will nonetheless always be ungarrisonable directly when selecting them).

Last point, this was not in your testcase, but the wall_garrisoned aura should be added when commiting such stuf.

[Stan]

Replying to mimo:

why is VisibleGarrisonAllowedClasses optional in the schema ? Wouldn't it be safer to have it mandatory, defined by default to "Unit" or "Ranged" (lines 222 to 224 could then be removed).

Not to break compatibility.

lines 270, 367 and 669 why defining this key ? and not simply "visible[entity] = ..." and why do you need 644 and following ? seems useless

I ran into trouble doing so, (I got {entity:true} instead of {17 : true})

Then concerning the design: when we garrison, we start by filling the visible spots and then do non-visible garrison. But when ungarrisoning a single unit, it is the first in the array with the right template which is ungarrisoned, independantly of it being visible or not. That's not consistent. I think that when a visibly garrisoned unit is killed or ungarrisoned, it should be replaced by any non-visible garrisoned which satisfy the requirements (and a message be thrown such that BuildingAI takes it into account), and when ungarrisoning a single unit, we should give priority to nonvisble ones (visible ones will nonetheless always be ungarrisonable directly when selecting them).

That could be nice, but I'm not sure I'm able to do that :/

Last point, this was not in your testcase, but the wall_garrisoned aura should be added when commiting such stuf.

Will keep that in mind.

[Stan]

Fix some var/let remove useless lines (that didn't work before but meh) and 644 is because you can't use msg.entity as a key, that toggles an error.

[mimo]

What's the problem with breaking compatibility if we believe non-optional is better? you'd have only to update the wall templates. Futhermore, I'm quite sure your current patch break the walls as AllowedToVisibleGarrisoning does not test if visibleGarrisonPoint.allowedClass is defined. Have you tested it ?

Can you be more explicit on the problem you have with 644? in which line does msg.entity gives the problem. I don't understand it.

Better to have a complete patch, and i think we need the proper handling of ungarrisoning. Giving priority to nonvisible in unloadtemplate is something which looks quite simple to do: if (!all), we should not do the push if the entity is visible, but cache it and continue the loop until we find a nonvisible one. And if none found, then we do the push on the cached one.

[Stan]

Replying to mimo:

What's the problem with breaking compatibility if we believe non-optional is better? you'd have only to update the wall templates. Futhermore, I'm quite sure your current patch break the walls as AllowedToVisibleGarrisoning does not test if visibleGarrisonPoint.allowedClass is defined. Have you tested it ?

Indeed it does break, I totally forgot to test that, tested boats and other stuff thou

I just thought I would be more annoying for artist to have to set each point

Can you be more explicit on the problem you have with 644? in which line does msg.entity gives the problem. I don't understand it.

Well it broke before when doing {msg.entity : true} but apparently works with brackets, so I fixed it now.

Better to have a complete patch, and i think we need the proper handling of ungarrisoning. Giving priority to nonvisible in unloadtemplate is something which looks quite simple to do: if (!all), we should not do the push if the entity is visible, but cache it and continue the loop until we find a nonvisible one. And if none found, then we do the push on the cached one.

Okay

[Stan]

Fix the order of disposability of units, this was more complex than I thought because you add to make sure you could still ungarison units in any order.

[Stan]

BTW I'm not sure auto adding units on towers and such is a good idea, since that basically weakens your building. Ie : If you garison 20 archers, i would just kill them one by one, and then capture the now empty building.

[mimo]

Replying to stanislas69:

BTW I'm not sure auto adding units on towers and such is a good idea, since that basically weakens your building. Ie : If you garison 20 archers, i would just kill them one by one, and then capture the now empty building.

That's exactly why on comment 11, i mentionned the design. It is not clear to me what is wanted, and how this is supposed to work. We should either have an automatic way to put units on visibleGarrison (with the drawback you mentionned), or add a button in the garrison panel to switch a unit from garrison to visibleGarrison (which will reduce a lot the impact of such a feature because people will prefer to stay garrisoned with arrows from BuildingAI). But having a clear idea (apart that it gives nice screenshots) of how such a feature is supposed to work in game would help.

Otherwise, for your changes in UnloadTemplate, it looks to me that you've taken a very complicated approach :D What i proposed you in my previous comment was simply add after line 465

let visibleEntity;

add just before line 479

if (!all && this.IsVisiblyGarrisoned(entity))
{
    if (!visibleEntity)
        visibleEntity = entity;
    continue;
}

and then, after line 484, add

if (visibleEntity && entities.length == 0)
    entities.push(visibleEntity);

Finally, concerning the xml, I'm not sure we need the tokens as visiblePoints will redefine their classes, and not use inheritance (see for example how GarrisonArrowClasses is defined in BuildingAI). And as we are inside the VisibleGarrisonPoints, no need to repeat VisibleGarrison in the name (X is not named VisibleGarrisonX). Thus everything will stay in one line <AllowedClasses>Infantry+Ranged</AllowedClasses> instead of your current 3 lines

[Stan]

Replying to mimo:

That's exactly why on comment 11, i mentionned the design. It is not clear to me what is wanted, and how this is supposed to work. We should either have an automatic way to put units on visibleGarrison (with the drawback you mentionned), or add a button in the garrison panel to switch a unit from garrison to visibleGarrison (which will reduce a lot the impact of such a feature because people will prefer to stay garrisoned with arrows from BuildingAI). But having a clear idea (apart that it gives nice screenshots) of how such a feature is supposed to work in game would help.

I could make a testcase for the quinquereme that is supposed to have a catapult turret.

Otherwise, for your changes in UnloadTemplate, it looks to me that you've taken a very complicated approach :D What i proposed you in my previous comment was simply add after line 465

let visibleEntity;

add just before line 479

if (!all && this.IsVisiblyGarrisoned(entity))
{
    if (!visibleEntity)
        visibleEntity = entity;
    continue;
}

and then, after line 484, add

if (visibleEntity && entities.length == 0)
    entities.push(visibleEntity);

I tried that, units fall in blocks and that doesn't work well... Can I keep my way ?

Finally, concerning the xml, I'm not sure we need the tokens as visiblePoints will redefine their classes, and not use inheritance (see for example how GarrisonArrowClasses is defined in BuildingAI). And as we are inside the VisibleGarrisonPoints, no need to repeat VisibleGarrison in the name (X is not named VisibleGarrisonX). Thus everything will stay in one line <AllowedClasses>Infantry+Ranged</AllowedClasses> instead of your current 3 lines

So Like this ?

    "<element name='AllowedClasses'>" +
        "<text/>" +
    "</element>" +

and replacing line 81 by

    offset.AllowedClasses

? instead of

    offset.VisibleGarrisonAllowedClasses

[mimo]

I'm not sure a testcase will help, but maybe starting a forum thread about it ?

Let's take your example of a quinquereme, if you have two catapults garrisoned and only one spot. We must define what behaviour we want: do we want one of them to be automatically visible (current behaviour), and then do we want to be able to switch it nonVisible when hurt (not implemented), and should we be able to make the second one visible (not implemented) or should it be automatically visible (not implemented) as soon as the first one is destroyed ? We need to have a clear design of what we want, and I've no good solution to propose as all seems to have some drawbacks as it depends on the situation: attacking (ships where replacing the catapult would be better) or defensive (fortresses where replacing the defenders may let you vulnerable to capture).

[sanderd17]

Taking this out of the review queue, as it needs some more design decisions.

[elexis]

Giving the player a choice whether to garrison units on buildings like fortresses and ships adds to the game experience, especially if it offers tactical advantages (f.e. shooting with a catapult instead of regular garrisoned arrows). Micro-managing this should be optional and rewarding. Agree that units shouldn't automatically transfer from visible to hidden or vice versa. Maybe we could have an alternative garrison key combo to fill the visible spots only if requested by the player.

[Michael D. Hafer]

Just implement the basic feature for A21, then refine it for A22. Do not be afraid to experiment. The game is in alpha after all. People can try out the basic feature and then come up with refinements for A22.

The simplest refinement that solves a lot of issues/concerns would be to have a button like "Garrison Battlements" that stocks the parapet with fresh (garrisoned) units of the required classes. Put the button at the bottom of the garrison panel. I think all garrison buttons should be moved to the garrison panel as well (see Age of Empires II for inspiration, but this is a different topic).

[elexis]

Backlogging due to lack of progress.

[Stan]

Basic Feature, tested by wowgetoffyourcellphone

[mimo]

Hi stan, in view of previous comments, i think nothing should be automatically done by the garrisonHolder. All garrisoned units should go hidden when garrisoned, and you should add two functions setVisible and setHidden. To use these functions in the gui, we will have to add new buttons but we can already start implementing them with hotkeys for test purposes. For example, when we Crtl-click on a icon template in the garrison panel, one of these units (the one with the greatest health ratio) would become visible if a spot is available for it, and when we Crtl-click on the ungarrison button (in the command panel) of a visible garrisoned unit, it becomes hidden. We could also add another function which would fill all the empty visible spots (although it will require an additional button in the gui). But, #4102 was supposed to add tests for the garrisonHolder components, and i think that one should come first before implementing such big changes in the code.

[Stan]

So i shall wait for #4102 to be commited then...

[Lionkanzen]

Add to Phabricator?

[Imarok]

Move tickets to Simulation as UI & Simulation got some sub components.

[Stan]

In 23630:

Allow specific garrison points to receive only specific units, for instance catapults on ships, or having both visible garrison points for fortresses and garrisonning elephants.

Reviewed by: @Freagarach,
Comments by: @elexis, @Angen

Fixes #3488

[Freagarach]

(User control is not addressed in the above commit.)

[Freagarach]

In 23856:

Move the ability to hold a turret to a separate file.

The logic concerning visible garrison points (i.e. turrets) is moved from GarrisonHolder to a separate file.
This is logical because garrisoned != turreted, so this allows for turrets that cannot be garrisoned (refs. D1958).
Also references #3488.

Differential Revision: D2367
Reviewed by: @wraitii