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

Better tooltips for banners and signs #8659

Merged
merged 9 commits into from
May 1, 2019

Conversation

Xkeeper0
Copy link
Contributor

@Xkeeper0 Xkeeper0 commented Jan 31, 2019

Here is an implementation of the request in issue #8593.

  • Adds tooltips to signs that are visible on hover.
    • Scrolling signs that have colored text use the text's color.
    • No entry signs show "No entry".
    • 3D landscape signs and other scrolling signs display their text.
  • Changes some of the localization strings.
    • Addition of a new string for sign tooltips that resets the tooltip formatting.
    • Banner messages that ended with - - have had that trimmed.
      • STR_RIDE_ENTRANCE_NAME, STR_RIDE_ENTRANCE_CLOSED, STR_PARK_CLOSED, STR_NO_ENTRY ...
    • Those banners now use STR_BANNER_TEXT_FORMAT or STR_SCROLLING_SIGN_TEXT to provide the final - -.

One small issue is that new signs that have not been edited yet will show the text "Sign" instead of the default nearby-ride name, but the tile inspector does this as well.


Scrolling sign with colored text
No Entry sign
3D sign

@Gymnasiast Gymnasiast self-requested a review February 1, 2019 08:37
data/language/en-GB.txt Outdated Show resolved Hide resolved
@Gymnasiast
Copy link
Member

The CI fails because of the formatting. Specifically, the following changes should be made: https://travis-ci.org/OpenRCT2/OpenRCT2/jobs/487190950

Formatting can be automatically done via the clang-format utility, or by git adding files, then doing git clang-format, followed by another git add.

@ZehMatt
Copy link
Member

ZehMatt commented Feb 1, 2019

I do appreciate the banner text on hovering them but not a big fan of using the banner colors, I think it would be enough to know what the banner text is.

@Gymnasiast
Copy link
Member

@ZehMatt That would require stripping formatting codes from the banner text. I don't think it's worth that. And personally, I do like the presence of the text colour.

@ZehMatt
Copy link
Member

ZehMatt commented Feb 1, 2019

They are not too bad but when you hover over a banner with red text for like a short time during scroll/zoom you can potentially think that you just saw an error. When it comes to text I would a maximum have 3 colors that I use which is different when it comes to real world banners. How hard would it be to just skip color formatting for a specific string.

@Gymnasiast
Copy link
Member

@ZehMatt Probably harder than you think. The banner text is saved in a stringid with the formatting code baked in. It requires copying and processing the string. All of this because of what I consider to be a cosmetic thing.

@ZehMatt
Copy link
Member

ZehMatt commented Feb 1, 2019

I think that is not correct, the banner stores only the raw string id, Banner.Paint combines that string with another to the color.
See here: https://github.com/OpenRCT2/OpenRCT2/blob/develop/src/openrct2/paint/tile_element/Paint.Banner.cpp#L101

@Ryder17z
Copy link

Ryder17z commented Feb 1, 2019

Either what @Gymnasiast said, or duplicate strings (one formatted, one not formatted) for each sign. Neither is really worth it I'd say.

@Gymnasiast
Copy link
Member

@ZehMatt That's not what the code says. It always combines with string 1731, which is:

STR_1731    :{WHITE}{STRINGID} - - 

This combines the string-with-colour with the - -.

Since I have done more stuff with banners, I'm very sure it saves the colour formatting tag in the stringid itself.

@ZehMatt
Copy link
Member

ZehMatt commented Feb 1, 2019

You are right, for some reason it always puts the final formatted string there rather than keeping string and colours as separate data. Well then it can't be helped I guess.

@jensj12
Copy link
Contributor

jensj12 commented Feb 1, 2019

As far as I know, this is the only map tooltip with 3 lines. Does it work properly with the bottom toolbar enabled (RCT1 style)?

@Xkeeper0
Copy link
Contributor Author

Xkeeper0 commented Feb 1, 2019

Thank you for the feedback! I'm going to work on implementing the suggestions here and should have an update Soon™, hopefully.


As far as I know, this is the only map tooltip with 3 lines. Does it work properly with the bottom toolbar enabled (RCT1 style)?

I had checked it before submitting it (one of my roommates uses the bottom toolbar, thankfully) and it ... almost works. The text does fit (though it is a tight squeeze). But the expanded text uses tooltip formatting, which doesn't match the bottom toolbar. (It's also missing the outline...)

image

This seems to be because openrct2-ui/windows/MapTooltip.cpp:168 uses STR_MAP_TOOLTIP_STRINGID, but openrct2-ui/windows/GameBottomToolbar.cpp:721 handles drawing the tooltip itself using STR_STRINGID. I'm not sure if there's an easy way to say "reset to the default tooltip styling" in formatting without having to check against UITHEME_FLAG_USE_FULL_BOTTOM_TOOLBAR every time, though.


They are not too bad but when you hover over a banner with red text for like a short time during scroll/zoom you can potentially think that you just saw an error. When it comes to text I would a maximum have 3 colors that I use which is different when it comes to real world banners. How hard would it be to just skip color formatting for a specific string.

For normal tooltips, you have to hover over them for a short period of time before the tooltip displays. For the bottom toolbar, though, that is true.


The CI fails because of the formatting. Specifically, the following changes should be made: https://travis-ci.org/OpenRCT2/OpenRCT2/jobs/487190950

Formatting can be automatically done via the clang-format utility, or by git adding files, then doing git clang-format, followed by another git add.

I knew it had a good chance of failing the code format tool. At the time, I didn't have clang-format handy. I'm going to follow this advice and push out an update to fix the formatting :)


I did encounter an issue with some testing last night. Some rides, apparently most tracked ones, suffer an amusing bug in that when the ride is unnamed, the entry hut banner shows "(Ride Type) #", except instead of being the auto-incremented number, it's a rapidly-incrementing number related to the scrolling of the text. I suspect it's something I may have fouled up with openrct2/paint/tile_element/Paint.Entrance.cpp:169, but I'm not immediately sure how (nor why it only seems to break for certain ride types). The queue banners display the names as expected.

@janisozaur
Copy link
Member

@Xkeeper0 thanks for your contributions, but it appears you have to rebase in order to resolve conflicts with translations.

@Xkeeper0
Copy link
Contributor Author

Xkeeper0 commented Feb 4, 2019

@Xkeeper0 thanks for your contributions, but it appears you have to rebase in order to resolve conflicts with translations.

I used Github's web UI to resolve the conflict by adjusting the numbers I used.

@janisozaur
Copy link
Member

@Gymnasiast is it ok now?

Copy link
Member

@AaronVanGeffen AaronVanGeffen left a comment

Choose a reason for hiding this comment

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

You accidentally stripped whitespace in the en-GB file.

@Xkeeper0
Copy link
Contributor Author

Xkeeper0 commented Feb 7, 2019

You accidentally stripped whitespace in the en-GB file.

Can you clarify where the whitespace was stripped? Github's "See review" doesn't seem to show any comments or changes and when I checked with diff, I didn't see any unintentional whitespace deletions:

image

In this case, the change of - - after Closed, No entry etc. is "fixed" by recycling the generic banner formats STR_BANNER_TEXT_FORMAT or STR_SCROLLING_SIGN_TEXT, which include the spaces themselves.

@AaronVanGeffen AaronVanGeffen dismissed their stale review February 7, 2019 09:18

Strings were changed intentionally

@AaronVanGeffen
Copy link
Member

@Xkeeper0 Thanks for pointing that out, I missed that bit!

re: OpenRCT2#8593

These strings use high IDs right now and are probably
not entirely suited for direct inclusion. There is also
some duplication in that "No entry" had to be unique,
as the actual no entry string is "No entry - -".
re: OpenRCT2#8593

Modifies the code for tooltip displays to show the
string assigned to a banner. It also uses the
color assigned to the sign (by sheer coincidence).

As of right now I do not think that it works for
non-banner signs (such as the 3D landscapes or
scrolling signs etc), but it works for the typical
case of banners, as well as "No entry" banners.
Changes some things to use STR_BANNER_TEXT_FORMAT and removes
instances of " - - " attached to other strings in the
localization files.

This can be used in the future to show the messages on
a sign or banner via a tooltip without having to
duplicate those messages without " - - ".
Changes the number of the sign tooltip string
and removes the now-duplicate "no entry" string
For some reason, all signs report "Sign" as their text
before actually being modified. This also happens with the
tile inspector, so for now I'm not terribly worried about it
This fixes the problem where the ride entrance hut
banner would show a nonsense number after auto-named
rides based on the amount the text had scrolled.
@AaronVanGeffen AaronVanGeffen merged commit 6098c1c into OpenRCT2:develop May 1, 2019
AaronVanGeffen added a commit that referenced this pull request Jul 10, 2019
- Feature: [#485] Rides can now be simulated with ghost trains during construction.
- Feature: [#1260] Option for making giant screenshots have a transparent background.
- Feature: [#2339] Find local servers automatically when fetching servers.
- Feature: [#7296] Allow assigning a keyboard shortcut for the scenery picker.
- Feature: [#8029] Add the Hungarian Forint (HUF) to the list of available currencies.
- Feature: [#8481] Multi-threaded rendering.
- Feature: [#8558] Guest debugging tab.
- Feature: [#8659] Banner and sign texts are now shown in tooltips.
- Feature: [#8687] New multiplayer toolbar icon showing network status with reconnect option.
- Feature: [#8791] Improved tile element flag manipulation in Tile Inspector.
- Feature: [#8919] Allow setting ride price from console.
- Feature: [#8963] Add missing Czech letters to sprite font, use sprite font for Czech.
- Feature: [#9154] Change map toolbar icon with current viewport rotation.
- Change: [#7877] Files are now sorted in logical rather than dictionary order.
- Change: [#8427] Ghost elements now show up as white on the mini-map.
- Change: [#8688] Move common actions from debug menu into cheats menu.
- Change: [#9428] Increase maximum height of the Hypercoaster to RCT1 limits.
- Fix: [#2294] Clients crashing the server with invalid object selection.
- Fix: [#4568, #5896] Incorrect fences removed when building a tracked ride through
- Fix: [#5103] OpenGL: ride track preview not rendered.
- Fix: [#5889] Giant screenshot does not work while using OpenGL renderer.
- Fix: [#5579] Network desync immediately after connecting.
- Fix: [#5893] Looking at guest window tabs other than the main tab eventually causes assertion.
- Fix: [#5905] Urban Park merry-go-round has entrance and exit swapped (original bug).
- Fix: [#6006] Objects higher than 6 metres are considered trees (original bug).
- Fix: [#7039] Map window not rendering properly when using OpenGL.
- Fix: [#7045] Theme window's colour pickers not drawn properly on OpenGL.
- Fix: [#7323] Tunnel entrances not rendering in 'highlight path issues' mode if they have benches inside.
- Fix: [#7729] Money Input Prompt breaks on certain values.
- Fix: [#7884] Unfinished preserved rides can be demolished with quick demolish.
- Fix: [#7913] RCT1/RCT2 title sequence timing is off.
- Fix: [#7700, #8079, #8969] Crash when unloading buggy custom rides.
- Fix: [#7829] Rotated information kiosk can cause 'unreachable' messages.
- Fix: [#7878] Scroll shortcut keys ignore SHIFT/CTRL/ALT modifiers.
- Fix: [#8219] Faulty folder recreation in "save" folder.
- Fix: [#8480, #8535] Crash when mirroring track design.
- Fix: [#8507] Incorrect change in vehicle rolling direction.
- Fix: [#8537] Imported RCT1 rides/shops are all numbered 1.
- Fix: [#8553] Scenery removal tool removes fences and paths while paused.
- Fix: [#8598] Taking screenshots fails with some park names.
- Fix: [#8602] Wall piece collision detection deviates from vanilla
- Fix: [#8649] Setting date does not work in multiplayer.
- Fix: [#8873] Potential crash when placing footpaths.
- Fix: [#8882] Submarine Ride does not count as indoors (original bug).
- Fix: [#8900] Peep tracking is not synchronized.
- Fix: [#8909] Potential crash when invoking game actions as server.
- Fix: [#8947] Detection of AVX2 support.
- Fix: [#8988] Character sprite lookup noticeably slows down drawing.
- Fix: [#9000] Show correct error message if not enough money available.
- Fix: [#9067] Land/water tools show prices when money is disabled.
- Fix: [#9124] Disconnected clients can crash the server.
- Fix: [#9132] System file browser cannot open SV4 files.
- Fix: [#9152] Spectators can modify ride colours.
- Fix: [#9202] Artefacts show when changing ride type as client or using in-game console.
- Fix: [#9240] Crash when passing directory instead of save file.
- Fix: [#9245] Headless servers apply Discord Rich Presence.
- Fix: [#9293] Issue with the native load/save dialog.
- Fix: [#9322] Peep crashing the game trying to find a ride to look at.
- Fix: [#9324] Crash trying to remove invalid footpath scenery.
- Fix: [#9402] Ad campaigns disappear when you save and load the game.
- Fix: [#9411] Ad campaigns end too soon.
- Fix: [#9476] Running `simulate` command on park yields `Completed: (null)`.
- Fix: [#9520] Time Twister object artdec29 conversion problem.
- Fix: Guests eating popcorn are drawn as if they're eating pizza.
- Fix: The arbitrary ride type and vehicle dropdown lists are ordered case-sensitively.
- Improved: [#6116] Expose colour scheme for track elements in the tile inspector.
- Improved: Allow the use of numpad enter key for console and chat.
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