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

UI: Highlight enemy units in "Show Enemy Moves" mode #3961

Merged
merged 2 commits into from Mar 5, 2019

Conversation

Projects
None yet
5 participants
@jostephd
Copy link
Member

commented Mar 2, 2019

Fixes #1989

Screenshot:
screenshot_2019-03-03_15-01-33

Does the implementation look right? I put the new member function in game_display next to the other reach functions, but then I had to use a dynamic_cast in unit_drawer because it can be constructed from a plain display:

unit_drawer drawer = unit_drawer(*this);

@jyrkive
Copy link
Member

left a comment

Looks mostly okay. Here is some small nitpicking.

Show resolved Hide resolved src/display.hpp Outdated
Show resolved Hide resolved src/game_display.cpp Outdated
Show resolved Hide resolved src/game_display.cpp Outdated
Show resolved Hide resolved src/game_display.cpp Outdated
Show resolved Hide resolved src/game_display.hpp Outdated
Show resolved Hide resolved src/game_display.hpp Outdated
Show resolved Hide resolved src/units/drawer.hpp
Show resolved Hide resolved src/game_display.cpp Outdated
Show resolved Hide resolved src/units/drawer.cpp Outdated
@jostephd

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

Thanks for the review, @jyrkive! I've pushed the fixes. I'll squash before merging.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

Um... how exactly are they hilited? By having an ellipse? Don't all units normally have an ellipse?

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2019

If you have two units and you click one of them, that unit is drawn whiter and with a stronger ellipse, right? That's how units are highlighted. I've re-taken the screenshot in the first post with ellipses enabled, you can now see the difference between the faint ellipses of units that can't reach and the bolder ellipses of units that can reach.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

Ahhh, got it. Makes sense.

@jostephd jostephd merged commit 25957e3 into wesnoth:master Mar 5, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

@soliton-

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

This looks like it highlights units that can move to the hex not also those that can attack it. The request is for the latter and that is what most people will expect as well I imagine. Nice work either way!

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

This looks like it highlights units that can move to the hex not also those that can attack it. The request is for the latter and that is what most people will expect as well I imagine.

Oh, I see. I highlighted all units that can reach a hex because that matched the number that's already shown. It's also logical in its own way: when you form a defensive line it's fair to ask both "which enemy units can attack this hex" and "which enemy units can reach that hex".

Highlighting the enemy units that can reach a given friendly unit.. I think that could be done by entering "Show Enemy Moves" mode while highlighting a friendly unit (possibly in planning mode), what do you think?

I'll reopen #1989 and fix changelog.

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

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

fixup! UI: Highlight enemy units in "Show Enemy Moves" mode (#3961)
In the test scenario, highlighting (26,12), pressing Ctrl+V,
highlighting (25,12), and pressing Ctrl+V again refreshed highlighting
of the Ghost but not of the Elvish Sharpshooter.  It was similar
with the enemy leaders in the first scenarios of AToTB and AOI.
I haven't figured out exactly why, but invalidating the units' hexes
like toggleellipse does seems to fix it.

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

fixup! UI: Highlight enemy units in "Show Enemy Moves" mode (#3961)
In the test scenario, highlighting (26,12), pressing Ctrl+V,
highlighting (25,12), and pressing Ctrl+V again refreshed highlighting
of the Ghost but not of the Elvish Sharpshooter.  It was similar
with the enemy leaders in the first scenarios of AToTB and AOI.
I haven't figured out exactly why, but invalidating the units' hexes
like toggleellipse does seems to fix it.
@vgaming

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

@jostephd awesome to see work on this feature -- thanks!
The difference to "Show Enemy Moves" is that the exact units can be highlighted, not only the number of those units. Basically, the feature proposal is to answer this question:
"If I go to hex XY, which units will be able to attack me?"

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2019

@vgaming Yeah, I understand the difference now. Code-wise it should be easy to implement what you describe (basically, instead of processing the hex under the mouse, process the hexes adjacent to it). What I'm not sure about is the user interface, what keyboard/mouse actions will trigger "Highlight enemy units that can reach the hex under the mouse" and what keyboard/mouse actions will trigger "Highlight enemy units that can reach hexes adjacent to the one under the mouse". Can you suggest a user interface for the latter?

@vgaming

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@jostephd do we really need an interface for that though? (For showing enemy units that can take a hex.)
I guess there are scenarios for that, but is it common? Personally, I'd only ever use the main feature I guess..

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

I was asking if you could suggest an interface for "highlighting units that can attack a hex". That said, "highlight units that can take a hex" sounded sensible/intuitive to me, for a few reasons:

  • The numbers in the hexes show how many units can take a hex, so it's intuitive to highlight those units.
  • It's easy to use "highlight units that can take a hex" to answer "which units can attack a hex" (do "Show Enemy Moves" on each of the surrounding hexes), but not easy to use "highlight units that can attack a hex" to answer "which units can take a hex".

Maybe you could playtest with this branch and see how it feels to you?

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.