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

Add widgets for compass #53980

Merged
merged 5 commits into from
Jan 8, 2022
Merged

Conversation

dseguin
Copy link
Member

@dseguin dseguin commented Jan 2, 2022

Summary

None

Purpose of change

Widgets representing the cardinal directions, indicating visible creatures and threats.
Related to wapcaplet's work on widgets: #53957.

Describe the solution


New widgets/layouts

Added new set of compass widgets:

  • "compass_N_text"
  • "compass_S_text"
  • "compass_E_text"
  • "compass_W_text"
  • "compass_NE_text"
  • "compass_NW_text"
  • "compass_SE_text"
  • "compass_SW_text"
  • "compass_local_text"
  • "compass_legend_text"

Also added these layouts:

  • "compass_top_layout" -> (NW - N - NE)
  • "compass_middle_layout" -> (W - L - E)
  • "compass_bottom_layout" -> (SW - S - SE)
  • "compass_all_layout" -> (Combination of above layouts + legend)

Arrangement

These widgets can be arranged in any order, including columns similar to the existing compass:

Existing compass (hardcoded):

static_compass

With the compass widgets:

custom_compass

The compass widgets can be modified in data/json/ui/compass.json, including the labels, widths, and arrangements.


Number of creatures displayed

Using the "width" field you can limit how many creatures to display in a compass segment:

Width = 6

6_wide

Width = 3

3_wide


Compass "direction"

The compass directions are defined using the "direction" field:

{
  "id": "compass_N_text",
  "type": "widget",
  "label": "N",
  "style": "text",
  "direction": "N",
  "var": "compass_text",
  "width": 6
}

Compass legend

Using the "height" field you can reserve multiple lines for the compass legend, which allows more creatures to be displayed in the list. See #54050 for details.

{
  "id": "compass_legend_text",
  "type": "widget",
  "style": "text",
  "var": "compass_legend_text",
  "height": 3
}

Describe alternatives you've considered

Testing

Added some tests to check that creatures are displayed in the appropriate directions. Also tested in-game by spawning creatures around and nearby the player (see screenshots above).

Additional context

Here's a comparison between the existing compass and the widget compass (available from the custom sidebar):
compass_compare

@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jan 2, 2022
@wapcaplet
Copy link
Contributor

This is a great idea!

I think I would prefer if the direction could be a separate field, instead of part of the widget_var name. Something like:

"var": "compass",
"direction": "NE"

We don't have any other immediate uses for a "direction" field but I could see it being applicable to things like terrain, furniture, items and whatnot - in other words, this one is a "monster compass" showing nearby monsters, but maybe a "terrain compass" showing the predominant terrain in each direction, or an "item compass" showing nearby items, might one day be useful.

Even if we don't come up with other good uses for "direction", I think it's worth it just to keep the var list from getting too cluttered.

@dseguin
Copy link
Member Author

dseguin commented Jan 3, 2022

I think I would prefer if the direction could be a separate field, instead of part of the widget_var name. Something like [...]

Sure, I can do that. I wonder if it might also be useful to have a generic field to pass additional data to widgets.

@NetSysFire NetSysFire added <Enhancement / Feature> New features, or enhancements on existing Info / User Interface Game - player communication, menus, etc. labels Jan 3, 2022
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 3, 2022
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 3, 2022
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 3, 2022
@PatrikLundell
Copy link
Contributor

What happens with the code description part of the the hard coded panel? A bunch of letters that may mean different things at different times isn't that useful without a key.

@dseguin
Copy link
Member Author

dseguin commented Jan 3, 2022

What happens with the code description part of the the hard coded panel? A bunch of letters that may mean different things at different times isn't that useful without a key.

Cheers for the feedback. I've added another widget for the compass legend:
(top is the existing compass, bottom is using compass widgets)
compass_compare

@PatrikLundell
Copy link
Contributor

It's easy to drop things along the way...
The next problem is with the legends widget: it needs to be variable height to account for more than two characters (you've got Z z... there showcasing the problem).

@dseguin
Copy link
Member Author

dseguin commented Jan 3, 2022

It's easy to drop things along the way... The next problem is with the legends widget: it needs to be variable height to account for more than two characters (you've got Z z... there showcasing the problem).

That may have to wait for a separate change to address the 1-line limitation for widgets.

@PatrikLundell
Copy link
Contributor

Optional legend widget 2, 3, 4, 5, ... that are displayed only when they contain anything? Might not be a reasonable option, though. If so I'd probably go for a fixed number of e.g. 3 legend widgets that would just be blank if there isn't anything to show.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 3, 2022
@dseguin dseguin force-pushed the widget_compass branch 2 times, most recently from 0820a56 to 5975192 Compare January 3, 2022 20:46
src/widget.cpp Outdated
case widget_var::compass_text:
// Compass color is specific to individual threats.
// Skip colorization.
desc = display::compass_text_color( _direction, _width );
Copy link
Contributor

@wapcaplet wapcaplet Jan 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see what you did here - the compass_text_color returns a std::string that already embeds colorized strings for each threat, and the second nc_color part is just a (dummy?) c_white that ends up being ignored.

There is a little bit of precedent in panels.h in the display namespace for functions that return only a std::string - sometimes even colorized, like display::get_moon_graphic. So I think you could change compass_legend_text_color to return an already-colorized string (instead of a string/color pair), and change its name to something like colorized_compass_legend_text, to avoid this little hack of ignoring the second part of the pair.

I'm also not crazy about doing a return from the middle of this switch/case statement; I would propose using a flag, like:

bool do_colorize = true;
switch( _var ) {
    // ...
    case widget_var::compass_text:
        desc.first = display::colorized_compass_legend_text( _direction, _width );
        do_colorize = false;
        break;
    // case ...
}
if( do_colorize ) {
    ret += colorize( desc.first, desc.second );
} else {
    ret += desc.first; // assume already colorized
}
return ret;

I expect this to come up again (in fact I was thinking about it for the vehicle and bodypart widgets too), and have a feeling this would give us a cleaner logical flow for these exceptional cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, I didn't realize I was breaking the pattern, haha.

I changed the return types for the compass display functions and removed the early returns. I'll try to stick to your format here going forward (sorry!).

@dseguin dseguin force-pushed the widget_compass branch 4 times, most recently from 21832c5 to ce48b57 Compare January 3, 2022 21:44
@dseguin dseguin force-pushed the widget_compass branch 2 times, most recently from 5edf142 to 37267a6 Compare January 6, 2022 17:50
tests/widget_test.cpp Outdated Show resolved Hide resolved
@wapcaplet
Copy link
Contributor

wapcaplet commented Jan 6, 2022

Playtested a bit, and it works great!

For fun, I bumped the width to 50, each compass point width to 9, and the legend to 6 lines - this is a little better equipped to deal with typical CDDA mobs:

image

I think the legend portion would be more readable if the monster-count came after the monster-symbol, like the old legend does. It's a legend, so the symbol ought to come first, ex. @ 5 feral humans instead of 5 @ feral humans.

The only other minor change I'd suggest in the legend is to separate monsters with 2 spaces instead of just 1. Due to wrapping limitations, there is often a space or three at the end that don't have anything better to do, so that may improve readability a little as well.

Edit Feral humans occasionally seem duplicated in the legend

@dseguin
Copy link
Member Author

dseguin commented Jan 6, 2022

I think the legend portion would be more readable if the monster-count came after the monster-symbol, like the old legend does. It's a legend, so the symbol ought to come first, ex. @ 5 feral humans instead of 5 @ feral humans.

The only other minor change I'd suggest in the legend is to separate monsters with 2 spaces instead of just 1. Due to wrapping limitations, there is often an space or three at the end that don't have anything better to do, so that may improve readability a little as well.

Thanks for giving it a try (and the feedback)! I'll hold off on making this ready until I fix the formatting.

Edit Feral humans occasionally seem duplicated in the legend

I found that happens because there are multiple different mtypes that use the "feral human" name. Not sure if I should address that by comparing the name strings instead of the id (it's a bit of a performance hit)...

Edit: Thankfully the formatting fixes were pretty straightforward. Moved the count after the symbol, and added 2 spaces between creatures:

compass_legend_format

@wapcaplet
Copy link
Contributor

Ha, you think you have enough zombie dogs there? That looks good. I am already stealing that bit of apply_color logic for the bodypart status widgets in #53975, since embedded colors will be much nicer.

@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 7, 2022
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 7, 2022
@dseguin dseguin marked this pull request as ready for review January 7, 2022 04:16
@dseguin dseguin force-pushed the widget_compass branch 2 times, most recently from f3fb0fe to 8bfc34f Compare January 7, 2022 04:59
@dseguin
Copy link
Member Author

dseguin commented Jan 7, 2022

I somehow completely forgot about monster difficulty colours. I've squashed those into the previous commit. Here's how it looks:

diff_example

@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 7, 2022
@wapcaplet
Copy link
Contributor

I somehow completely forgot about monster difficulty colours.

So did I, until the subject came up on discord last night, and I couldn't remember if you'd included it. Nice catch.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 7, 2022
src/panels.h Outdated
Comment on lines 105 to 106
std::string colorized_compass_text_color( const cardinal_direction dir, int width );
std::string colorized_compass_legend_text_color( int width, int height );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::string colorized_compass_text_color( const cardinal_direction dir, int width );
std::string colorized_compass_legend_text_color( int width, int height );
std::string colorized_compass_text( const cardinal_direction dir, int width );
std::string colorized_compass_legend_text( int width, int height );

Sorry, one last nitpick - the _text_color suffix naming convention was intended for the ones returning a std::pair<std::string, nc_color> (both text and color).

The ones that return just a std::string don't follow a particular pattern so much, but for these pre-colorized ones I'm leaning toward colorized_xxx_text, hence the suggestion here. I went with colorized_bodypart_status_text for the recently merged bodypart status widgets, and will probably do colorized_overmap_text for the upcoming overmap one, so would like to set a good precent early if possible. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers! I folded those name changes into the previous commit.

@kevingranade kevingranade merged commit 400dd45 into CleverRaven:master Jan 8, 2022
@dseguin dseguin deleted the widget_compass branch January 8, 2022 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Enhancement / Feature> New features, or enhancements on existing Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants