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 range and damage type icons #3732

Closed
wants to merge 1 commit into from

Conversation

@jostephd
Copy link
Member

commented Nov 18, 2018

From #3197 (comment) @CelticMinstrel

This makes the sidebar look like this:

screenshot_2018-11-16_00-16-47

The new icons have tooltips "longbow" "ranged" "pierce".

If we wanted the attack icon (first one) to be larger, we could make it look like this:

Larger attack icons - can be implemented

screenshot_2018-11-18_13-01-07

Larger attack icons - better looking, but don't know how to implement

screenshot_2018-11-18_05-09-14

This is not a screenshot, it's a mock-up created in gimp. If would be great to be able to implement this, but as CelMin said on the other issue:

But I'm not sure if the ThemeWML can do that within a single report? (And if weapon icons were a separate report you'd have alignment issues, because a weapon description's length depends on the number of specials.)

  • Use icons in sidebar
  • Move the icons a few pixels down to align them with the text
  • Delete the words "melee-impact" in the sidebar
  • Use icons in unit_preview_pane - #3740
  • Use icons in generated help pages - #3940
  • Document in the wiki how to supply custom icons for custom ranges/damage types - added to https://wiki.wesnoth.org/Unittypewml#Attacks
  • Use attack icons in the sidebar - not done, opened #3941 for it

@jostephd jostephd added this to the 1.15.0 milestone Nov 18, 2018

@jostephd jostephd self-assigned this Nov 18, 2018

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2018

I thought the whole idea behind having items was to replace the corresponding text (without loosing information of course!), which in particlar fixes probems arising from the text having different lengths in differnt languages and this potentially not fitting into the screen in some languages.

Your current patch does however give the 'name' label even less space than before potentially resulting in the attack names being cut off in some languages, for example in your screenshot the text 'longbow' has too few space behind it which will likeley be a problem in other languages.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

For the record, if we can't get the mocked-up version, I'd probably prefer not showing the attack icon, since it either wastes a lot of space or is too tiny to make out.

I'll also add, again, that I think the text indication of range and damage type is now redundant and can be removed.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

I'm not sure about removing the words "ranged-pierce" from the text part of the report. It could work for mainline, but a UMC era that uses custom ranges or damage types would need to include icons for them. Would that be an obstacle for UMC authors? We should be making things easy for them.

These icons are quite small, so I don't think it would be a major obstacle for UMC authors; and I don't imagine many addons use custom ranges or damage types anyway. (Maybe we should ask someone to grep for that though.)

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2018

I can remove the "melee-impact" text, sure.

About the mockup, wouldn't it leave too little horizontal room for the weapon specials? I.e., even if we could implement it, would it be acceptable?

There are icons for traits too (in the same directory as the icons for damage types) but some of them are just text, like https://github.com/wesnoth/wesnoth/blob/1.14/images/icons/profiles/skirmisher.png

About UMC, Imperial Era has an energy damage type and Ageless Era has

#ifdef MULTIPLAYER
[language]
    range_artillery= _ "artillery"
    type_insects= _ "insects"
    type_secret= _ "secret"
    range_sapper= _ "sapper"
    range_kamikaze= _ "kamikaze"
    range_study= _ "study"
[/language]

(I recognize the first, third and fifth from campaigns)

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

About the mockup, wouldn't it leave too little horizontal room for the weapon specials? I.e., even if we could implement it, would it be acceptable?

You actually have a very good point there...

I also don't like how the small icons appear right under the attack icon in the first version with larger icons, though. Would it work to put the small icons at the end of the line instead of the beginning?

One other thing: I'm not overly enamoured by the red background of the damage type icons. I don't have any better suggestions though.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2018

You were right, without the melee-impact text it looks much better:

screenshot_2018-11-18_15-45-27

I also added fallback, if either the black icon or the red one isn't found, the melee-impact text will be added anyway. (for UMC)

(I'm waiting for more input about the attack icons)

I also don't like how the small icons appear right under the attack icon in the first version with larger icons, though. Would it work to put the small icons at the end of the line instead of the beginning?

Hmm... I think you could do that but you'd have to use ~BLIT() to add the two small icons in one add_image() call, i.e., one add_image() of a 32x32 image and one add_image() of a single 16x32 image. Could you try it?

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2018

the new screenshow still has the problem with of cut-off text in translations. (also: did you remove the the wepaons specials text or it is just becasue the wepons in your case have no specials?)

I think it'd be better if the text stayed in its own line meaning basicially making it look like it does in 1.15 but have the 'range-type' and the 'strikes-damange' lines meged in one line with the icons and the 6x3 string.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2018

the new screenshow still has the problem with of cut-off text in translations.

Yes, like I said, I'm waiting for more feedback on the 16x16 attack icon. We might remove it, keep it, or enlarge it to 32x32 and put the red and black icon vertically stacked next to it, and the attack stats below. Nothing's been decided yet.

(also: did you remove the the wepaons specials text or it is just becasue the wepons in your case have no specials?)

The latter.

I think it'd be better if the text stayed in its own line meaning basicially making it look like it does in 1.15 but have the 'range-type' and the 'strikes-damange' lines meged in one line with the icons and the 6x3 string.

So, you're proposing exactly what the last screenshot shows, but to remove the attack icon, right?

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

So, you're proposing exactly what the last screenshot shows, but to remove the attack icon, right?

hmm no i think i misrememberd how it looks on 1.14 sry.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

I still think "4 x 2 dagger [melee] [pierce]" would be better if you can get that, and with this format I think it would look fine with the large attack icon above it.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

@CelticMinstrel Like this?

screenshot_2018-11-20_08-19-55

There's a problem in the screenshot: the unit has 4 attacks, but the fourth attack (life bomb) and the weapon specials of the third attack aren't shown at all, because the longbow attack overflowed the width. (The code generating this is in the spoiler.)

I don't think I said this already: I like the red and black icons being before the numbers because then those icons are aligned from line to line, and every line starts with the range, damage type, hits and damage per hit. The Orcish Archer shows this nicely.

The red and black icons are placed a few pixels too high, I think. (I don't mean their size - they're 16x16 and I guess that's fine - I mean their position relative to the vertical alignment of the line of text.) Look in the screenshots how the top of the icon is close to the tails of the letters "y" and "j" of the line above it. How do I fix this? How do I know how many pixels to offset the icons by?

diff --git a/src/reports.cpp b/src/reports.cpp
index a565574b858..cec2f9612e0 100644
--- a/src/reports.cpp
+++ b/src/reports.cpp
@@ -721,17 +721,15 @@ static int attack_info(reports::context & rc, const attack_type &at, config &res
 
 		// The default images are 60x60 and have a 2-pixel transparent border. Trim it.
 		// The first SCALE() accounts for theoretically possible add-ons attack images larger than 60x60.
-		add_image(res, at.icon() + "~SCALE_INTO_SHARP(60,60)~CROP(2,2,56,56)~SCALE_INTO_SHARP(16,16)", at.name());
+		add_image(res, at.icon() + "~SCALE_INTO_SHARP(60,60)~CROP(2,2,56,56)~SCALE_INTO_SHARP(32,32)", at.name());
 		add_text(res, " ", "");
 		// SCALE_INTO_SHARP() is needed in case the 72x72 images/misc/missing-image.png is substituted.
 		const std::string range_png = std::string("icons/profiles/") + at.range() + "_attack.png~SCALE_INTO_SHARP(16,16)";
 		const std::string type_png = std::string("icons/profiles/") + at.type() + ".png~SCALE_INTO_SHARP(16,16)";
 		const bool both_images_exist = image::locator(range_png).file_exists() && image::locator(type_png).file_exists();
-		add_image(res, range_png, range);
-		add_image(res, type_png, lang_type);
 		str << span_color(dmg_color) << "  " << damage << naps << span_color(font::weapon_color)
 			<< font::weapon_numbers_sep << num_attacks << ' ' << at.name()
-			<< "</span>\n";
+			<< "</span>";
 		tooltip << _("Weapon: ") << "<b>" << at.name() << "</b>\n"
 			<< _("Damage: ") << "<b>" << damage << "</b>\n";
 
@@ -787,6 +785,10 @@ static int attack_info(reports::context & rc, const attack_type &at, config &res
 		}
 
 		add_text(res, flush(str), flush(tooltip));
+		add_text(res, " ", "");
+		add_image(res, range_png, range);
+		add_image(res, type_png, lang_type);
+		add_text(res, "\n", "");
 
 		if(!both_images_exist) {
 			str << span_color(font::weapon_details_color) << "  " << "  "
@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

Another design:

screenshot_2018-11-20_14-43-17

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

I think it'd be better if you had basically two lines one with just the name, the other with 7x3 [range icon], [type icon]. And in front of these two the big attack icon going over these two lines.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

I could see that working with the attack icon on the left, as in the last screenshot, but not with it above, because then there would be a ton of unused horizontal space.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

yes I meant on the left.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

so smething like
grafik

My main reason is again that i think you last two suggestio will have problems with longer attack names, like for exmple in mainline "composite bow" or "berzerker frenzy".

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

Yeah, I think I like gfgtdf's proposal. It might be better to indent the weapon specials more to line up with the rest of the text, though.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

I understand what you mean now, but I don't think it's currently possible to put two lines of text alongside the large attack icon (not without changing display::refresh_report). If I do add_image(... ~SCALE_INTO(32,32)...); add_text(res, "foo\nbar\n", "");, the foo is aligned with the top of the image but the bar is shown below the image, where the weapon specials are. What I know how to produce is this:

screenshot_2018-11-21_03-04-26

The code for this is in the spoiler.

diff --git a/src/reports.cpp b/src/reports.cpp
index 2779331a2ac..2cfac9a2573 100644
--- a/src/reports.cpp
+++ b/src/reports.cpp
@@ -724,18 +724,17 @@ static int attack_info(reports::context & rc, const attack_type &at, config &res
 		const std::string range = string_table["range_" + at.range()];
 		const std::string lang_type = string_table["type_" + at.type()];
 
-#if 0
+#define ATTACK_ICON 1
+#if ATTACK_ICON
 		// The default images are 60x60 and have a 2-pixel transparent border. Trim it.
 		// The first SCALE() accounts for theoretically possible add-ons attack images larger than 60x60.
-		const std::string attack_icon = at.icon() + "~SCALE_INTO_SHARP(60,60)~CROP(2,2,56,56)~SCALE_INTO_SHARP(16,16)";
+		const std::string attack_icon = at.icon() + "~SCALE_INTO_SHARP(60,60)~CROP(2,2,56,56)~SCALE_INTO_SHARP(32,32)";
 #endif
 		// SCALE_INTO_SHARP() is needed in case the 72x72 images/misc/missing-image.png is substituted.
 		const std::string range_png = std::string("icons/profiles/") + at.range() + "_attack.png~SCALE_INTO_SHARP(16,16)";
 		const std::string type_png = std::string("icons/profiles/") + at.type() + ".png~SCALE_INTO_SHARP(16,16)";
 		const bool both_images_exist = image::locator(range_png).file_exists() && image::locator(type_png).file_exists();
-		str << span_color(dmg_color) << "  " << damage << naps << span_color(font::weapon_color)
-			<< font::weapon_numbers_sep << num_attacks << ' ' << at.name()
-			<< "</span>\n";
+		str << span_color(font::weapon_color) << at.name() << "</span>\n";
 		tooltip << _("Weapon: ") << "<b>" << at.name() << "</b>\n"
 			<< _("Damage: ") << "<b>" << damage << "</b>\n";
 
@@ -797,6 +796,14 @@ static int attack_info(reports::context & rc, const attack_type &at, config &res
 				<< range << font::weapon_details_sep
 				<< lang_type << "</span>\n";
 		}
+		const std::string range_and_type_fallback_text = flush(str);
+
+		str
+			<< span_color(dmg_color) << "  " << damage << naps
+			<< span_color(font::weapon_color)
+			<< font::weapon_numbers_sep << num_attacks
+			<< "</span>\n";
+		const std::string damage_and_num_attacks_line2 = flush(str);
 
 		tooltip << _("Weapon range: ") << "<b>" << range << "</b>\n"
 			<< _("Damage type: ")  << "<b>" << lang_type << "</b>\n"
@@ -837,14 +844,15 @@ static int attack_info(reports::context & rc, const attack_type &at, config &res
 		}
 		const std::string damage_versus_tooltip = flush(tooltip);
 
-#if 0
+#if ATTACK_ICON
 		add_image(res, attack_icon, at.name());
 		add_text(res, " ", "");
+		add_text(res, damage_and_num_attacks.str, damage_and_num_attacks.tooltip);
 #endif
 		add_image(res, range_png, damage_versus_tooltip);
 		add_image(res, type_png, damage_versus_tooltip);
-		add_text(res, damage_and_num_attacks.str, damage_and_num_attacks.tooltip);
-		add_text(res, flush(str), damage_versus_tooltip);
+		add_text(res, damage_and_num_attacks_line2, damage_and_num_attacks.tooltip);
+		add_text(res, range_and_type_fallback_text, damage_versus_tooltip);
 
 		const std::string &accuracy_parry = at.accuracy_parry_description();
 		if (!accuracy_parry.empty())
@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

That's once again the thing I don't like - the two small icons being flush with the large one. I guess I could live with that gap between "thorns" and "7 × 3", though it'd be better if "thorns" could be bottom-aligned relative to the image (but I'm guessing that's not supported).

What about swapping the "7 × 3" and the small icons? Not sure if that would look good with "7 × 3" right below the attack icon, though...

...

...

Come to think of it, I suppose maybe you could get gfgtdf's design by slicing the attack icon in half and adding it as two separate images... but I wonder how reliable that would be, maybe it would end up putting a gap between the two halves in some (or all?) situations?

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

the two small icons being flush with the large one

It'd be easy to add some padding before them.

What about swapping the "7 × 3" and the small icons? Not sure if that would look good with "7 × 3" right below the attack icon, though...

The icons are the same width for all attacks, but the 7x3 part is not, so if the 7x3 part were first the attack icons wouldn't be vertically aligned with each other.

Come to think of it, I suppose maybe you could get gfgtdf's design by slicing the attack icon in half and adding it as two separate images... but I wonder how reliable that would be, maybe it would end up putting a gap between the two halves in some (or all?) situations?

I guess that would work.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

So it works...

screenshot_2018-11-21_04-52-26

... but the two halves are not 16px and 16px as you might expect, but 18px and 14px. If it were 16px, there would be a 2px gap between the top and bottom half. I expect that gap would reappear if someone edited their default.cfg to increase the font size, and therefore I don't think this solution would be acceptable in mainline.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

Yeah, I guess that's no good then. It looks great though... :(

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

So I guess we'll go with #3732 (comment) minus the attack icons?

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

we could also edit the cpp to be able to handle this, I know we are not that motivated updating the theme engine that we wanted to remove, but if it's simple I say that's still an option.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

Sure, let's go with that.

Maybe we could use the other format that I liked in the unit preview pane, though? If we're using icons in the sidebar we should probably use the same icons in places like the recruit window.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2018

Okay, I'll clean up this PR to use the agreed format and then I'll see about adding the range and type icons to the unit preview pane (that was already on my list).

@jostephd jostephd force-pushed the jostephd:attack-icons branch from 3a0c011 to b85faf5 Nov 23, 2018

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2018

Force pushed, it now looks like this:

screenshot_2018-11-23_04-46-09

Okay to merge? (I'll do the unit preview pane changes in a separate PR.) (edit: #3740)

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

What does it look like with weapon specials?

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2018

screenshot_2018-11-23_14-06-53

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

Yeah, seems fine.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 24, 2018

I thought the padding was a bit off, but okay. I'll give it a few more days and merge if no further comments.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Nov 24, 2018

If you want to adjust the padding, I probably wouldn't have any objections.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2018

Did you test whether the longer names like "berzerker frenzy" still fit in?

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 25, 2018

@gfgtdf That shows as "berserker fre..." and the weapon special shows on the next line as usual. I think that falls under "acceptable losses".

@Vultraz

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

Is it possible to add some more vertical spacing between attacks to further differentiate them?

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2018

We could easily add an empty line before each black icon, if that wouldn't overflow the bottom of the sidebar for units with many attacks and many weapon specials?

@Vultraz

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

How does that look?

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2018

screenshot_2018-11-26_09-27-27

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

Feels like a bit too much space... personally I don't think it even needs extra space though.

@Vultraz

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

Yeah, that does seem like too much space. How about half that?

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

screenshot_2018-11-26_09-27-27-2

This is a mockup, the previous one was a screenshot.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2019

Merged in 08c4695 and 9296158.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2019

Remaining work is tracked in other issues now (see first post).

@jostephd jostephd closed this Feb 24, 2019

jostephd added a commit that referenced this pull request Mar 8, 2019

@jostephd jostephd added this to Damage type and range icons in Damage type and range icons Mar 20, 2019

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