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

AGI: Fix missing words from our dictionary #5676

Merged
merged 1 commit into from Mar 1, 2024

Conversation

antoniou79
Copy link
Contributor

This fixes bug #15000 "V Demo does not recognize valid word ammunition"

Our code was actually not parsing correctly many (all?) words starting with "a" in this particular game.

I've also quickly checked with Space Quest 0 AGI fan made game and it seems that the workaround code (which was made for a bug reported for that game) still works with my fix.

Of course someone more familiar with the AGI engine should review this.

This fixes bug #15000 "V Demo does not recognize valid word ammunition"

Our code was actually not parsing correctly many (all?) words starting with "a" in this particular game.
@sluicebox
Copy link
Member

Thanks! I'm going to compare this to some other parsers (and mine) first, then run some tests. Good job figuring out that this is affecting multiple games.

@bluegr
Copy link
Member

bluegr commented Feb 29, 2024

Nice work!

The change looks to be correct, that workaround code is skipping words, indeed - good catch!

I also did some checks on other AGI interpreters. For reference:

Apart from NAGI, which does additional checks on the dictionary load code, the other interpreters don't include special handling, like we do

@sluicebox
Copy link
Member

Here's the history:

In 2013 it was noticed that the fan-game Space Quest 0's dictionary broke our parser, because it contains words that start with digits. A workaround was added, but the workaround was broken: b1a7492

The workaround attempted to skip words with digits, but it also skipped parsing the word id, so it broke parsing subsequent words in the same letter section, "a" in this case.

Nine years later, someone noticed this problem in V Demo, it's just like SQ0: it has words with digits. It's just that nobody noticed that the SQ0 workaround wasn't complete.

This PR fixes the workaround in a simple way, and I'm going to merge it so that it can be cherry picked for 2.8.1.

I think the bigger problem is with this parser structure; it's based on the incorrect premise that a word can't begin with a copy-byte value of zero. Fix that and you don't need workarounds at all. The error checking is spotty and it's inconsistent with the V1 parser's structure, which also has incorrect copy/pasted comments. I plan on rewriting this to be simpler and not have these problems, but not for 2.8.1. For context, I tried using this code to write my parser last month and gave up and read the spec instead, and mine parses all of these without special handling.

Thanks for figuring this out!

@sluicebox sluicebox merged commit 9828447 into scummvm:master Mar 1, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants