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

Overhaul look around panels #39793

Merged
merged 11 commits into from
May 25, 2020
Merged

Overhaul look around panels #39793

merged 11 commits into from
May 25, 2020

Conversation

ymber
Copy link
Member

@ymber ymber commented Apr 21, 2020

Summary

SUMMARY: Interface "Overhaul look around panels"

Purpose of change

Shows more information in look around menus and makes it easier to read at a glance.

Describe the solution

Hijack code from #36324.

Testing

Panels render as expected.

Additional context

This is the same as where #36324 left off with some minor code changes. Still need to make it format right for translations.

@ymber ymber added <Enhancement / Feature> New features, or enhancements on existing [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. Translation I18n labels Apr 21, 2020
@esotericist
Copy link
Contributor

As I said in the previous PR, I'd like this to be gated by an option because a lot of the choices here are deleterious to how I process information.

long term, what we want is for this sort of thing to be json-driven, but that's a much bigger project.

@ymber
Copy link
Member Author

ymber commented Apr 22, 2020

This should be alright for translation formatting now. Some changes also to improve UX.

@Night-Pryanik
Copy link
Contributor

@ymber could you please provide some screenshots with your rework?

@ymber
Copy link
Member Author

ymber commented Apr 23, 2020

2020-04-23_1041_04
2020-04-23_1041_08
2020-04-23_1041_22
2020-04-23_1154_20
Everywhere that has the Label: Value pattern the second string is printed one space after the translated label so it should be adaptive enough.

Copy link
Contributor

@Night-Pryanik Night-Pryanik left a comment

Choose a reason for hiding this comment

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

Looks good as far as I can tell.

Though I'd like to see the most complex info in one picture: terrain, furniture, trap, field, vehicle, items, monster/npc.

@ymber
Copy link
Member Author

ymber commented Apr 24, 2020

2020-04-24_2212_06
2020-04-24_2216_06
2020-04-24_2220_09
2020-04-24_2217_41
Some more complex panels with a load of stuff on the same tile. I think the last one is showing bugs with the trap information not ending with a newline and the health bar for the zombie not showing at extremely low health.

@Night-Pryanik
Copy link
Contributor

Cool, thanks. LGTM.

@CleverRaven CleverRaven deleted a comment from captnblood Apr 25, 2020
@CleverRaven CleverRaven deleted a comment from ZhilkinSerg Apr 25, 2020
@CleverRaven CleverRaven deleted a comment from captnblood Apr 25, 2020
@wapcaplet
Copy link
Contributor

wapcaplet commented Apr 25, 2020

I had trouble seeing the differences from the current version to this one, so I took a couple screenshots with a monster and NPC. On the left is the master branch as of 889f657, and on the right is the new panel as of 337b496

Monster:
image

NPC:
image

Overall the changes look fairly subtle. I like the white highlight for the terrain (especially since it is listed at the top), and the red highlight for monster awareness. The addition of a health bar is definitely good, and the blue colors for NPC items was kind of distracting, so grey text makes sense there.

@ymber Could the "Lighting:" label be restored, instead of just having the word "bright" (or "dark" etc.)? This would be more consistent with the new labels. Also, the pink color-coding for the NPC was a useful highlight to distinguish them from monsters - would you consider restoring as well?

My only other concern is the increased vertical space requirement. NPC attitude ("Neutral") could perhaps go on the same line with "Aware of your presence" to save a line.

@esotericist
Copy link
Contributor

So, while we're tampering with this, I'd like to bring up something that came up in discord recently
specifically: "Aware of your presence/Knows you are there"

this line is actually a complete lie. The logic behind it is not actually keying off of whether the critter/character is aware of you, but whether or not the critter/character has vision capability to the tile you're in. Notably, when X peeking, it can show "Aware of your presence" because you've moved to a tile they have line of sight to, even if they can't actually see you.

We might want to pursue phrasing along the lines of: "Can see to your current location". I also agree with wapcaplet about being interested in combining it with the hostility indicator, although that's tricky with longer verbiage.

Another thing we might consider adding at the bottom: terrain/furniture descriptions. We have these in-game, but they are exceedingly unobvious to get at so most people don't know they exist (which contributes to many of them being incorrect, since they're never seen). They're comparatively unimportant, so shoving them at the bottom keeps them from eating space that belongs to higher priority things.

I have other desires, but I think they fall into "highly subjective bits, best reserved for hypothetical customization". I think aside from what's been mentioned above, this should be fine.

@esotericist
Copy link
Contributor

Oh, one thing:
Can you pad the critter health with . so it has a consistent width?

@ymber
Copy link
Member Author

ymber commented Apr 26, 2020

Restored lighting label and reworded the awareness indicator. When AI map awareness gets reworked at some point in the future we should revisit how we communicate monster awareness to the player. I added dot padding to the health bars in this panel and in the shift+v panel while I was on.

I was considering making monster and NPC names the same color as their symbols. This is the way the shift+v menu went and it seems like the best solution if white names for all monsters and NPCs makes them harder to distinguish at a glance.

I think putting furniture and terrain descriptions under everything else would be bad for how information is clustered. Given how much screen space they'd take the way I'd go for that would be having an extended descriptions key in the panel that shows all that. If that's added we might want to hide monster descriptions behind it as well.

@Zireael07
Copy link
Contributor

I was considering making monster and NPC names the same color as their symbols.

YES PLEASE.

src/game.cpp Outdated Show resolved Hide resolved
@ymber
Copy link
Member Author

ymber commented May 5, 2020

Fixed terrain and furniture flags disappearing across lines.
2020-05-05_0946_37

@ZhilkinSerg
Copy link
Contributor

astyle regressions found.
Formatted  src/game.cpp Formatted  src/npc.cpp

@ZhilkinSerg ZhilkinSerg self-assigned this May 25, 2020
src/game.cpp Outdated Show resolved Hide resolved
src/game.cpp Outdated Show resolved Hide resolved
src/game.cpp Outdated Show resolved Hide resolved
src/game.cpp Outdated Show resolved Hide resolved
src/game.cpp Outdated Show resolved Hide resolved
@ZhilkinSerg
Copy link
Contributor

Caught one thing with graffiti (not caused by current PR) - https://github.com/CleverRaven/Cataclysm-DDA/pull/30281/files#r429820719. Otherwise looks good for me.

@ZhilkinSerg ZhilkinSerg merged commit 16aba7e into CleverRaven:master May 25, 2020
@ymber ymber deleted the lookaround branch May 25, 2020 10:29
@ZhilkinSerg ZhilkinSerg removed their assignment May 27, 2020
tung pushed a commit to tung/Cataclysm-DDA that referenced this pull request Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` <Enhancement / Feature> New features, or enhancements on existing Info / User Interface Game - player communication, menus, etc. Translation I18n
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants