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: Transparency option for cost and income indicators #11001

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

zephyris
Copy link
Contributor

Motivation / Problem

The cost and income indicators (the rising red and green text) from vehicle income, construction costs, vehicle purchase, etc. are always visible.

This can become problematic. All other signs/text/labels can be hidden, either by transparency options (invisible signs, vehicle loading indicators) or the settings dropdown (notably town names). As far as I could tell, there was no way to hide the cost and income indicators - therefore added it as a transparency option.

This option is particularly for players who want clean interfaces or beauty screenshots. It is also useful for late game construction where (especially on fast forward) you can end up building in a cloud of green vehicle income text.

Description

This feature adds a show/hide of cost and income indicators as a transparency option, as another transparency toggle button to the transparency control window. When transparent, no cost/income text. When not transparent, normal cost/income text. It toggles along with all other transparency with the normal X transparency hotkey. It can be locked, like the other transparency options, with Ctrl+click.

This implementation imitates handling of the loading/unloading indicators via a transparency button.

Now we can have beautiful screenshots and clean, text free, viewports!

Limitations

I believe this is a fully functional minimal feature, however:

There is no dedicated hotkey. Ctrl+<number> is used for transparency toggling of individual features, but 1-9 are already in use. Ctrl+0 could be used, but I thought was unintuitive.

x toggles transparency of all features including the cost and income indicators. This is a change in behaviour, which might be undesirable? eg. for a new player when building in transparent mode, no cost would be shown.

It is not addressed here, but a setting, similar to the loading/unloading indicators, could be added. Alternatively, cost and income indicator transparency could default to locked.

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, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain TrueBrain added the preview This PR is receiving preview builds label Jun 13, 2023
@DorpsGek DorpsGek temporarily deployed to preview-pr-11001 June 13, 2023 11:46 Inactive
@llugo
Copy link

llugo commented Jun 13, 2023

I like the motivation (having a clean scenery for screenshots), but I wonder if there'll be many players who want to hide income but not loading indicators and vice versa. What about putting these two together in a single setting?

@zephyris
Copy link
Contributor Author

True... Easy to implement as an alternative.

src/texteff.cpp Outdated Show resolved Hide resolved
@DorpsGek DorpsGek temporarily deployed to preview-pr-11001 June 14, 2023 15:57 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-11001 June 15, 2023 14:53 Inactive
rubidium42
rubidium42 previously approved these changes Jun 15, 2023
@LordAro
Copy link
Member

LordAro commented Jun 15, 2023

I agree with @llugo tbh, I don't think this deserves a new button/setting

@2TallTyler 2TallTyler added the work: needs rebase This Pull Request needs a rebase label Jun 27, 2023
@zephyris
Copy link
Contributor Author

On digesting @llugo and @LordAro 's comments, I think I agree... a separate transparency button is overkill. Merge the loading/unloading indicator and cost/income transparency to both use the existing loading/unloading indicator transparency button? Do you think this demands a new icon?

@TrueBrain TrueBrain added preview This PR is receiving preview builds and removed preview This PR is receiving preview builds labels Jul 8, 2023
@TrueBrain TrueBrain added preview This PR is receiving preview builds and removed preview This PR is receiving preview builds labels Jul 8, 2023
@TrueBrain TrueBrain temporarily deployed to preview July 8, 2023 12:15 — with GitHub Actions Inactive
@TrueBrain TrueBrain dismissed rubidium42’s stale review July 14, 2023 12:02

Removing approval as everyone seems to agree this should be one button :)

@2TallTyler 2TallTyler added work: needs more work This Pull Request needs more work before it can be accepted and removed work: needs rebase This Pull Request needs a rebase labels Jul 14, 2023
@zephyris zephyris temporarily deployed to preview August 22, 2023 14:57 — with GitHub Actions Inactive
@2TallTyler 2TallTyler removed the work: needs more work This Pull Request needs more work before it can be accepted label Aug 22, 2023
@zephyris
Copy link
Contributor Author

Simple revision, simplifies the change so that cost/income indicator visibility is toggled with the existing loading indicator visibility button. Changed the tooltip to indicate that it toggles both (and changed the name of the tooltip string and widget to reflect this new functionality). I think the icon of a vehicle is still OK, as cost/income and loading are all strongly vehicle-associated.

Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, just a few minor details for code readability. 😃

src/transparency.h Outdated Show resolved Hide resolved
src/lang/english.txt Outdated Show resolved Hide resolved
src/transparency_gui.cpp Outdated Show resolved Hide resolved
src/widgets/transparency_widget.h Outdated Show resolved Hide resolved
src/texteff.cpp Outdated Show resolved Hide resolved
@zephyris zephyris temporarily deployed to preview August 23, 2023 09:03 — with GitHub Actions Inactive
Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

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

Oof, I'm sorry, my suggestion for handling IsTransparencySet() wasn't quite right. It should return early before the for loop on line 121, rather than pointlessly continuing through the entire loop.

Change that, and I'll approve (as I was about to when I realized my error). 😳

src/texteff.cpp Outdated Show resolved Hide resolved
@glx22 glx22 merged commit 96fdfb9 into OpenTTD:master Aug 25, 2023
20 checks passed
@zephyris zephyris deleted the transparency-costincome branch February 27, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants