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

Fix #4456: Sidebar abilities display leaks shrouded information #4462

Open
wants to merge 2 commits into
base: master
from

Conversation

@blaf
Copy link
Contributor

blaf commented Oct 13, 2019

No description provided.

Copy link
Member

jostephd left a comment

Thanks for the quick follow up PR! However, I think the fix isn't quite right.

const map_location& mouseover_hex = rc.screen().mouseover_hex();
const map_location& displayed_unit_hex = rc.screen().displayed_unit_hex();
const map_location& hex = (mouseover_hex.valid() && !viewing_team.shrouded(mouseover_hex)) ? mouseover_hex : displayed_unit_hex;

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 14, 2019

Member

I'm sorry, but this doesn't seem to be the correct fix. With this change:

patch: units adjacent to the Friendly Explorer are invisible
diff --git a/data/core/macros/abilities.cfg b/data/core/macros/abilities.cfg
index 32e9f492c79..11d26755176 100644
--- a/data/core/macros/abilities.cfg
+++ b/data/core/macros/abilities.cfg
@@ -259,9 +259,9 @@ Enemy units cannot see this unit while it is in a village, except if they have u
 Enemy units cannot see this unit while it is in deep water, except if they have units next to it. Any enemy unit that first discovers this unit immediately loses all its remaining movement."
         affect_self=yes
         [filter]
-            [filter_location]
-                terrain=Wo*^*
-            [/filter_location]
+            [filter_adjacent]
+                side=3
+            [/filter_adjacent]
         [/filter]
     [/hides]
 #enddef
diff --git a/data/scenario-test.cfg b/data/scenario-test.cfg
index 75034c8b22a..24421e8b526 100644
--- a/data/scenario-test.cfg
+++ b/data/scenario-test.cfg
@@ -76,7 +76,7 @@ Xu, Xu, Qxu, Qxu, Ql, Ql, Ql, Xu, Xu, Xu, Xu, Xu, Xu, Xu, Xu, Gg, Gg, Gg, Gg, Gg
         gold=2000
         village_support=5
         team_name="1"
-        shroud=yes
+        fog,shroud=yes,no
         recall_cost=42
         [village]
             x,y=9,7

There's still an information leak:

2019-10-14-025011_168x143_scrot

(That's the northeast corner of the test scenario.)

Look at, say, the unit_alignment report for comparison:

wesnoth/src/reports.cpp

Lines 396 to 411 in 7c4e037

REPORT_GENERATOR(unit_alignment, rc)
{
const unit *u = get_visible_unit(rc);
const map_location& mouseover_hex = rc.screen().mouseover_hex();
const map_location& displayed_unit_hex = rc.screen().displayed_unit_hex();
const map_location& hex = mouseover_hex.valid() ? mouseover_hex : displayed_unit_hex;
return unit_alignment(rc, u, hex);
}
REPORT_GENERATOR(selected_unit_alignment, rc)
{
const unit *u = get_selected_unit(rc);
const map_location& attack_indicator_src = game_display::get_singleton()->get_attack_indicator_src();
const map_location& hex_to_show_alignment_at =
attack_indicator_src.valid() ? attack_indicator_src : u->get_location();
return unit_alignment(rc, u, hex_to_show_alignment_at);
}

That report doesn't check shroud explicitly; instead, it uses get_visible_time_of_day_at. I suppose something similar is needed here.

This comment has been minimized.

Copy link
@blaf

blaf Oct 20, 2019

Author Contributor

I spent some time digging and eventually I went back to check version before my fix to find out that this particular leak of information at a fogged area is already there, and does not seem related to REPORT_GENERATOR(unit_abilities).

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 21, 2019

Member

First of all, sorry for the wild goose chase you went on. I didn't mean to imply this issue was a regression caused by this PR. I do agree, it's a separate issue, I've created #4497 for it.

What I was trying to say, however, is that I don't understand why unit_abilities special-cases shrouded units while unit_alignment does not. I suspect that means unit_abilities would show the wrong information for fogged-but-not-shrouded abilities in some cases, even if the example in my previous comment isn't the right one.

This comment has been minimized.

Copy link
@blaf

blaf Oct 21, 2019

Author Contributor

The get_visible_time_of_day_at also uses .shrouded() internally.
I think that what needs to be looked at is not these sidebar reports but something that drives the invisibility pictogram to appear on the mouseover hex.

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 21, 2019

Member

Yes, the mouseover issue is separate from the sidebar issue. #4497 is about the mouseover issue and this PR is about the sidebar. This PR makes unit_abilities check shrouded but not check fog, and I am not sure that's correct: I think there might be some cases where the sidebar abilities display will show the wrong information when fog is enabled and shroud not.

get_visible_time_of_day_at which you mention checks both shroud and fog internally. The else if (fogged) case in that function, for example, returns a time of day modified by terrain illumination (caves, lava, etc) but not by unit abilities (such as adjacent Mages of Light).

This comment has been minimized.

Copy link
@blaf

blaf Oct 26, 2019

Author Contributor

I narrowed the issue down to

!unit_filter(vconfig(afilter)).set_use_flat_tod(illuminates).matches(*this, loc)

inside unit::ability_active.

When ability_active is called from ability_tooltips, it works just fine for fogged units. If it is called from unit::invisible through get_ability_bool, it reveals the hidden information.
The line mentioned above gives different boolean value for the two cases.

Anyway I think that it is a separate issue and this PR maybe could be merged?

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 27, 2019

Member

Thanks for analyzing that. Is it about #4497? If so, could you post it over on that issue, please?

I'm still not sure it's correct to handle shroud but not fog in unit_abilities, but I'm afraid I won't have much time for code reviews this week, I hope one of the other developers can take over.

This comment has been minimized.

Copy link
@blaf

blaf Oct 27, 2019

Author Contributor

Yup, will put the comment there.

I think that the fog handling of illuminates is not perfect anyway. If, after applying your patch, I put a blue side unit with that ability at 27,15 (fog border), the fogged yet illuminated area does show illuminated on the map, but on mouse over it does not show sidebar as illuminated.

As far as I can tell the abilities work just fine in sidebar (with this PR), you can see that (again after applying the patch) even when the hidden pictogram shows, the submerge ability is greyed out as not active. After revealing the location the submerge is properly highlighted as active.

}
REPORT_GENERATOR(selected_unit_abilities, rc)
{
const unit *u = get_selected_unit(rc);

const map_location& mouseover_hex = rc.screen().mouseover_hex();
const unit *visible_unit = get_visible_unit(rc);
if(visible_unit && u && visible_unit->id() != u->id() && mouseover_hex.valid())
const team &viewing_team = rc.teams()[rc.screen().viewing_team()];
if (visible_unit && u && visible_unit->id() != u->id() && mouseover_hex.valid() && !viewing_team.shrouded(mouseover_hex))

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 14, 2019

Member

I suppose this shrouded check would need to change too, but even before that, what does this change fix? #4456 doesn't affect the selected_unit_abilities report, does it?

This comment has been minimized.

Copy link
@blaf

blaf Oct 20, 2019

Author Contributor

It does not, I changed this to be consistent with the REPORT_GENERATOR(unit_abilities).

@blaf blaf requested a review from jostephd Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.