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

Colored messages #7471

Merged
merged 17 commits into from May 4, 2014

Conversation

Projects
None yet
8 participants
@Wuzzy2
Copy link
Contributor

commented Apr 29, 2014

This huge pull request adds message types. Different message types a colored differently in the message section of the sidebar and in the message log. Instead of new messages being red and a bit old messages being dark gray, new and and a bit older messages have a color depending on the mssage type. The aim is to improve usability.
Screenshot: https://i.imgur.com/HfawNIj.png

It follows a list of used colors and their meanings.
First color is for fresh messages, second color is for slightly older messages. Old messages are always dark gray.

  • white/light gray: Neutral event / no effect or only slight effect / non-critical failure of an action. This message type is the default one.
  • light blue/blue: informational message (eg. usage description, tips, examination result)
  • light green/green: Positive effect, something good happened. (Death of hostile enemy, good mutation, …)
  • light yellow/brown: Warning, possible danger (eg. enemy spotted)
  • light red/red: Something bad effect happened (eg. take damage)
  • pink/magenta: Positive/bad effect mixed happened. (eg. gain a mutation with mixed effect)

Unused colors:

  • light cyan
  • cyan

(If you have an idea for one more message type, I would happily include it, using these colors, but in a different pull request)

Here are the most important parts of this PR:

  1. Adding the enum game_message_type for the message types. It is documented in messages.h. It is listed in ENUMS.md.
  2. Overloading add_msg (and friends in messages.cpp) with a new parameter type, of type game_message_type.
  3. Overloading its wrapper (?) functions: add_msg_if_player, add_msg_if_npc, add_msg_player_or_npc.
  4. Going through every call of the add_msg (and wrappers) and assigning them a message type. This is the part which took the most work.
  5. Special treatment for mutations.

add_msg has been intentionally overloaded, not rewritten. The old function without the type parameter still exists. The message will be then of the neutral type (m_neutral). This has been done for convenience reasons so you don’t have to supply the type every time you call add_msg.

The special treatment for mutations works as this:
Determination of the proper message type if you just lose or gain a mutation is easy. If you gain a mutation, you get a “good” message if the mutation is good, a “bad” message if the mutation is bad, a “mixed” message if the mutation has mixed_effect. Neutral mutations produce a "neutral" message. If you lose a mutation, good and bad messages are reversed. This is straightforward.
The tricky part was to assign a message type if a mutation gets repaced. I have done it this way: Above all, if one of both mutations are mixed, the message is mixed. Period. If not, the points of the replacing mutation minus the points of the replaced mutation are compared. If the replacing mutation has more points, the message is positive, if it has less points, the message is negative. Otherwise it is neutral.
This behavior is different again if a natural trait is being replaced. Here the message is positive iff the replaced trait is bad and the replacing trait is good. This vise-versa produces a negative message. The message is neutral iff both traints are neutral, else the message is mixed. The two traits may be simply too different to surely be able to say “this is good/bad”, so better “play it safe” by using a “mixed” message more likely.
These methods to get the message type from mutation changes may seem strange, but so far they seem to work. But if you can find an example where the message color does not really fit the change, please say so!

There is one part of messages which is currently not coverd: Effects. This part seems to be in flux right now (probably port to JSON ongoing?), so I won’t touch this for now. Currently, all effects from JSON produce "neutral" messages, which is of course not always appropriate. You also see this in the screenshot (“You're blinded!”). I will try to tackle this after this PR is merged (it is big enough already!).

I took care I did not accidentally change any strings, but I have split a very few strings for style purposes, the additional load on translators is minimal.

@BevapDin

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2014

Nice work. Going through all those add_msg calls... awesome.

Some nitpicking:

Why did you create so many add_msg(m_neutral, ... calls, instead of leaving them without type, as m_neutral is the default anyway?

Messages::add_msg_string(const std::string &s) should simply call the other add_msg_string with the neutral type. There are several pointlessly duplicated lines of code.


Btw. git rebase -i can be used to resort/combine commits, this can get you rid of commits that simply fix an error in previous commits. That's how I hide all my stupid mistakes before opening a PR (-:

Also this branch does not have some of the recent changes like f1f2ae0 and e192341 - another thing that rebase can be used for - rebase is great tool.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Apr 29, 2014

I've only had a chance to read the description so far, but this sounds like
it will be a great enhancement to the game.

Wuzzy
@Wuzzy2

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2014

@BevapDin: Actually, there are not so many m_neutrals explicitly written. m_info, m_bad and m_good are much more common. And those few m_neutrals This is intentional. This is simply a matter of style. I used it when I wanted to emphasize this message is considered neutral and not that it looks like I forgot something.

I fixed that stupid redundant code for add_msg_string. I forgot to clean that part up.

I’ll look into rebase and the other redundant code at sometime.

@MattAmmann

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2014

This will help with immersion in a big way. It was a lot of work...nice job! 👍

@Zireael07

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2014

Wow, great!

@kevingranade

This comment has been minimized.

Copy link
Member

commented Apr 29, 2014

Always using m_neutral is good in that it makes it easier to find if
someone omits it entirely, we might just deprecate the version that doesn't
take a color modifier at some point.

@KA101

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2014

May as well let people configure their colors; only objections I've got are potential colorblindness issues and having my message log get garish.

But then I didn't think there needed to be a "mixed effect" mutation color, either. (Still don't.)

@Lain-

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2014

I like it.

@KA101 I think this will be better than the old scheme even for color-blind people, as the red-gray messages we're using now can be very problematic for people with red-green blindness. http://www.color-blindness.com/protanopia-red-green-color-blindness/
With mor variety in colors, the chance that a person cannot distingish between the used colors at all will be lower.
Of course it would be nice if such color can be customized so that people can tweak the exact color to their liking.

@Wishbringer

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2014

This is similar to @BevapDin unPR'ed message coloring that I've been using.
The good thing with this is that it's used across the code-base (fully implemented).
It would be great if the best ideas and coding solutions could be used from both implementations.

@@ -11183,7 +11183,7 @@ void game::complete_butcher(int index)
if( corpse->has_flag(MF_CBM_CIV) ) {
//As long as the factor is above -4 (the sinew cutoff), you will be able to extract cbms
if( skill_shift >= 0 ) {
add_msg(_("You discover a CBM in the %s!"), corpse->name.c_str());
add_msg(m_good, ("You discover a CBM in the %s!"), corpse->name.c_str());

This comment has been minimized.

Copy link
@kevingranade

kevingranade Apr 30, 2014

Member

lost the translation macro.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Apr 30, 2014

I'd actually prefer just to make the message type required and get rid of the old methods (or augment the old methods with the new parameter, whichever).
However I think that and configurable colors are both good fodder for a follow-up PR, this is good to go IMO (but I don't have time to merge today).

Wuzzy
@BevapDin

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2014

@Wishbringer My code was terrible incomplete, I'm just to lazy to go through all the add_msg calls and change them (and most of the time I would apply the wrong message type) - Wuzzy2 has put a lot of good work into that. And using an enum is actually better than what I did.

Loading color definitions from json is not difficult, an example is the ammo types to json: 3cd7ed6. Someone will do it, once this got merged.

@Wuzzy2

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2014

Damn it, sorry, people. I tried a rebase and then I totally screwed up my local reposity and I find myself unable to recover. I cloned my repo again from GitHub, now rebase requires me to go through 800 commits, most of them with conflicts in files I never touched. :-( It looks like it takes some time until I set up everything again. (I have to be more careful with git! Damn. Forgive me my clumsiness)
Unless I manage to set my environment up again, I guess you have to do the merge work alone, without my help. (Well, you may still ask me in IRC if you get stuck on a conflict.) Also I probably won’t make any changes to this PR, the remaining issues could all be tackled in future PRs.
If you want this to merge, better do it soon, as this PR touches basically everything, it will attract conflicts like a magnet … :-(
So sorry …

@kevingranade

This comment has been minimized.

Copy link
Member

commented May 1, 2014

I was strongly leaning toward merging soon and fixing up any issues anyway,
sorry to hear about your trouble. (I've nuked a number of repos too, it's
a pain)

@kevingranade kevingranade merged commit c279617 into CleverRaven:master May 4, 2014

1 check failed

default Unmergeable pull request.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.