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

Option to yell sentences #21495

Merged
merged 3 commits into from
Aug 22, 2017
Merged

Conversation

AlecWhite
Copy link
Contributor

@AlecWhite AlecWhite commented Jul 27, 2017

Asked around in the IRC channel and Discord server if people would like to be able to yell sentence for the role-playing flavour, and they liked it so I decided to make this.

Now you can yell whatever you want at the moose.

  • Added "yell sentence" option in chat menu.
  • Sentence can be 128 character long and will produce sound.

Had to re-made the if/else which I don't know if I did it right, so I could use some help reviewing that part please.

Also I couldn't decided on how to display the yelled sentence, if either after colon, if forced upper case or between quotation marks. So I left it without any kind of fancy styling.

src/game.cpp Outdated
available[nmenu.ret]->talk_to_u();
} else {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Bad indent.

@DangerNoodle
Copy link
Contributor

I can see this being amusing.

src/game.cpp Outdated
@@ -10872,24 +10872,45 @@ void game::chat()
}

uimenu nmenu;
nmenu.text = std::string( _("Who do you want to talk to?") );

nmenu.text = std::string( _( "Who do you want to talk to or yell?" ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be: "Who do you want to talk to or yell at?"

Otherwise, this reads (without the first clause): "Who do you want to yell?"

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't checked what the actual UI looks like, but you may not even need to add the "Yell" clause to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That text is displayed whether there are NPCs or not, we could add a check to it, but it would automagically tell the player if there are NPCs around.

src/game.cpp Outdated

nmenu.addentry( yell = i++, true, 'a', _( "Yell" ) );
nmenu.addentry( yell_sentence = i++, true, 'b', _( "Yell a sentence" ) );
nmenu.addentry( cancel = i++, true, 'q', _( "Cancel" ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you've touched this line, could you remove it and update the condition below (see #12112)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, but I don't know exactly what you want me to do, mind explaining it please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Most of our menus are escapable i.e. they can be closed using Esc button, which makes options like "Cancel" redundant. You can simply remove it to make this particular menu more consistent with the others.

src/game.cpp Outdated

int yell, yell_sentence, cancel;

nmenu.addentry( yell = i++, true, 'a', _( "Yell" ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of assignment is ugly and confusing. An enum could have been used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I googled around if you can assign an enum's value can be assigned when called, and it seems that they are constant expression. So I don't know how enums can be used in this case.

I could comment more all the code, to it is at least less confusing, but between that I'm a noob at programming and that this method didn't have much comments, I don't really know when to comment and how much to comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

and it seems that they are constant expression.

They certainly are. I meant getting rid of any assignments entirely. See this function as a good example of what I'm talking about. You can use negative values to avoid overlapping with indexes of NPCs:

    enum class choices {
        just_yell = INT_MIN,
        yell_something,
        ...
    };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using enum class doesn't seems to work (had to I cast it to int for ui.addentry() to accept it) which for some reason it takes it as zero and one. Using non-class enums works only for the first enum but not for the second as it is taken as 1.

I'm manually assigning them both to -2 and -1 respectively. And it seems that the addentry() and query() both seems to support negative integers.

Any advise?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are negative numbers sorted after non-negative by the game's engine? If no, what you propose won't work.

There is no sorting done by the "game engine" (what do you actually mean with that?) at all. The menu displays the options in the order they have been added (actually: the order in which they appear in uimenu::entries).

@kevingranade kevingranade merged commit 16767cb into CleverRaven:master Aug 22, 2017
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.

7 participants