-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Bodypart status widget: Symbols + Legend #54531
Bodypart status widget: Symbols + Legend #54531
Conversation
Spell checker encountered unrecognized words in the in-game text added in this pull request. See below for details. Click to expand
This alert is automatically generated. You can simply disregard if this is inaccurate, or (optionally) you can also add the new words to |
87bbf2f
to
b8eb2d5
Compare
b2c375b
to
9e53ca0
Compare
This looks so cool, awesome job. The code looks nice and clean as well, and extensible for more statuses (I expect we'll have significantly more body part statuses than this some day). |
9e53ca0
to
7a05a51
Compare
5525b57
to
ae931e4
Compare
Co-authored-by: Eric Pierce <[email protected]>
This also changes the checks to match individual statuses. The reason is because std::map modifies the order, so checking against a single string is not guaranteed to match.
ae931e4
to
113d04a
Compare
"id": "bodypart_status_sym_text", | ||
"//": "Base widget for body part status symbols; needs bodypart field defined in derived widget.", | ||
"type": "widget", | ||
"style": "text", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although the base style is "phrase", the copy has style "text", and it still works OK right?
If so, there may be no need for a separate "phrase" style; we should just use "text" for all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, copy-from
will overwrite the style with whatever you give it. You can put anything in the style
field, really. And the display
function can decide how to interpret it if it detects that the widget has a phrases
array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hangon, I think I know why I got confused, and I hope you'll indulge me a lengthy explanation because I just realized it's an important oversight in the widget implementation.
There's something in the widget class that I stuck there as part of my original idea for phrase widgets, but which is basically dead code. It's this widget::text
function, invoked by widget::value_string
for "text" style widgets.
Lines 765 to 768 in 62a046c
std::string widget::text( int value, int /* value_max */ ) | |
{ | |
return _strings.at( value ).translated(); | |
} |
My thinking back then was to define a field something like "strings": [ "Peckish", "Hungry", "Ravenous" ]
, and these strings would be mapped to 0, 1, 2 - as returned by some (still mythical) function that yielded a simple numerical progression. It's even defined in the header, and the first thing read in at the top of widget::load
:
Lines 159 to 160 in 62a046c
// String values mapped to numeric values or ranges | |
std::vector<translation> _strings; |
Lines 223 to 225 in 62a046c
void widget::load( const JsonObject &jo, const std::string & ) | |
{ | |
optional( jo, was_loaded, "strings", _strings ); |
But widget::text
is never invoked, because widget::show
circumvents color_value_string
for any widget that matches uses_text_function
- in other words all the *_text
widgets I've defined since then, because it was just easier that way.
Lines 409 to 411 in 62a046c
if( uses_text_function() ) { | |
// Text functions are a carry-over from before widgets, with existing functions generating | |
// descriptive colorized text for avatar attributes. The "value" for these is immaterial; |
Now, I like the "phrases"
data structure a lot better than a naked list of "strings"
, but this may be the opportunity to hook up that widget::text
function for the dynamic rendering it was always destined for. All the dirty work is already done by your widget_phrase
class. Would it be feasible to hook into place, you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be feasible to hook into place, you think?
Absolutely. I can connect the dots in a follow-up PR. It will probably pave the way for #54585.
[...] this may be the opportunity to hook up that widget::text function for the dynamic rendering [...]
That's a great idea, and along the lines of what I'd like to do as well. It could work like a convenience function to wrap around the phrase functions.
My thought for widget_phrase
was a simple container for fields, which are accessed via static methods:
widget_phrase::get_<field>_for_<condition>( <conditions>, <widget to search> );
widget_phrase::get_text_for_id( const std::string &phrase_id, const widget_id &wgt );
The widget functions could wrap these for convenience. The main reason I'm using static accessors for widget_phrase
is because I wanted to avoid passing the widget itself to the display
functions.
113d04a
to
2f5ece6
Compare
Replaces descriptions for body_parts and statuses to reflect the new json additions.
Summary
None
Purpose of change
Fixes #54455.
Describe the solution
Add a series of widgets for displaying bodypart status in symbols, with a legend to describe the listed symbols. I also did a little bit of refactoring for code reuse, as well as give the compass widget the custom width treatment.
The new status widgets only display symbols when there's an affliction on a listed body part:
(New widget on top, existing widget on bottom)
No status / healthy
With a few status ailments
Even more ailments
All status effects
Each of these (plus the legend) are separate widgets that can be arranged in any order/layout.
Data-driven text + colour
I've also partially implemented #54585 for data-driven definitions for text, symbols and colour:
Describe alternatives you've considered
Use to the existing bodypart status widgets, which work fine but take up more space.
Testing
In addition to the screenshots above, I added some unit tests to verify that the behaviour is at least on par with the existing bodypart status widget. The refactored part uses a
std::map
to deal out the status strings, which is not guaranteed to maintain a specific order, so I modified some of the existing tests to check statuses individually instead of as a single string.Additional context
wapcaplet mentioned some additional features in the linked issue that I'd like to include:
Dynamic height is working as expected (unused lines are collapsed):