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

Sidebar selected_unit_weapons report doesn't determine which weapon specials are active correctly #4071

Open
jostephd opened this issue May 11, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@jostephd
Copy link
Member

commented May 11, 2019

When a unit with drains and backstab attacks an Orcish Grunt, the damage calculations dialog looks like this:

2019-05-11-132246_1920x1049_scrot

Note how the words "backstab" and "drains" are shown in different colors, because drains is active (Orcs aren't immune to drains) but backstab is not (there isn't an ally of the Thief behind the orc).

I use a custom theme that displays selected_unit_weapons in the sidebar. That report shows the same information as the Damage Calculations dialog:

2019-05-11-132014_1920x1080_scrot

The word "drains" and "backstab" are shown in different colors depending on whether the weapon special is active or not, but the determination of "is active?" is wrong. For example, when backstabbing a Soulless, "backstab" is shown as inactive and "drains" as active, even though it should be the other way around:

2019-05-11-135716_1920x1049_scrot

The weapon specials are added to the report here:

wesnoth/src/reports.cpp

Lines 874 to 896 in 08979a6

{
auto ctx = at.specials_context_for_listing();
boost::dynamic_bitset<> active;
const std::vector<std::pair<t_string, t_string>> &specials = at.special_tooltips(&active);
const size_t specials_size = specials.size();
for ( size_t i = 0; i != specials_size; ++i )
{
// Aliases for readability:
const t_string &name = specials[i].first;
const t_string &description = specials[i].second;
const color_t &details_color = active[i] ? font::weapon_details_color :
font::inactive_details_color;
str << span_color(details_color) << " " << " " << name << naps << '\n';
std::string help_page = "weaponspecial_" + name.base_str();
tooltip << _("Weapon special: ") << "<b>" << name << "</b>";
if ( !active[i] )
tooltip << "<i>" << _(" (inactive)") << "</i>";
tooltip << '\n' << description;
add_text(res, flush(str), flush(tooltip), help_page);
}
}

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Not 100% sure whether it's related but the was iirc a controversial change in 1.12 that made all weapon specials filters default to true on the sidebar. This was iirc in particular controversial because it made units that appear in loti that had two attack specials "x2 damage against elves" and "x2 damage against orcs" show the x4 damage even though the unit would obviously never get x4 damage against any unit.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

Not 100% sure whether it's related but the was iirc a controversial change in 1.12 that made all weapon specials filters default to true on the sidebar.

I don't see that. I had the code print "active" or "inactive" and it looks like this:

2019-05-15-110941_1920x1080_scrot

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

I mistyped I meant that all filters filtering the enemy like [filter_opponent] iirc now default to true.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

Ah, thanks for clarifying that. The change in 1.12 was to the regular unit_weapons report, right? In that context we don't have an enemy to work with, so maybe we can't be right in every case, for exapmle, some other addon could have "x2 damage against all but orcs" and "x2 damage against all but elves" specials... but in the highlighted_unit_weapons/selected_unit_weapons report we deal with two units, the selected unit and the mouseover unit, so the problem won't arise: we should be able to run the filters on the actual enemy unit.

I think it's just a matter of replacing the call to specials_context_for_listing by a call to one of the other specials_context getters..

@Leonard03

This comment has been minimized.

Copy link

commented May 17, 2019

Can't seem to be able to reproduce this issue. The code from reports.cpp above is different than what's currently in the master branch, perhaps it's a bug in the custom theme?

@jostephd

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

@Leonard03 Good catch. I was trying with 1.14 so I linked the 1.14 source code. I have now re-tested in master and there all specials show as active. This is likely due to the change @gfgtdf remembered.

So yes, the situation in master and 1.14 is different, but I stand by my opinion that when the report simulates a concrete battle between two specific units, it should display the enabled/disable values for that particular battle, not just "enabled" across the board. What do you think?

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.