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

XEEN Russian version #3173

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

XEEN Russian version #3173

wants to merge 23 commits into from

Conversation

@Ardash
Copy link

@Ardash Ardash commented Jul 19, 2021

I'm working on Russian version of Clouds of Xeen support.

Copy link
Member

@lephilousophe lephilousophe left a comment

Thanks for the PR, I left some comments where code style could be improved to match our style.
There are changes on rc file which seem bogus.

@@ -24,7 +24,6 @@ scummclassic.zip FILE "gui/themes/scummclassic.zip"
scummmodern.zip FILE "gui/themes/scummmodern.zip"
scummremastered.zip FILE "gui/themes/scummremastered.zip"
residualvm.zip FILE "gui/themes/residualvm.zip"
achievements.dat FILE "dists/engine-data/achievements.dat"

This comment has been minimized.

@lephilousophe

lephilousophe Jul 19, 2021
Member

This should not be removed

This comment has been minimized.

@Ardash

Ardash Jul 29, 2021
Author

Fixed

@@ -108,7 +107,7 @@ ultima.dat FILE "dists/engine-data/ultima.dat"
wintermute.zip FILE "dists/engine-data/wintermute.zip"
#endif
#if PLUGIN_ENABLED_STATIC(XEEN)
xeen.ccs FILE "dists/engine-data/xeen.ccs"
xeen.ccs FILE "../dists/engine-data/xeen.ccs"

This comment has been minimized.

@lephilousophe

lephilousophe Jul 19, 2021
Member

I think this change is not good either

This comment has been minimized.

@Ardash

Ardash Jul 29, 2021
Author

Fixed

int KEY_SHOW_CREDITS;
int KEY_VIEW_ENDGAME;
} CLOUDSOFXEENMENU;
} KEY_CONSTANTS;

This comment has been minimized.

@lephilousophe

lephilousophe Jul 19, 2021
Member

This structure is not well named as it's not constant at all.
All caps names are reserved for constants, which it's not.
It confused me when reading the switch to if conversion above (which makes sense when variable is not constant)

This comment has been minimized.

@Ardash

Ardash Jul 29, 2021
Author

Fixed

for (uint idx = 0; idx < (combat._globalCombat == 2 ? combat._combatParty.size() :
party._activeParty.size()); ++idx) {
Character &c = combat._globalCombat == 2 ? *combat._combatParty[idx] :
party._activeParty[idx];
const char **_tmpConditions = c._sex == FEMALE ? (const char **)Res.CONDITION_NAMES_F : (const char **)Res.CONDITION_NAMES_M;

This comment has been minimized.

@lephilousophe

lephilousophe Jul 19, 2021
Member

underscore prefix is usually for member variables not local ones

This comment has been minimized.

@Ardash

Ardash Jul 29, 2021
Author

Fixed

case CONS_GOLD: return Res.CONSUMABLE_GOLD_FORMS[0];
case CONS_GEMS: return Res.CONSUMABLE_GEM_FORMS[0];
}
return Res.CONSUMABLE_NAMES[consumableId];

This comment has been minimized.

@lephilousophe

lephilousophe Jul 19, 2021
Member

it would be cleaner to add this in a default case above

This comment has been minimized.

@Ardash

Ardash Jul 29, 2021
Author

Fixed

@@ -812,6 +820,18 @@ bool Party::arePacksFull() const {
return total == (_activeParty.size() * NUM_ITEM_CATEGORIES);
}

const char *Party::getFoundForm(const Character &c) {
if (Common::RU_RUS == g_vm->getLanguage()) {

This comment has been minimized.

@lephilousophe

lephilousophe Jul 19, 2021
Member

it could be more readable with

if ((Common::RU_RUS == g_vm->getLanguage()) &&
    (c._sex == FEMALE)){
    return Res.FOUND[1];
} else {
    return Res.FOUND[0];
}

This comment has been minimized.

@Ardash

Ardash Jul 29, 2021
Author

Fixed

@@ -1462,6 +1482,30 @@ bool Party::giveTake(int takeMode, uint takeVal, int giveMode, uint giveVal, int
return false;
}

const char *Party::getPickLockForm(const Character &c) {
if (Common::RU_RUS == g_vm->getLanguage()) {

This comment has been minimized.

@lephilousophe

lephilousophe Jul 19, 2021
Member

Same as above

This comment has been minimized.

@Ardash

Ardash Jul 29, 2021
Author

Fixed

}

const char *Party::getUnablePickLockForm(const Character &c) {
if (Common::RU_RUS == g_vm->getLanguage()) {

This comment has been minimized.

@lephilousophe

lephilousophe Jul 19, 2021
Member

Same as above

This comment has been minimized.

@Ardash

Ardash Jul 29, 2021
Author

Fixed

@@ -85,7 +85,8 @@ void Spells::executeSpell(MagicSpell spellId) {
&Spells::wizardEye
};

(this->*SPELL_LIST[spellId])();
if (spellId < 76)
(this->*SPELL_LIST[spellId])();

This comment has been minimized.

@lephilousophe

lephilousophe Jul 19, 2021
Member

I don't know if an error should be raised here if spellId is out of range

This comment has been minimized.

@dreammaster

dreammaster Jul 29, 2021
Member

Depends. If there's any place in the Russian version where a spell Id >= 76 is passed, it's better to have this as a workaround then to always crash the game

@Ardash
Copy link
Author

@Ardash Ardash commented Jul 21, 2021

@lephilousophe I've commit some fixes

encoding.dat FILE "dists/engine-data/encoding.dat"
achievements.dat FILE "dists/engine-data/achievements.dat"

This comment has been minimized.

@sev-

sev- Jul 27, 2021
Member

It would be better to remove this change from the history, so it is not removed in the first place

This comment has been minimized.

@Ardash

Ardash Jul 30, 2021
Author

Removed

@sev-
Copy link
Member

@sev- sev- commented Jul 27, 2021

Only one change from me and I have no objections for merging this

@sev- sev- requested a review from dreammaster Jul 27, 2021
@dreammaster
Copy link
Member

@dreammaster dreammaster commented Jul 28, 2021

I'll devote some time to review it tomorrow night.

engines/xeen/font.cpp Outdated Show resolved Hide resolved
engines/xeen/font.cpp Outdated Show resolved Hide resolved
public:
class EN_DialogsCharInfo : public DialogsCharInfo {
public:
const int KEY_ITEM() { return Common::KEYCODE_i; }

This comment has been minimized.

@dreammaster

dreammaster Jul 29, 2021
Member

Not sure, but I think some compilers might generate warnings over the redundant use of const for an int return. Unless you meant to do it like "int KEY_ITEM() const", which makes sense since the methods don't have any side-effects.

This comment has been minimized.

@Ardash

Ardash Jul 29, 2021
Author

const removed

@dreammaster
Copy link
Member

@dreammaster dreammaster commented Jul 29, 2021

I've reviewed the pull request and made some comments. I also see that there's still some other issues others have raised that are still outstanding. Overall, good work. Once they've been resolved, I'll be all for having it merged.

@dreammaster
Copy link
Member

@dreammaster dreammaster commented Jul 29, 2021

Also, apologies if I already posted this elsewhere; I can't remember. But the last two commits also need to be interactively rebased to reword the engine prefix to "XEEN:" instead of just "XEEN". You could do it at the same time you do the rebase to remove the removal of the line from the .rc file in the first commit.

@Ardash
Copy link
Author

@Ardash Ardash commented Jul 29, 2021

Also, apologies if I already posted this elsewhere; I can't remember. But the last two commits also need to be interactively rebased to reword the engine prefix to "XEEN:" instead of just "XEEN". You could do it at the same time you do the rebase to remove the removal of the line from the .rc file in the first commit.

Fixed

@Ardash Ardash force-pushed the Ardash:XEEN_RU branch from 056ede3 to c172709 Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants