Skip to content
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

Show unrecallable/unrecruitable units greyed/shaded (GNA #18932) #1282

Open
wesnoth-bugs opened this issue May 8, 2017 · 18 comments

Comments

@wesnoth-bugs
Copy link

@wesnoth-bugs wesnoth-bugs commented May 8, 2017

Original submission by fendrin on 2011-11-05

Currently when recruiting, only the too expensive unavailable units are greyed out.

We would like to have the units that are unavailable because of different reasons marked graphically in the gui as well.

(Reproduced on All)
Release: 1.11.0+
Priority: 1 - Later
Severity: 2 - Minor

@wesnoth-bugs

This comment has been minimized.

Copy link
Author

@wesnoth-bugs wesnoth-bugs commented May 8, 2017

Modified on 2012-01-29

shadowmaster changed status: Postponed -> None

@wesnoth-bugs

This comment has been minimized.

Copy link
Author

@wesnoth-bugs wesnoth-bugs commented May 8, 2017

Modified on 2013-12-10

fendrin changed status: None -> Postponed

@wesnoth-bugs

This comment has been minimized.

Copy link
Author

@wesnoth-bugs wesnoth-bugs commented May 8, 2017

Modified on 2015-10-22

fendrin changed status: Postponed -> None

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 5, 2019

For what reason would a unit be unavailable for recall/recruitment besides being short on gold? Story reasons? (In that case, I think the units are simply removed from the recruit/recall lists.)

@gfgtdf

This comment has been minimized.

Copy link
Contributor

@gfgtdf gfgtdf commented Sep 5, 2019

For what reason would a unit be unavailable for recall/recruitment besides being short on gold?

Maybe this is about [filter_recall] or no leader standing on a keep

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 5, 2019

Sounds like it's MP context then...

@gfgtdf

This comment has been minimized.

Copy link
Contributor

@gfgtdf gfgtdf commented Sep 5, 2019

Sounds like it's MP context then...

how so? [filter_recall] is be used in mp and sp.

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 5, 2019

Hmm, I was thinking that it's not possible to recall units without a leader on the keep in SP. Wasn't really considering WML. Regardless, you demonstrated there are times where this visual feedback might be useful (though I don't encounter them normally), so that's enough to keep this request around.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

@gfgtdf gfgtdf commented Sep 5, 2019

Hmm, I was thinking that it's not possible to recall units without a leader on the keep in SP. Wasn't really considering WML

i think the mainline showcase here is really LoW sp mode where you have multiple leaders but each one can only recall certain units ([filter_recall]), so depending on which leader is currently on a keep you can recall other types of units.

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 5, 2019

Thanks for the reminder. After rescuing Cleodil and having different recruit lists, I notice that just selecting the recruit menu from anywhere else on the map you have the option of the combined recruit list of all leaders. Selecting a specific hex will give you the recruit list for that specific leader. So with the former case, if (for example) Cleodil's keep is already full and the player attempts to recruit another Cleodil-specific unit, then you get an notification advising there is no more room. In this case I suppose it would be nice to have Cleodil's units marked in some way to say they are unavailable.

I also thought you're not able to recruit/recall when leader is not on the keep - but it still allows you to attempt but then gives you a notice that your leader must be on a keep.

@jostephd jostephd changed the title Show unrcallable/unrecruitable units greyed/shaded (GNA #18932) Show unrecallable/unrecruitable units greyed/shaded (GNA #18932) Sep 14, 2019
jostephd added a commit to jostephd/wesnoth that referenced this issue Sep 18, 2019
Also add another overload to font::span_color.

Part of issue wesnoth#1282.
jostephd added a commit to jostephd/wesnoth that referenced this issue Sep 18, 2019
@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 20, 2019

There are several cases here. Let's go through them and the current and requested behaviors:

  • Recall too expensive

    Current behavior: listed normally

    New behavior: listed in gray. #4362 (comment)

  • Recruit too expensive

    Current behavior: listed in red

    New behavior: listed in gray. #4362 (comment)

  • Recall - no leader on keep, or no free castle hexes

    Current behavior: list all units

    New behavior: unchanged

  • Recruit - no leader on keep, or no free castle hexes

    Current behavior: list all units

    New behavior: unchanged

  • Recall - [filter_recall] doesn't match. (Selected hex either has a leader, or is in a castle)

    Current behavior: not listed

    Question: Should units that don't match the leader's filter_recall be listed in gray?

  • Recruit - extra_recruit doesn't match. (Only possible when there are multiple canrecruit=yes units. Selected hex either has a leader, or is in a castle that not all leaders are in) - #4443 is part of this

    Current behavior: show only the selected leader's units (grayed out if they're too expensive)

    Question: Should units that can only be recruited by other leaders be listed in gray?

Agreed? Are there any other cases?

test case
diff --git a/data/campaigns/An_Orcish_Incursion/maps/01_Defend_the_Forest.map b/data/campaigns/An_Orcish_Incursion/maps/01_Defend_the_Forest.map
index fdcd60945c7..5173674243b 100644
--- a/data/campaigns/An_Orcish_Incursion/maps/01_Defend_the_Forest.map
+++ b/data/campaigns/An_Orcish_Incursion/maps/01_Defend_the_Forest.map
@@ -14,9 +14,9 @@ Gs^Fds, Gg^Fet, Gs^Fds, Gg, Gg, Gg^Efm, Gg^Efm, Gg, Gg^Efm, Gs^Fds, Gs^Fds, Gg^F
 Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gg, Gs^Fds, Gs^Fds, Gg, Gs^Fds, Gg^Efm, Gg, Gs^Fds, Gs^Fds, Gg^Ve, Gs^Fds, Gs^Fds, Gs^Fds, Gg, Gs^Fds, Gg^Ve, Gs^Fds, Gs^Fds
 Gs^Fds, Gs^Fds, Gg^Ve, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gg^Ve, Gg, Gg^Efm, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gg^Efm, Gg, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds
 Hh, Hh, Gs^Fds, Gg^Fet, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gg, Gg, Gg, Gs^Fds, Gs^Fds, Gg^Fet, Gs^Fds, Gg, Gs^Fds, Gs^Fds, Gg^Fet, Gs^Fds, Gs^Fds
-Hh, Hh, Hh, Gs^Fds, Hh, Gs^Fds, Gs^Fds, Gs^Fds, Gg, Gg^Efm, Gs^Fds, Gg^Efm, Gg, Gs^Fds, Gg, Gg, Cv, Gg, Gg^Efm, Gs^Fds, Gs^Fds, Gs^Fds
-Mm, Mm, Mm, Hh, Hh, Gg^Fet, Gg^Efm, Gs^Fds, Gg^Efm, Gg, Gs^Fds, Gs^Fds, Gs^Fds, Gg^Efm, Gg, Cv, 1 Kv, Cv, Gg, Gg, Gs^Fds, Gs^Fds
-Hh, Hh, Hh, Hh, Hh, Hh, Gg^Efm, Gg, Gg^Ve, Gs^Fds, Gs^Fds, Gs^Fds, Gg^Ve, Gs^Fds, Gg, Cv, Cv, Cv, Gg, Gg^Efm, Gg^Fet, Gs^Fds
+Hh, Hh, Hh, Gs^Fds, Hh, Gs^Fds, Gs^Fds, Gs^Fds, Gg, Gg^Efm, Gs^Fds, Gg^Efm, Gg, Gs^Fds, Gg, Cv, Cv, Cv, Gg^Efm, Gs^Fds, Gs^Fds, Gs^Fds
+Mm, Mm, Mm, Hh, Hh, Gg^Fet, Gg^Efm, Gs^Fds, Gg^Efm, Gg, Gs^Fds, Gs^Fds, Gs^Fds, Gg^Efm, Gg, Cv, 1 Kv, Kv, Cv, Gg, Gs^Fds, Gs^Fds
+Hh, Hh, Hh, Hh, Hh, Hh, Gg^Efm, Gg, Gg^Ve, Gs^Fds, Gs^Fds, Gs^Fds, Gg^Ve, Gs^Fds, Gg, Cv, Cv, Cv, Cv, Gg^Efm, Gg^Fet, Gs^Fds
 Hh, Hh, Gg, Hh, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gg^Fet, Gs^Fds, Gs^Fds, Gg, Gg, Gg, Gg, Gg, Gs^Fds, Gs^Fds, Gs^Fds
 Gg, Gg, Gs^Fds, Gg, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds
 Gg, Gg, Gs^Fds, Gg, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds, Gs^Fds
diff --git a/data/campaigns/An_Orcish_Incursion/scenarios/01_Defend_the_Forest.cfg b/data/campaigns/An_Orcish_Incursion/scenarios/01_Defend_the_Forest.cfg
index 53ea5f5be8e..922df2478cf 100644
--- a/data/campaigns/An_Orcish_Incursion/scenarios/01_Defend_the_Forest.cfg
+++ b/data/campaigns/An_Orcish_Incursion/scenarios/01_Defend_the_Forest.cfg
@@ -50,7 +50,7 @@
     [side]
         side=1
         controller=human
-        {GOLD 200 150 100}
+        gold=20
         income=0
         team_name=Elves
         user_team_name= _ "Elves"
@@ -58,9 +58,19 @@
 
         # wmllint: recognize Erlornas
         {CHARACTER_STATS_ERLORNAS}
+        extra_recruit=Spearman
+
+        [filter_recall]
+            level=1
+        [/filter_recall]
 
         facing=nw
 
+        {UNIT 1 Mage recall recall ()}
+        {UNIT 1 Mage recall recall (recall_cost=25)}
+        {UNIT 1 Peasant recall recall ()}
+        {UNIT 1 (Inferno Drake) 17 17 (canrecruit,extra_recruit=yes,Drake Clasher)}
+
         [unit]
             side=1
             type=Elvish Rider
diff --git a/data/campaigns/An_Orcish_Incursion/utils/characters.cfg b/data/campaigns/An_Orcish_Incursion/utils/characters.cfg
index fcf5d0d2385..595ffcb0d21 100644
--- a/data/campaigns/An_Orcish_Incursion/utils/characters.cfg
+++ b/data/campaigns/An_Orcish_Incursion/utils/characters.cfg
@@ -6,7 +6,7 @@
     name= _ "Erlornas"
     profile=portraits/erlornas.png
     canrecruit=yes
-    extra_recruit="Elvish Archer,Elvish Fighter,Elvish Scout,Elvish Shaman"
+    extra_recruit="Elvish Archer"
     unrenamable=yes
 #enddef
 
@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 21, 2019

I know there's already discussion in the PR, but my thoughts:

  • Too expensive: I'm inclined to keep the current red listing behaviour but for both recruit and recall because it shows clearly that insufficient gold is the cause.
  • No leader on keep or no free castle hexes: I would have this situation greyed out for both recruit and recall.
  • I'm not sure what situations the last two describe. If it's related to the situation in Legend of Wesmere where we have multiple leaders and each leader can only recruit certain types of units, then I don't mind keeping the existing behaviour where ineligible units are simply not listed. But changing them to listed as grey is also acceptable.

I can't think of other situations at the moment.

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

@GregoryLundberg GregoryLundberg commented Sep 21, 2019

Units which do not match the filter should be hidden from view.

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

@GregoryLundberg GregoryLundberg commented Sep 21, 2019

I guess I should expand upon that. The list should be only those units which one could recruit with the selected leader, assuming the leader is in (or moves to) a keep, a keep hex is (or will be) available, and the player has (or could eventually have) sufficient funds. Greying to indicate the unit is presently unavailble sounds like a good idea. I'd consider a tooltip (hover text) which gives the specific reason.

The recall list can contain units for several leaders. If no leader is selected, the list should display empty (perhaps with a hint to select a leader). The list should be only those units available to the current leader. Note there may be units on the list which are presently not available to ANY leader; perhaps the leader has died, will appear later, or perhaps the units are being held for a later scenario.

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 21, 2019

Too expensive: I'm inclined to keep the current red listing behaviour but for both recruit and recall because it shows clearly that insufficient gold is the cause.

That is the current behavior of the recruit dialog but not of the recall dialog. The unit list in the recall dialog is not affected by how much gold the team has, only by whether a unit has a custom recall cost that is different from the team's default recall cost.

For the recruit dialog, I had #4362 change red to gray because red normally represents an error, or something that requires attention or intervention. Gray for "disabled" seemed more suitable. I think the recruit dialog will be more useful when units that can't be recruited this turn are listed in a muted fashion than when they are listed in an eye-catching color.

No leader on keep or no free castle hexes: I would have this situation greyed out for both recruit and recall.

🤔 I'm not sure. That would mean all units would be listed grayed out. That doesn't sound quite right, somehow. Not sure how to explain this - maybe it's that colors are a big part of recognizing unit sprites, and taking that away for all units in the list would make the list harder to review?

Maybe we should just disable the Recruit button (make it not clickable) and add a label, below the units list, that says "You can't recruit right now because you don't have a leader on a keep"? (and the same thing in the recall dialog) We could even set that text in color (say, yellow) to draw attention to it.

I do agree, however, that it would be good to allow the player to review the recruit/recall list even when their leader isn't on a keep or they have no free castle hexes.

I'm not sure what situations the last two describe. If it's related to the situation in Legend of Wesmere where we have multiple leaders and each leader can only recruit certain types of units, then I don't mind keeping the existing behaviour where ineligible units are simply not listed. But changing them to listed as grey is also acceptable.

I haven't played that far into LoW, but you can get this in AOI too. Start S5 and open the recruit dialog. The dialog lists "Mage" but if you try to recruit that, you're told you can only do that if Linaere is on a keep.

The way it works is, each team has a list of unit types it can recruit, and each leader has a list of unit types it can recruit over and above the team's list. In AOI, the team's list is empty, and Erlornas and Linaera both have extra_recruit lists, so the net effect is that Erlornas can recruit only elves and Linaera only mages. For recalls, it's similar, any canrecruit=yes unit can have a [filter_recall] tag and can only recall units that match that SUF.

I think I'm leaning toward listing other leaders' units in grey... 🤔

The list should be only those units which one could recruit with the selected leader, assuming the leader is in (or moves to) a keep, a keep hex is (or will be) available, and the player has (or could eventually have) sufficient funds. Greying to indicate the unit is presently unavailble sounds like a good idea.

The current behavior of the recruit dialog is as follows. The units listed in the recruit dialog depend on what hex is selected. If you select a hex with a leader on a keep, only units recruitable by that leader are listed. If you select a castle hex, only units recruitable by leaders in that castle are listed. If you select a random grassland hex, units recruitable by all leaders are listed, even if currently only one leader is on a keep.

That sounds like what you proposed, except that if one selects a leader that's not on a keep, one currently gets the "random grassland hex" behavior, not the "that leader's units" behavior.

I'm not completely sure, but I think I do like the fact that when you select a grassland hex, you get the units list of all leaders. That gives an easy way to review the combined recruit lists of all leaders.

I'd consider a tooltip (hover text) which gives the specific reason.

👍

The recall list can contain units for several leaders. If no leader is selected, the list should display empty (perhaps with a hint to select a leader). The list should be only those units available to the current leader.

Okay, so how would a player go about reviewing the combined recall list of all leaders? I.e., the list of units that any leader currently in play could recall? (In theory there can be canrecruit=yes units on the recall list, so one could argue that the recall dialog should list the transitive closure of recallable units...)

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

@GregoryLundberg GregoryLundberg commented Sep 21, 2019

Consider DM. Delfador has several distinct recall lists. They are mutually exclusive. This is relatively simple to do using a filter, but requires a large amount of work storing and restoring the units in variables if you don't use the filter. It makes no sense to show all those Elven units when Delfador can only recruit Loyalists, or Loyalists when he can only recruit Elves. So, if the filter is to show all units, the make the campaign work as intended we'll need to ensure everything is saved and restored. That's a lot of work, difficult to code correctly, and prone to error when a simple filter which hides the dis-allowed units does the same job quickly, easily, and with a lot less chance of error.

So, if you're going to change the filter to show the units, do it by adding an option to expose them, default to off.

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 23, 2019

Noted.

jostephd added a commit that referenced this issue Sep 26, 2019
Wedge009 added a commit to Wedge009/wesnoth that referenced this issue Nov 12, 2019
Wedge009 added a commit to Wedge009/wesnoth that referenced this issue Nov 13, 2019
Wedge009 added a commit that referenced this issue Nov 13, 2019
Part of issue #1282.

(cherry picked from commit a7fc184)
@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Nov 13, 2019

#4362 and #4444 seemed to do a lot for this request... what's left to consider?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.