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

Jsonize species footsteps sound #31493

Merged

Conversation

ZhilkinSerg
Copy link
Contributor

Summary

SUMMARY: Features "Jsonize species footsteps sound"

Purpose of change

Allow definition of monster species footsteps sound via json.

@ZhilkinSerg ZhilkinSerg added Code: Performance Performance boosting code (CPU, memory, etc.) Translation I18n [JSON] Changes (can be) made in JSON [C++] Changes (can be) made in C++. Previously named `Code` Monsters Monsters both friendly and unfriendly. labels Jun 15, 2019
src/mtype.cpp Outdated
for( const species_id &s : species ) {
return s.obj().get_footsteps();
}
return "footstepz.";
Copy link
Member

Choose a reason for hiding this comment

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

Typo? (footstepz instead of footsteps)

@@ -790,6 +790,8 @@ void MonsterGenerator::load_species( JsonObject &jo, const std::string &src )

void species_type::load( JsonObject &jo, const std::string & )
{
optional( jo, was_loaded, "footsteps", footsteps, "footsteps." );
footsteps = _( footsteps );
Copy link
Contributor

Choose a reason for hiding this comment

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

Translation should occur in get_footsteps. Changing the language midway through a game will not change the stored (and already translated) string, so the footsteps will still be in the language that was selected upon loading the game.

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 is intentional - translation calls inside of footsteps function are very expensive.

Copy link
Member

Choose a reason for hiding this comment

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

It's possible to detect language changes and swap out this value, we can look into doing that in a future PR.

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 was thinking whether it is possible not to make rather costly translation calls in a runtime, but instead do translation on json data loading (possibly with data/translation reloading when language is changed or simply requiring game restart on language change).

I don't think that changing language during running game is a frequent use-case. It is really cheaper to select language and reload game then doing thousands of calls to get the same footstep string.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think translation calls are expensive. After a simple benchmark, 10 samples without I18N and another 10 samples with I18N, it seems a single gettext call only costs extra 0.32us on a pretty old Core Duo laptop.

#include <iostream>
#include <libintl.h>
using namespace std;
const int cnt = 1000*1000;
int main()
{
    setlocale(LC_ALL, "zh_CN");
    bindtextdomain( "cataclysm-dda", "lang/mo" );
    bind_textdomain_codeset( "cataclysm-dda", "UTF-8" );
    textdomain( "cataclysm-dda" );
    for(int i = 0; i < cnt; i++)
#ifdef I18N
        cout << gettext("battery") << endl;
#else
        cout << "battery" << endl;
#endif
    return 0;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are doing various optimizations now and libgettext calls started to appear on top in various scenarios after other things were optimized.

@@ -68,15 +69,18 @@
{
"type": "SPECIES",
"id": "WORM",
"footsteps": "rustle.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Translatable strings in JSON require separate code in "lang/extract_json_strings.py" to extract them for translation.

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've updated Python script - hope everything was correct.

CC: @BrettDong

@ZhilkinSerg ZhilkinSerg force-pushed the jsonize-species-footsteps branch from 208b653 to c7e7322 Compare June 15, 2019 21:39
@kevingranade kevingranade merged commit 717ef27 into CleverRaven:master Jun 17, 2019
@ZhilkinSerg ZhilkinSerg deleted the jsonize-species-footsteps branch June 17, 2019 10:27
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` Code: Performance Performance boosting code (CPU, memory, etc.) [JSON] Changes (can be) made in JSON Monsters Monsters both friendly and unfriendly. Translation I18n
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants