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

Feature: Show cargo icon on cargo filter lists. #11487

Merged
merged 5 commits into from Dec 2, 2023

Conversation

PeterN
Copy link
Member

@PeterN PeterN commented Nov 23, 2023

Motivation / Problem

Cargo icons are only visible in the station view window, when stations have waiting cargo. Painstakingly drawn 10 pixel high pixel art masterpieces are being denied to us.

Description

Add cargo icons to cargo filter dropdown lists, and revel in beauty of them.

Cargo icons have various widths so we have a function to get the largest and make sure all items use the same width.
Cargo icons have various widths so they are drawn at the end of the item. Other positions were tested and were less visually coherent.

This also introduces the CRTP (Curiously Recursive Template Pattern) redesign for drop down list items, which allows them to be composed of various parts instead of to a fixed design, but unfortunately means they are templates to are implemented in the head filters.

https://github.com/OpenTTD/OpenTTD/assets/639850/00fe6c55-118b-4eff-86a5-38de324e123e

image

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')


/* Commonly used drop down list items. */
using DropDownListDividerItem = DropDownDivider<DropDownListItem>;
using DropDownListStringItem = DropDownString<DropDownDivider<DropDownListItem>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't fully get why DropDownDivider as base of DropDownString

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the old base DropDownListItem provided the divider itself. The alternative is to undo #11447, and explicitly create a DropDownListDividerItem. That may not be too much hassle now.

@DefinitelyNotRau117
Copy link

Good idea, it will really improve the clarity of filter lists!
But on the “very wide” windows a problem may arise with “fictitiously misalighed” icons when placing icons after text. To be honest, I didn’t even notice these icons on your screenshot…
изображение
What about placing an icon before the text? This will help to visually connect “icon-cargoname” pairs without having to refocus to the other end of the window.
изображение

@2TallTyler
Copy link
Member

We discussed icons before the text on Discord. It didn't look great because icons are all different sizes, so their alignment with text looks bad.

I'd rather see the dropdown clamped to a suitable width (using a spacer to fill the empty space) to avoid the problem. 🙂

@PeterN
Copy link
Member Author

PeterN commented Nov 24, 2023

I wasn't making this up...

Cargo icons have various widths so they are drawn at the end of the item. Other positions were tested and were less visually coherent.

https://github.com/OpenTTD/OpenTTD/assets/639850/7d09d6bd-3505-4d04-a26e-7f305ed03a9b

@planetmaker
Copy link
Contributor

Arguably the icons don't match well in width. However that's a task for the (New)GRF authors to take care of. The list with the icons on the left still looks better to me than with the icons on the right with the huge separation from the text.

Could the maximum size of the icons be chosen as icon column width and all icons be centered wrt the icon column instead of left-aligned to make it look better?

@PeterN
Copy link
Member Author

PeterN commented Nov 25, 2023

At the end of the day, I prefer it on the right.

@LordAro
Copy link
Member

LordAro commented Nov 25, 2023

Add a setting ;)

But I do agree that slightly less of a gap would be better

@PeterN
Copy link
Member Author

PeterN commented Nov 25, 2023

image

@PeterN
Copy link
Member Author

PeterN commented Nov 25, 2023

And with a checkmark, which is also on the left to match the menu items on the settings toolbar menu:

https://github.com/OpenTTD/OpenTTD/assets/639850/6d49a92e-ace2-42cf-a46b-e47e19c9a53e

Default parameters allowed Dimension to be constructed with only a width.

Instead use separate empty and width/height constructors to ensure that either none or both are provided.
This allows list items to built from component parts as required, and additional
functionality is added:

* Icons and text can be positioned at the start or end of the space (templated.)
* Font size of text can be changed (templated.)
* Palette of sprites can be set (runtime.)
This avoids the need to construct a DropDownIcon and set the dimension after.
@PeterN
Copy link
Member Author

PeterN commented Nov 25, 2023

Okay, now changed to put the icon on the left, along with a few other changes to make that easier to perform.

First commit is because I ran into an issue where any integer could be converted to a Dimension of height 0 unintentionally...

Dimension dim; ///< Dimensions of string.
public:
template <typename... Args>
explicit DropDownString(StringID string, Args&&... args) : TBase(std::forward<Args>(args)...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, nice idea/trick when using CRTP.

@PeterN PeterN merged commit 1aedea8 into OpenTTD:master Dec 2, 2023
20 checks passed
@PeterN PeterN deleted the cargo-filter-cargo-icon branch December 2, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants