Add support for the 2021 re-release

[andrei-drexler]

This fixes crashes with the 2021 re-release and adds basic support for localized strings so that the new maps display proper messages instead of text id's. Localization data is read from a loc_english.txt file in the mod directory, in the same format as the files in the localization folder in the rerelease/QuakeEX.kpf zip archive. Currently the files need to be copied manually - adding code to read zip files just for this feature seemed like feature creep, and can be added later if necessary.

@sezero, I'm hoping this can also be merged into QS, can you please have a look?

[sezero]

Haven't tried in vkquake, but tried adapting to qs - see attached patch:

• renames MOVETYPE_EXT_BOUNCEMISSILE to MOVETYPE_GIB
• fixes the unwanted format argument in LOC code I noted
• removes C99'isms
• changes loc file name to localization/loc_english.txt

The illegible server message and bad movetype errors are gone.
However the messages don't seem to be retrieved from the localization file,
i.e. I see $map_skill_easy, $map_start_e1, $map_jump_across and
so on - maybe I made a mistake somewhere when backporting. (Also note
that the localize file is in a zip archive, i.e. no way of extracting it from within
the engine without specifically adding code, which I don't think is worth the
trouble.)

[sezero]

The qs version of your patch: patch_qs.txt

[andrei-drexler]

Thank you!

I think I've implemented all your changes, except for one that I wasn't sure about: I've noticed that the overflow warning in PF_VarString was moved to the legacy concatenation code path (the else block of if (localized)), whereas in my code it was triggered in both cases, since I figured we're generating a large string that would have overflowed WinQuake's buffer either way. It's a minor point, but I wanted to confirm it's intentional.

I've also tested the QS patch on a fresh 0.93.3 codebase, with the localization file copied to both mg1/localization and id2/localization (with id2 being a copy of id1 from the re-release), and the strings were properly localized in both cases. Note that each mod gets its own localization - the right choice for an open engine, IMO, but different from the re-release - maybe that's the issue?

As for the zip support, I completely agree, and I tried to keep this patch as simple as possible. For true localization we'd also have to add UTF-8 support, UI integration, and maybe per-map localization files, but those are different issues for another time.

[sezero]

I've noticed that the overflow warning in PF_VarString was moved to the legacy concatenation code path (the else block of if (localized)), whereas in my code it was triggered in both cases, since I figured we're generating a large string that would have overflowed WinQuake's buffer either way. It's a minor point, but I wanted to confirm it's intentional.

Not intentional, I missed it I guess. BTW, why did you changed the error into a warning? (haven't yet tested)

I've also tested the QS patch on a fresh 0.93.3 codebase, with the localization file copied to both mg1/localization and id2/localization (with id2 being a copy of id1 from the re-release), and the strings were properly localized in both cases. Note that each mod gets its own localization - the right choice for an open engine, IMO, but different from the re-release - maybe that's the issue?

Ah, I totally missed the quakefs behavior to load from gamedir, not basedir: moving the localization file under id1 works.

As for the zip support, I completely agree, and I tried to keep this patch as simple as possible. For true localization we'd also have to add UTF-8 support, UI integration, and maybe per-map localization files, but those are different issues for another time.

Yes: That alone makes the whole localization code here non-functional: you want to add it for reference purposes?

[andrei-drexler]

moving the localization file under id1 works.

👍

Yes: That alone makes the whole localization code here non-functional: you want to add it for reference purposes?

I wanted to add this mostly for compatibility with the re-release (with only a small bit of manual user intervention), but I think it can also serve as a good first step towards true localization. Even without UTF-8, basic support for Italian, French, Spanish, German, latinized Russian etc. could be added relatively easily in the future. It wouldn't be as comprehensive as what the NightDive engine offers, but it also wouldn't be a whole lot of extra code.

[sezero]

Pushed the three minimal changes with your name. Please rebase.

[sezero]

Wtih the three minimal parts in, which avoids the errors and the campaign spam,
I have reservations about the rest, as explained above. To summarize:

• Changing the Host_Error to simply a warning in CL_ParseServerMessage::
  svc_updatefrags case may not be a good idea. Loading saves in QS doesn't
  error, so we should find the reason for errors in vkQuake.

• Localized strings support doesn't work unless the user manually extracts the
  relevant files to under id1/. Therefore we need not those parts, save for them
  being for future reference.

Leaving the decisions to @Novum

[sezero]

BTW, someone posted a link to achievement thing on sf.net/qs site; for reference,
in case anyone is interested: https://steamdb.info/app/2310/stats/

[vide0hanz]

EDIT: Disregard, I see now that localization changes were not yet commited. Personally I do not see an issue with a little manual user intervention regarding this. Documenting how to do it should suffice. After manually patching in the changes, it works great.

I'm currently testing this but cannot get the localization strings to appear correctly. I have tried putting loc_english.txt in id1/ as well as id1/localization/loc_english.txt. The spam campaign error messages are resolved, and the game no longer crashes when triggering an achievement; only issue for me is the localization.

If I understand the comments above, extracting loc_english.txt into id1 should work, correct?

[bangstk]

* Localized strings support doesn't work unless the user manually extracts the
  relevant files to under `id1/`. Therefore we need not those parts, save for them
  being for future reference.

I think this should be kept in, because without this, all the centerprint messages show up as stuff like "$map_skill_hard" ingame. Yes it does take a bit more effort for an end user (extracting the strings file from the QuakeEx.kpf, which is just a renamed zip file), but it's preferable to not having readable strings at all. Unless you would like to hardcode the English strings directly into the engine....

[vide0hanz]

@andrei-drexler Do you happen to have a patch file handy for all of your proposed changes here?

[andrei-drexler]

@andrei-drexler Do you happen to have a patch file handy for all of your proposed changes here?

Sure, here it is (a neat trick that I only learned recently - you can append .patch or .diff to the URL of a pull request or commit to get a plain-text version).

[vide0hanz]

@andrei-drexler Do you happen to have a patch file handy for all of your proposed changes here?

Sure, here it is (a neat trick that I only learned recently - you can append .patch or .diff to the URL of a pull request or commit to get a plain-text version).

Oh nice, I didn't know that. Thanks! Just wanted this for reference.

[vide0hanz]

@andrei-drexler Hmm, while the localization stuff works, for some reason the messages which are displayed when completing an episode still show their variable names:

$qc_finale_e1 for example.

[andrei-drexler]

@sezero:

I have reservations about the rest, as explained above. To summarize:

* Changing the `Host_Error` to simply a warning in `CL_ParseServerMessage`::
  `svc_updatefrags` case may not be a good idea. Loading saves in QS doesn't
  error, so we should find the reason for errors in vkQuake.

* Localized strings support doesn't work unless the user manually extracts the
  relevant files to under `id1/`. Therefore we need not those parts, save for them
  being for future reference.

For the svc_updatefrags error, it's true that I'm only treating a symptom instead of addressing the root cause, but this fix seemed a lot safer than the alternative, and it can easily be reverted once we have a better solution. In the meantime, though, the new content isn't really playable if loading a savefile doesn't work.

Setting aside the savegame issue (which QS doesn't have anyway), even if the maps are technically playable, they're still less enjoyable IMO because of the missing localization - seeing $map_mg1_intro_e1 doesn't have the same effect as This is the realm of Machinists. The cogs are turning. For the kind of players that would even consider trying the update in QuakeSpasm or vkQuake, I also think unzipping the localization files manually shouldn't be much of a problem if the process is clearly documented in the readme/release notes, and seems preferable to having illegible message throughout the whole game.

[sezero]

For the svc_updatefrags error, it's true that I'm only treating a symptom instead of addressing the root cause, but this fix seemed a lot safer than the alternative, and it can easily be reverted once we have a better solution. In the meantime, though, the new content isn't really playable if loading a savefile doesn't work.

Setting aside the savegame issue (which QS doesn't have anyway),

I wonder whether QSS has the issue
I mean, is it possible that we incompletely merged the FTE protocol extensions?

even if the maps are technically playable, they're still less enjoyable IMO because of the missing localization - seeing $map_mg1_intro_e1 doesn't have the same effect as This is the realm of Machinists. The cogs are turning. For the kind of players that would even consider trying the update in QuakeSpasm or vkQuake, I also think unzipping the localization files manually shouldn't be much of a problem if the process is clearly documented in the readme/release notes, and seems preferable to having illegible message throughout the whole game.

I see your points, I do. If we are to apply the localization though, we

• either have to document what the users have to do,
• or add some unzip code to extract the localization file

Either way, it still is problematic when e.g. you have a network play:
If a rerelease server sends the localized message, the clients shall
have no means at all to decode it unless they also are running the
rerelease thing (no, I haven't tested that, just theorizing.)

I still wonder what @Novum has to say on the matter?

[vide0hanz]

I would think that the localization stuff is mostly for client-side messages. That said, even with this current approach, it is hard coded to look for the loc_english.txt file. Not sure if it can be made more generic to just look for *.txt instead, otherwise adding in || loc_french.txt || loc_german.txt || loc_italian.txt || ...etc would be needed, which also doesn't seem ideal.

While I am definitely in favor of having something which works as a stop-gap just to play the rerelease content, I have to wonder if there is a better way of going about it.

[andrei-drexler]

@vide0hanz:

@andrei-drexler Hmm, while the localization stuff works, for some reason the messages which are displayed when completing an episode still show their variable names:

$qc_finale_e1 for example.

It should work, I've just tested on e1m7 (and also in the new campaign earlier)

[vide0hanz]

@andrei-drexler Odd. I have no idea why these specific variables aren't localizing on my end when everything else works fine. I'm using a new pakfile I created which includes all of the updated models, and while I can't think of any obvious reason why that would affect this, its possible I'm missing something there.

EDIT: removed my custom pakfile from the equation, same issue.

UPDATE: I manually applied your patch again from a new git clone of this repo and it appears to be working.

This error is still curious though, happens as soon as you beat the base game:

quakec/monsters/shub.qc : finale_5
<NO FUNCTION>
unimplemented builtin #79 - finaleFinished
Host_Error: Program error

[andrei-drexler]

This error is still curious though, happens as soon as you beat the base game:

Good catch, fixed in 787ab3f

[sezero]

We also need some dummy menu_credits command. The following
is e.g. from dopa or mg1 when you complete the game.

             WE SALUTE YOU.
����������������������������������������
Unknown command "menu_credits"
Sending clc_disconnect
Client player removed

[andrei-drexler]

We also need some dummy menu_credits command.

Oh, you're right, I saw that too a couple of times, but I was always in the middle of something else and never got to it. 2fd8993

[sezero]

Hit this in hipnotic:

You got Mjolnir
Gremlin stole your Rocket Launcher
You got {}$qc_rocket_launcher, 4 rockets
You receive 25 health

[sezero]

Hit this in hipnotic:

You got Mjolnir
Gremlin stole your Rocket Launcher
You got {}$qc_rocket_launcher, 4 rockets
You receive 25 health

Seems to always happen when gremlins steal your weapon
and then you kill it and take it back:

Gremlin stole your Rocket Launcher
You got {}$qc_rocket_launcher, 5 rockets
Gremlin stole your Lightning Gun
You got {}$qc_thunderbolt, 38 cells
Gremlin stole your Nailgun
You got {}$qc_nailgun, 36 nails
Gremlin stole your Lightning Gun
You got {}$qc_thunderbolt, 40 cells

Save file: s4.sav.zip

[andrei-drexler]

Rebased (since there were changes to vkquake.pak on master) and pushed 79f5de5. It looks like the old .ent.orig files in the repo had newlines added at the end, the versions I committed don't, they're straight from the bsp's (since we're crc-ing them, after all).

Hm?? There is no way of playing any other mission packs etc using the in-game UI either, what is special about mg1?

Just trying to make it easier for new players whose first contact with Quake was the re-release (which does show the new expansions in the UI) and might not know about the game command.

[sezero]

It looks like the old .ent.orig files in the repo had newlines added at the end, the versions I committed don't, they're straight from the bsp's (since we're crc-ing them, after all).

In the new commit, they have a nul-terminator at the end instead of newline:
(correction: '\0' after the last newline)
How did you extract the ents into files? Do the extracted files crcs match the
in-game generated ones?

Hm?? There is no way of playing any other mission packs etc using the in-game UI either, what is special about mg1?

Just trying to make it easier for new players whose first contact with Quake was the re-release (which does show the new expansions in the UI) and might not know about the game command.

Well, if someone is wanting to play rerelease using qs or vkquake, it means they either can't run the rerelease as is and/or don't care about rerelease's new features, either way mentioning that is unnecessary: qs has documentation for the game command (feel free to mention it in vk vrsion if you want, though.)

[sezero]

It looks like the old .ent.orig files in the repo had newlines added at the end, the versions I committed don't, they're straight from the bsp's (since we're crc-ing them, after all).

In the new commit, they have a nul-terminator at the end instead of newline:
(correction: '\0' after the last newline)
How did you extract the ents into files? Do the extracted files crcs match the
in-game generated ones?

OK, the ents in the pak do contain a nul terminator and the crcs orig files that you
replaced do match the in-game generated ones. I don't like the nul-terminator though.

[andrei-drexler]

In the new commit, they have a nul-terminator at the end instead of newline:
(correction: '\0' after the last newline)
How did you extract the ents into files? Do the extracted files crcs match the
in-game generated ones?

I've used COM_WriteFile during loading, passing it the entire lump data. The zeroes are present in the BSP, but I guess I could check for a terminating NUL, and only hash (and re-export) the preceding part. It seems more hacky/error-prone, but it will clean up the .orig file.

[sezero]

I could check for a terminating NUL, and only hash (and re-export) the preceding part. It seems more hacky/error-prone, but it will clean up the .orig file.

No need to add extra complexity I guess.

[andrei-drexler]

Hmm. I have mixed feelings about this, but I think it would be a good idea to be able to CRC .ent files extracted with bsputil and have them match if nothing's changed. And it seems bsputil doesn't include the NUL.

[sezero]

Well, if the only change will be l->filelen - 1 then it's OK

[andrei-drexler]

Unfortunately, it seems bsputil writes files in text mode, so there will be newline differences anyway...

[sezero]

Unfortunately, it seems bsputil writes files in text mode, so there will be newline differences anyway...

Ugh. That can happen on windoze.. just dos2unix the extracted files.

(BTW, your generated pak has all the files in dos cr/lf mode..)

[sezero]

OK, I think the following should be good, and we can go back to old orig files,
If it looks good to you feel free to apply and update crcs.

diff --git a/Quake/gl_model.c b/Quake/gl_model.c
index d15bebf..f8ed30c 100644
--- a/Quake/gl_model.c
+++ b/Quake/gl_model.c
@@ -801,12 +801,14 @@ void Mod_LoadEntities (lump_t *l)
 	char	basemapname[MAX_QPATH], entfilename[MAX_QPATH];
 	char		*ents;
 	int		mark;
-	unsigned int	path_id, crc;
+	unsigned int	path_id;
+	unsigned int	crc = 0;
 
 	if (! external_ents.value)
 		goto _load_embedded;
 
-	crc = CRC_Block(mod_base + l->fileofs, l->filelen);
+	if (l->filelen > 0)
+		crc = CRC_Block(mod_base + l->fileofs, l->filelen - 1);
 	mark = Hunk_LowMark();
 
 	q_strlcpy(basemapname, loadmodel->name, sizeof(basemapname));

[andrei-drexler]

Ugh. That can happen on windoze..

And it did, that's how I found the issue :(

just dos2unix the extracted files

Sure, just that I was hoping other users could extract a crc-matching file with bsputil without any manual intervention, but it seems manual intervention is a recurring theme...

(BTW, your generated pak has all the files in dos cr/lf mode..)

Ouch, I was wondering why they were bigger... I guess that's what mkpak.sh ends up doing on windows (using msys). It looks like I'll have to reboot and try this again using ubuntu.

OK, I think the following should be good, and we can go back to old orig files,
If it looks good to you feel free to apply and update crcs.

Alright.

[sezero]

Ouch, I was wondering why they were bigger... I guess that's what mkpak.sh ends up doing on windows (using msys). It looks like I'll have to reboot try this again using ubuntu.

Don't worry about that, you finalize everything else, I will regenerate the pak
And we can then start merging: (1) ent-crc patch, (2) default.cfg patch,
and finally (3) localization patch.

[sezero]

To verify each other's results, with the nul-terminator removed the crcs I found are:
e1m1 -> c49d
e1m2 -> 0caa
e1m4 -> 958e
e2m2 -> fbfe
e2m3 -> 237a
e2m7 -> 10a8

[andrei-drexler]

Don't worry about that, you finalize everything else, I will regenerate the pak
And we can then start merging: (1) ent-crc patch, (2) default.cfg patch,
and finally (3) localization patch.

If you're already taking care of the ents/paks (thank you, I've rebooted but it looks like you're already ahead), I'm not sure what's left to do besides the readme tweaks :)

To verify each other's results, with the nul-terminator removed the crcs I found are:
e1m1 -> c49d
e1m2 -> 0caa
e1m4 -> 958e
e2m2 -> fbfe
e2m3 -> 237a
e2m7 -> 10a8

Confirmed.

[sezero]

If you're already taking care of the ents/paks (thank you, I've rebooted but it looks like you're already ahead), I'm not sure what's left to do besides the readme tweaks :)

Pushed f9156f1 to git master: make sure all are OK (and that ents are identified correctly at runtime)

[sezero]

And pushed the default.cfg patch as cbd25e6

[andrei-drexler]

I checked, and all the ent files load correctly. I also force-pushed the rerelease branch back to the commit before the default_cfg change.

[sezero]

Merged the rest, i.e. the localization stuff.

Let's do any further improvement in master (or with new pull requests if necessary.)

[andrei-drexler]

Alright, thank you for helping out with all this, and thanks to everyone else that tested the in-progress code and offered feedback!

[sezero]

Now pushed the patch to QS, too.

[andrei-drexler]

I've only looked at this very briefly earlier, but there seemed to be significant differences when I compared the entity list from a rerelease map to the old version - at the very least the order of the entities has changed, so I'm not sure we can safely take the new fog/waterlevel values and apply them to the old entity files to produce a new pak. Plus, I really think z-fighting should be dealt with by the renderer - after all, the same maps with the same problematic entities don't have this issue in the new engine.