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

Improve display of the crafting screen header on narrow windows #56822

Merged

Conversation

ZeroInternalReflection
Copy link
Contributor

@ZeroInternalReflection ZeroInternalReflection commented Apr 15, 2022

Summary

Interface "Improve display of the crafting screen header on narrow windows"

Purpose of change

On narrow screens (80-~120 characters wide, depending on language), the the crafting screen (&) encounters several rendering issues:

  1. The information on hidden recipes will start to print over the recipe category tabs in the header.
  2. On particularly narrow screens, the recipe tabs will occasionally wrap around, partially printing over the header border
  3. Subcategory tabs will print over the border of the crafting screen, and on particularly narrow screens, wrap around to the next line
  4. Subcategory tabs that would be printed to the right of a tab that has wrapped are not printed at all, with no indication that there are additional subcategories for the player to look at.

Observe that the "Practice", "Animals", "Other", "Appliance", "Armor", and even "Electronic" tabs are progressively obscured by the discussion of hidden recipes. The "Other" subcategory also disappears as the window gets smaller.
Before_English_Weap_Tab

Describe the solution

Move the recipe category tabs and the general information to separate windows to prevent overlapping.
Prior to drawing the recipe category tabs (and the subcategory subtabs), the available width is checked and the required width to display all tabs is calculated. If the full list of tabs won't fit, then the largest collection of them that a) fits, and b) contains the currently selected tab is generated. This is then drawn, complete with "<" and ">" placeholder tabs indicating if there are additional tabs to the left and right, respectively.

Describe alternatives you've considered

Ignore it:

  • I'm not sure what percentage of the user base plays at this screen size. However, since there are still new crafting categories being added, it's going to be more and more of a problem.

Abbreviate crafting categories when there's insufficient space, as done in the vehicle part installation menu:

  • Usefully abbreviating "Ammo", "Armor", "Appliance",and "Animals" sounds like fun. Particularly when a properly abbreviated translation is required for each tab

Hiding empty subcategories rather than merely using a different colour:

  • Doesn't feel like a very long-term solution and doesn't fix display issues with the main category tabs

Move the display of hidden/new/crafting status information to a subheader

  • Wouldn't completely resolve the width issue, and is likely to cause knock-on UI issues

Raise the minimum screen width:

  • Seems like a major decision

Testing

Tested in the full range of widths:
English_Weap_Tab_After
Tested scrolling through tabs in a small window:
Loop_Through_Tabs_After
Tested scrolling through subcategories in a small window:
Loop_Through_Subtabs_After

Tested in several other languages. I cannot read Portuguese or Ukrainian, but it doesn't look like the changes broke anything:
Port_Other_Tabs_After

Ukr_Practice_Tab_After

Additional context

  • I'm not entirely satisfied with the overloading of fit_tabs_to_width vs. simple_fit_tabs_to_width, but this is the simplest setup I found that worked for both draw_tabs and draw_subtab. Perhaps another set of eyes will see a cleaner way.
  • Several other places use draw_tabs/draw_subtab, such as the faction screen, bionics, and character creation. To my knowledge, none of them have the same screen real estate issues. However, if they do, it should be relatively simple to implement this on those screens as well
  • The way I added the unread recipe flag to the category tabs meant that draw_tab needed to be able to handle color tags. I don't believe this will break anything, but it might be relevant for anyone working on those screens
  • I was tempted to prioritize showing the tabs immediately left and right of the selected tab (so the user can see what the next category they'll look at will be), but ultimately didn't think the benefit would be significant. If others disagree, I don't think it would be too difficult to implement.
  • I believe that at least one of the templated overloads of draw_tabs in output.h is now no longer in use, but I did not remove it in this PR.
  • The colour c_dark_gray displays oddly on my computer. In the "Before" animation, it defaulted to c_light_gray. For the "After" animations, I changed it to c_dark_gray_red so I could see which subcategories were empty.
  • If this was too many gifs, do let me know. I tried to compress them further, but that obscured a lot of the relevant information.

On narrow screens (80-~120 characters wide, depending on language), the
the crafting screen (&) encounters several rendering issues:
1. The information on hidden recipes will start to print over the
recipe category tabs in the header.
2. On particularly narrow screens, the recipe tabs will occasionally
wrap around, partially printing over the header border
3. Subcategory tabs will print over the border of the crafting screen,
and on particularly narrow screens, wrap around to the next line
4. Subcategory tabs that would be printed to the right of a tab
that has wrapped are not printed at all, with no indication that there
are additional subcategories for the player to look at.
@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Apr 15, 2022
@haveric
Copy link
Contributor

haveric commented Apr 15, 2022

First of all, this is fantastic work!

Is there any chance you can make it so the crafting percent doesn't get cut off here?
image

Edit: It also seems to be disappearing sometimes? Is it wrapping to the next line maybe?
image

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 15, 2022
@ZeroInternalReflection
Copy link
Contributor Author

First of all, this is fantastic work!

Is there any chance you can make it so the crafting percent doesn't get cut off here? image

Edit: It also seems to be disappearing sometimes? Is it wrapping to the next line maybe? image

Yeah, I definitely should have prioritized keeping craft speed % visible on minimum window width. It should be pretty easy to do that, though it will depend on what the longest translation of "crafting is slow" is.

For the disappearing crafting speed, I'm pretty sure that's an artifact of how I produced the animation, not a problem in the code. Rather than finding a way to directly record cataclysm like a sensible person, I took a screenshot at each width/position, then assembled it into an animation. Since the crafting speed indicator blinks on and off, some of my screenshots were captured while it blinked off.

For the clang-tidy errors, I'll also be fixing the point_zero error once I get a chance. The select_crafting_recipe length error is a thornier issue, though Qrox's #56812 should alleviate some of the pressure there

The crafting info panel, at minimum window size, was not wide enough
for the display of crafting speed.  This adds a check to ensure that
the minimum width of this panel is enough to display any of the current
language's crafting speed descriptions.
In addition, the crafting info panel was not being properly cleared
when changing recipes, meaning that, .e.g. moving from an item that can
be crafted in the dark to one that cannot would result in the old
"crafting is slow" message being partially visible.
@ZeroInternalReflection
Copy link
Contributor Author

One of the clang-tidy errors should be resolved with this commit. The select_crafting_recipe length error will not be.

I also resolved an issue where I was not properly clearing w_head_info with each recipe, resulting in this:
Before--Improperly_cleared_head_info

The issue of ensuring enough room for "Crafting is slow"-style warnings in the upper right was a bit more complicated than I had hoped. Most languages don't yet translate these strings, but there's a fair bit of variability in length among those that do. I think the Russian translation is the longest, and requires about twice as much room as the English. I briefly implemented a user-settable option for the width of w_head_info, but decided this wasn't something most users would root around in menus looking for, so I've implemented the current system:

  1. Collect all of the strings for crafting speed/ability in one place
  2. Translate them when setting up the crafting screen
  3. Find the longest one in the current language, and use that for the minimum width of w_head_info (We could try to use information on the current recipe, but we would need to recalculate the header on every recipe change)
  4. If the minimum width is less than a quarter of the full width (i.e. the window is fairly wide), give it a bit more room for the less-critical information on how many recipes are in the category

As a result, the space available for category tabs varies a bit between languages:
Dynamic_info_width_RU
Dynamic_info_width_EN
Dynamic_info_width_EN_wide

I think the logic on that is fairly sound, though my current implementation leaves a lot to be desired. There's definitely a way to implement this without adding global variables, but I haven't thought of it yet.

@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 16, 2022
src/crafting_gui.cpp Outdated Show resolved Hide resolved
src/crafting_gui.cpp Outdated Show resolved Hide resolved
src/crafting_gui.cpp Outdated Show resolved Hide resolved
src/crafting_gui.cpp Outdated Show resolved Hide resolved
src/crafting_gui.cpp Outdated Show resolved Hide resolved
src/crafting_gui.cpp Outdated Show resolved Hide resolved
@ZeroInternalReflection
Copy link
Contributor Author

@Qrox I've implemented the changes you've recommended and everything seems to be working. The only reservation I have is the refactor of draw_can_craft_indicator with a loop. I'm not sure what I've done is exactly what you had in mind, but I find the new version to be a much more daunting block of code than the original version.

Also of note is that as far as I can tell, crafting_speed_multiplier(rec) will not, under any reasonable conditions, produce a value <=0.0f (too slow to craft) or >1.0f (crafting is fast), so those messages will not come up in-game (either under the original draw_can_craft_indicator or the new implementation). That's probably something that will need a separate PR, perhaps also addressing #49274.

And clang-tidy has already failed with a complaint about FLT_MAX and FLT_EPSILON. Is there a preferred method for that?

@Qrox
Copy link
Contributor

Qrox commented Apr 17, 2022

I find the new version to be a much more daunting block of code than the original version.

Yeah, I was mainly thinking of getting rid of the string literal keys, in hindsight it's better to just convert the keys to enums like you did.

FLT_MAX and FLT_EPSILON

You can use numeric_limits.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 17, 2022
@ZeroInternalReflection
Copy link
Contributor Author

With this last commit I've moved draw_can_craft_indicator back to a batch of if/else checks rather than the array and loop, since my loop implementation ended up being harder to read.
I think that addresses all of the comments raised/issues noted so far. Clang-tidy still doesn't like select_crafting_recipe, but this isn't the PR to fix that.

Copy link
Member

@dseguin dseguin left a comment

Choose a reason for hiding this comment

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

Merged & tested locally, looks good. The only issue is the complexity limit from clang-tidy for select_crafting_recipe, but as mentioned before that's being fixed separately.

mvwprintz( w, point( pos_x - 2, 1 ), c_light_green, "⁺" );
auto it = flagged_names.find( cat );
cata_assert( it != flagged_names.end() );
( *it ).second += "<color_green>*</color>";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason is changed to * here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...huh.
The new version is a * because I always read the previous version as a *.
If you want a quick PR to change it back to a ⁺, let me know. I have no particular attachment to the *.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll change it back to a for consistency with the subtabs then.

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 [C++] Changes (can be) made in C++. Previously named `Code` 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.

4 participants