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

Fixes for Portuguese Carmageddon #353

Merged
merged 12 commits into from
May 28, 2024
Merged

Fixes for Portuguese Carmageddon #353

merged 12 commits into from
May 28, 2024

Conversation

madebr
Copy link
Collaborator

@madebr madebr commented Dec 19, 2023

The Portuguese Carmageddon segfaulted because its character set caused signed characters to access out-of-bounds memory.

I also added a few fixes for warnings/errors seen in rec2.

Fixes #352

@PierreMarieBaty
Copy link

A possibly related problem: the French version of Carmageddon has text file translations in which accentuated characters over ASCII value 127 are used. This is clearly a mistake from the translator as no corresponding glyphs can represent these characters, but it makes an assertion fail in src/DETHRACE/common/utility.c in the GetALineWithNoPossibleService() function:

    } while (!isalnum(s[0]) // <-- assertion failure
        && s[0] != '-'
        && s[0] != '.'
        && s[0] != '!'
        && s[0] != '&'
        && s[0] != '('
        && s[0] != '\''
        && s[0] != '\"'
        && s[0] >= 0);

When ignoring the assertion, the game runs, but these characters are displayed as blank in the game messages (note that this is consistent with the DOS version).

@madebr
Copy link
Collaborator Author

madebr commented Jan 6, 2024

Using current dethrace master, I don't see glyph related issues with dethrace, using this French game data from archive.org.

(I see a crash on Linux because the audio backend cannot handle audio files with wrong cases)

French Carmageddon provides DATA/SOUND/fyeah1.WAV where it expects DATA/SOUND/FYEAH1.WAV
@madebr
Copy link
Collaborator Author

madebr commented Jan 6, 2024

The last commit fixes the crash due to some French audio files having a wrong casing.

@PierreMarieBaty
Copy link

PierreMarieBaty commented Jan 6, 2024 via email

@madebr
Copy link
Collaborator Author

madebr commented Jan 6, 2024

Oh yes, now I see it. I was expecting a crash and did not read all texts.
Muscle memory knows what buttons to press :)

So yes, this pr will fix these messages.

Screenshot from 2024-01-06 14-41-11

@madebr
Copy link
Collaborator Author

madebr commented Jan 6, 2024

I was wrong, the à in "Retourner à la vie de tous les jours, péniblement calme?" is still missing.

@madebr
Copy link
Collaborator Author

madebr commented Jan 6, 2024

DATA/TRNSLATE contains this:

QUT3STIL,164,34,1,C,L,QUITTER
QUT3STIL,164,80,1,C,L,RETOURNER  LA VIE 
QUT3STIL,164,91,1,C,L,DE TOUS LES JOURS, 
QUT3STIL,164,102,1,C,L,P<C9>NIBLEMENT CALME?
QUT3STIL,116,130,2,C,L,ANNULER
QUT3STIL,213,130,2,C,L,O.K.

(The format is <FLICNAME>,<x>,<y>,<font>,<just>)

So apart from explicitly doing a strcmp, I don't think we can't fix this.
The translators forgot to add the "À".

When I manually modify the file (by adding hexcode 0xc0), it works:
image

@PierreMarieBaty
Copy link

PierreMarieBaty commented Jan 6, 2024 via email

@madebr
Copy link
Collaborator Author

madebr commented Jan 6, 2024

I see the missing whitespace before the colon for the 4th race.
I think it's not too unreasonable for dethrace to require correct input data (as long as it does not crash).
If you want these fixed, I think that works need to be done in a patch set, served by moddb.com or rr2000.

@PierreMarieBaty
Copy link

PierreMarieBaty commented Jan 6, 2024 via email

@madebr
Copy link
Collaborator Author

madebr commented Jan 7, 2024

Printing a warning is not unreasonable, but we need to be careful to not spam the console.
In some places, carmageddon will draw text every frame.

cp = BrScratchString();
int a = BrFileGetLine(cp, 256, df->h);
a = BrFileGetLine(cp, 256, df->h);
(void)a;
Copy link
Owner

Choose a reason for hiding this comment

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

what is this doing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I silences a warning about setting a variable, but not using it.
It's probably better to just remove the a = thing.

@@ -672,6 +672,9 @@ void StripControls(unsigned char* pStr) {
if (pStr[i] < ' ') {
memmove(&pStr[i], &pStr[i + 1], (len - i) * sizeof(char));
len--;
#ifdef DETHRACE_FIX_BUGS
i--;
Copy link
Owner

Choose a reason for hiding this comment

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

what behavior does this fix?

Copy link
Collaborator Author

@madebr madebr Jan 16, 2024

Choose a reason for hiding this comment

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

This fixes StripControls for localized Carmageddon.
e.g. the French version calls this function with the input [NON ATTRIB]\r\n (for the key names in the controls menu).
When you remove the control character at location 12, in the next loop iteration the length of the string is reduced by 1, but you need to also retest the same i index (because it now contains the next character).

Without the fix, this function returns [NON ATTRIB]\n instead of [NON ATTRIB].
This results in the following:
image
Including this fix, the following options menu is displayed:
image

The actual behavior is undefined, since the address sanitizer triggers due to buffer overflow.

madebr and others added 2 commits January 16, 2024 19:49
Co-authored-by: Dethrace Engineering Department <78985374+dethrace-labs@users.noreply.github.com>
@madebr madebr merged commit 62bbf66 into dethrace-labs:main May 28, 2024
5 checks passed
@madebr madebr deleted the fixies branch May 28, 2024 12:59
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.

Improve support for the Brazilian Portuguese version
3 participants