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

WINTERMUTE: Add detection for current Steam release of reversion1 #890

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@lotharsm
Member

lotharsm commented Jan 15, 2017

This adds proper detection for the current version of the
Steam release of 'Reversion: Chapter 1 - The Escape'.

Partially fixes #9679.

The Linux versions will follow as soon as I get a working Linux VM for testing.

rootfather
WINTERMUTE: Add detection for current Steam release of reversion1
This adds proper detection for the current version of the
Steam release of 'Reversion: Chapter 1 - The Escape'.

Partially fixes #9679.
@lotharsm

This comment has been minimized.

Member

lotharsm commented Jan 30, 2017

I tried to add the Linux versions of reversion1 today. The games are detected, but the only version that starts is the Spanish one. I have to figure out why, I did the same as I did with the Windows versions. Maybe language selection via Common::XY_XYZ is broken for non-Windows wintermute?

Therefore I suggest to merge this PR at it's current state in order to fix #9679. We can always open a new PR if I can sort out the Linux issues.

What do you think?

@lotharsm lotharsm requested a review from sev- Feb 4, 2017

@@ -68,8 +68,8 @@ static const PlainGameDescriptor wintermuteGames[] = {
{"pigeons", "Pigeons in the Park"},
{"projectdoom", "Project: Doom"},
{"projectjoe", "Project Joe"},
{"reversion1", "Reversion: The Escape"},
{"reversion2", "Reversion: The Meeting"},
{"reversion1", "Reversion: Chapter 1 - The Escape"},

This comment has been minimized.

@sev-

sev- Feb 5, 2017

Member

Is it how the games were actually named? E.g. with "Chapter 1 -" in the title?

This comment has been minimized.

@lotharsm

lotharsm Feb 5, 2017

Member

I based this on the game titles how they appear in the Steam store.

See http://store.steampowered.com/app/270570/ and http://store.steampowered.com/app/281060/ for reference.

This comment has been minimized.

@somaen

somaen Feb 8, 2017

Member

That's not how they appear in the Steam-store either.

Reversion - The Escape (1st Chapter) (Where the buy button ignores the "1st Chapter" part)
Reversion - The Meeting (2nd Chapter)

What does the in-game window-title say?

I'm not against adding the numbers, but it needs to make sense and be consistent. (For instance the Carol Reed games are all numbered, even though the numbering is not ALWAYS applied in the games themselves).

This comment has been minimized.

@lotharsm

lotharsm Mar 11, 2017

Member

The in-game window-title just says "Reversion - Wintermute Engine". The in-game title screen says "Reversion: Chapter 1 - The Escape.

// to the game directory.
WME_WINENTRY("reversion1", "Steam",
WME_ENTRY2s("data.dcp", "5e4d40075f69fa7702530e38c349d2fd", 254293949,
"data.dcp", "5e4d40075f69fa7702530e38c349d2fd", 254293949), Common::ES_ESP, ADGF_UNSTABLE, LATEST_VERSION),

This comment has been minimized.

@sev-

sev- Feb 5, 2017

Member

What you need to use here is WME_ENTRY1s() macro:

WME_ENTRY1s("data.dcp", "5e4d40075f69fa7702530e38c349d2fd", 254293949),

Or you mean that when you're using it with one entry, the language is not detected correctly?

If so, then you're nicely exploiting the fact that AD makes preference by the number of matched files, which is 2 in this case.

This comment has been minimized.

@lotharsm

lotharsm Feb 5, 2017

Member

I tried to use the WME_ENTRY1s() macro and it failed, the Spanish version simply isn't detected.

Somewhere in the detection_tables.h there's a note that you mustn't mix WME_ENTRY1s and WME_ENTRY2s/3s.

This comment has been minimized.

@sev-

sev- Feb 7, 2017

Member

I don't know whose idea is that it is bad to mix those macros', but there is no problem with mixing those.

Could you please provide me with the full log at '-d11' what happens when you're trying to detect Spanish version with WME_ENTRY1s()?

This comment has been minimized.

@lotharsm

lotharsm Feb 8, 2017

Member

I'll try this tomorrow. Would be a lot nicer than the current 'hack'.

This comment has been minimized.

@somaen

somaen Feb 8, 2017

Member

So, the thing here is that quite a few of the Wintermute-games are distributed with a bunch of language-files installed, and then they do language-selection by shuffling files prior to starting the engine, such that one language file is within reach of the engine.

Now, what you WANT to happen in that case, is that the detection detects ALL the language versions that are available, so that the user gets the options. (The engine itself then selects which files to ignore/load based on the language it gets passed from the launcher).

I assume that what's going on here, and the reason why I added the comment about not mixing WME_ENTRY1s and WME_ENTRY2s, is to ensure that you get all the languages detected no matter if you have 10 or 1 language file in the folder at the time when detection is run. (My assumption being that the WME_ENTRY1s hit will be ignored if you have 1 or more 2-entry hits).

So, the use case here is:
"User has installed game with i.e. xlanguage_en.dcp available in game folder, but would like to play in Spanish too".

If my assumption is correct, then using the WME_ENTRY1s solution would lead to only detecting english, while doing the extra entry with WME_ENTRY2s would present the user with the option of choosing spanish as well.

This comment has been minimized.

@lotharsm

lotharsm Mar 11, 2017

Member

Then I guess your assumption is correct, because when the only entry present in the detection table is the WME_ENTRY1s to the data file, then only the English version is detected. When I mix it with WME_ENTRY2s, the ENTRY1s entry is ignored. When I only use WME_ENTRY2s, then all languages are detected.

// seems to switch between languages by simply copying the desired
// language file from /languages to the main folder.
//
// To enable Spanish speech, remove xvoice_en.dcp from your game directory.

This comment has been minimized.

@somaen

somaen Feb 8, 2017

Member

This isn't optimal, and I wasn't aware of this artifact, there's code in base/base_file_manager.cpp that explicitly whitelists files that are in the languages/ folder based on the current language, that list currently lacks handling of spanish (and any explicit white-listing of xvoice_*.dcp files.)

What's the default after installation? xvoice_en.dcp in the game-directory directly?

This comment has been minimized.

@lotharsm

lotharsm Mar 11, 2017

Member

Yes. xvoice_en.dcp and xlanguage_en.dcp are in the game-directory after installation. On first start, you get a language selection screen which then copies the necessary language files to the game-directory if you choose any other language than English.

@sev-

This comment has been minimized.

Member

sev- commented Mar 11, 2017

Any update on this?

@lotharsm

This comment has been minimized.

Member

lotharsm commented Mar 11, 2017

@sev-, I provided the missing information.

@csnover

This comment has been minimized.

Member

csnover commented Oct 7, 2017

@RootFather there was a lot of past discussion on this ticket. When you get a chance can you just recap what is still outstanding here that needs to be addressed? It looked like there was a naming issue and a language detection issue and it’s unclear to me from the discussion what was the final outcome/decision.

@lotharsm

This comment has been minimized.

Member

lotharsm commented Oct 7, 2017

@csnover I haven't worked on this patch for quite some time, so I have to somewhat figure out again what's going on.

One thing we have to handle with is that WME_ENTRY1s cannot be used because it won't detect the other languages and we have to use WME_ENTRY2s twice to detect the spanish version.

And yes, there's still a naming issue, but I think that can simple be reverted to the state it was before this PR.

@lotharsm lotharsm closed this Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment