#3488 closed defect (fixed)
[PATCH] Extend Visible Garrisoning Schema
Reported by: | Stan | Owned by: | Stan |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 24 |
Component: | Simulation | Keywords: | |
Cc: | Patch: | Phab:D2308 |
Description (last modified by )
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
Attachments (18)
Change History (50)
by , 9 years ago
Attachment: | 3488.patch added |
---|
comment:1 by , 9 years ago
Keywords: | patch review added |
---|---|
Milestone: | Backlog → Alpha 20 |
Owner: | set to |
Status: | new → assigned |
Summary: | Extend Visible Garrisoning Schema → [PATCH] Extend Visible Garrisoning Schema |
by , 9 years ago
Attachment: | 3488.2.patch added |
---|
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.
comment:2 by , 8 years ago
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.
comment:3 by , 8 years ago
Keywords: | review removed |
---|
by , 8 years ago
Attachment: | 3488.3.diff added |
---|
Not sure if it asserts all the above, could someone check ?
comment:5 by , 8 years ago
Keywords: | review added |
---|---|
Milestone: | Backlog → Alpha 21 |
by , 8 years ago
Attachment: | 3488.4 - Testcase.diff added |
---|
Little testcase if needed, you can only add an onager on the nearest tower, and all the other are for archers.
comment:6 by , 8 years ago
Style things:
IsVisiblyGarrisonned
should use the array-functionsome
.- 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)
comment:7 by , 8 years ago
L746 : I don't understand what is sent in the Message. What is entity in this.IsVisiblyGarrisonned(entity) ?
follow-up: 9 comment:8 by , 8 years ago
It sends true if the unit is visible false otherwise =) Entity is the entity's id. Same one that are sent in the other.
comment:9 by , 8 years ago
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
by , 8 years ago
Attachment: | 3488.5.diff added |
---|
Should fix the above, thanks fatherbushido for the insight.
comment:10 by , 8 years ago
Style:
- still one
if(
if (visibleGarrisonPoint)
andif (this.AllowedToVisibleGarrisoning(visibleGarrisonPoint, entity))
can be merged to a singleif-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) });
by , 8 years ago
Attachment: | 3488.10.diff added |
---|
Should fix most of the issues and be a little more clean.
by , 8 years ago
Attachment: | 3488.13.diff added |
---|
New version this time creating temporary objects as the game would replace the value of the variable by the name.
follow-up: 12 comment:11 by , 8 years ago
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.
comment:12 by , 8 years ago
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.
by , 8 years ago
Attachment: | 3488.14.diff added |
---|
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.
follow-up: 14 comment:13 by , 8 years ago
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.
comment:14 by , 8 years ago
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
by , 8 years ago
Attachment: | 3488.15.diff added |
---|
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.
follow-up: 16 comment:15 by , 8 years ago
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.
follow-up: 17 comment:16 by , 8 years ago
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
comment:17 by , 8 years ago
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
comment:18 by , 8 years ago
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).
comment:19 by , 8 years ago
Keywords: | review removed |
---|
Taking this out of the review queue, as it needs some more design decisions.
comment:20 by , 8 years ago
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.
comment:21 by , 8 years ago
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).
comment:23 by , 7 years ago
Keywords: | rfc added |
---|---|
Milestone: | Backlog → Work In Progress |
comment:24 by , 7 years ago
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.
comment:25 by , 7 years ago
So i shall wait for #4002 to be commited then...
comment:27 by , 5 years ago
Component: | UI & Simulation → Simulation |
---|
Move tickets to Simulation
as UI & Simulation
got some sub components.
comment:28 by , 5 years ago
Keywords: | rfc patch removed |
---|---|
Patch: | → Phab:D2308 |
comment:29 by , 4 years ago
Milestone: | Work In Progress → Alpha 24 |
---|
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.