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

Refactor RTP handling #1754

Merged
merged 20 commits into from Jul 14, 2019

Conversation

@Ghabry
Copy link
Member

commented Apr 22, 2019

Installed RTP selection algorithm:
All assets in a directory are mapped against all RTP and the hit/max ratio is checked. The RTPs with the highest ratio in the folder are considered as the candidates.

Game RTP selection algorithm:
We don't know which RTP the game uses, so the strategy is: Take the requested game asset (that was not found in the game folder) and map it against all RTP in the table -> Results:

  • 0 RTP matches: Not a RTP asset -> do direct search in all RTP paths
  • 1 RTP match: Perfect, we know the RTP now (the Game RTP selection algorithm is finished)
  • 2+ RTP matches: When this was the first asset -> write all candidates in a list
  • 2+ RTP matches and this was not the first asset: Compare the new candidates against the candidates of the previous asset and only keep the RTPs that contain all assets (this will reduce the list to 1 RTP after 2 or 3 assets). In rare cases (totally messed up game or rtp folders) the can result in 0 RTPs, in this case the algorithm repeats from the beginning.

Lookup strategy:
All game RTP against all detected RTP until the FileFinder finds a file.
This is the new Any-To-Any lookup, it doesn't matter anymore which RTP(s) are installed.
Fix #802
(while typing this I noticed that I forgot the "game RTP = detected RTP" case for the lookup)

When all this detection magic fails it directly searches the file in all RTP search paths (Fix #1709). This is after all the magic to prevent undesired hits in the folder (because of name overlaps between the different RTPs)

I changed the RTP structure to a table and do linear searching in it. Some cases can be still optimized, e.g. finding the correct asset category can be a binary search because this column is sorted.
Also prints diagnostics about the detected RTPs, great to blame the Android users when they send us bogus bug reports :P.

Design decision: Don's RTP extras are included in the table, this is not really necessary because there is mapping possible but it is great for the startup diagnostics (maybe could add an extra warning that the game uses such an asset)

@Ghabry Ghabry added the FileFinder label Apr 22, 2019

@Ghabry Ghabry added this to the 0.6.1 milestone Apr 22, 2019

@Ghabry Ghabry force-pushed the Ghabry:rtp-table branch from 9da44bf to 40d92ae Apr 22, 2019

@Ghabry

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

Test cases:

RTP detection:

  • Folder contains one RTP (tested 2k(3) EN/JP, Don, Advocate). The wave dash in 2k3JP could be problematic.
  • Folder contains two RTPs (e.g. Don + DonExtra)
  • Folder contains partial RTP
  • Folder contains nonsense or is empty (hello, Android users)

RTP FileFinder:

  • Game uses same RTP as installed
  • Game uses other RTP as installed
  • Game uses asset that is not in the game folder and not a RTP asset (passthrough test), ExFont is a good test candidate here

Extra:

  • Game RTP detection logic test: The 1st asset yields 2 possible RTP
@carstene1ns

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

Ghabry> wow RPG advocate reencoded all sound effects to mp3 m(

As said on IRC (which you do not read), they did it to decrease download size and shipped a decompression script: https://paste.xinu.at/Fsl8Aj/ RPG_RT does not play mp3 SE.

@Ghabry

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

I would prefer keeping this mp3 detection because this mitigates a user error (though this is only cosmetic, even with no sounds detected Advocate will still win in a Advocate-only folder).

@Ghabry Ghabry force-pushed the Ghabry:rtp-table branch 2 times, most recently from 9c5893f to 7b8444e Apr 22, 2019

@Ghabry Ghabry marked this pull request as ready for review Apr 22, 2019

@fdelapena fdelapena added the Refactor label Apr 22, 2019

@Ghabry

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

Some lookup test cases, e.g. the Title BGM is "Suspicion" (Official English) and the installed RTP is Don+extras:

Debug: RTP is "Don Miguel English Translation" (500/500)
Debug: RTP is "Don Miguel RTP Addon" (503/503)

system
Candidate: Official English
Candidate: Don Miguel English Translation
---
title1
Candidate: Official English
Candidate: Don Miguel English Translation
---
suspicion
Candidate: Official English
---
Debug: Game uses RTP "Official English"

The Title is skipped and installed RTP2k3 is Advocate:

Debug: RTP is "RPG Advocate English Translation" (675/675)

system
Candidate: Official English
Candidate: Vlad Russian Translation
Candidate: RPG Universe Spanish/Portuguese Translation
---
ship
Candidate: Official English
Candidate: RPG Advocate English Translation
Candidate: Vlad Russian Translation
---
actor1
Candidate: Official English
---
Debug: Game uses RTP "Official English"

To RTP are installed (2kjp and 2ken) in different folders, 2kjp lacks the Music files for some reason:
Result: The song still plays.

Debug: RTP is "Official Japanese" (368/465)
Debug: RTP is "Don Miguel English Translation" (500/500)

system
Candidate: Official English
Candidate: Don Miguel English Translation
---
title1
Candidate: Official English
Candidate: Don Miguel English Translation
---
suspicion
Candidate: Official English
---
Debug: Game uses RTP "Official English"

@Ghabry Ghabry force-pushed the Ghabry:rtp-table branch from 7b8444e to 0f58421 Apr 23, 2019

@Ghabry

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

I observed something strange with the 64bit Wine prefix: Some keys are not redirected to Wow6432Node, as you can see in this picture:
(HKCU redirection is wrong btw, Windows doesn't redirect them)

Screenshot_20190423_211018-fs8

RTP and 2003 are 32bit executables, so I'm not sure what is causing this. I changed our wine registry now to check both Wow6432Node and the not redirected case. Now my RTP installs in Wine are actually found again :)

okay now this is really ready for review

@Ghabry Ghabry force-pushed the Ghabry:rtp-table branch from 5200831 to 5457e43 Apr 23, 2019

@carstene1ns

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

(HKCU redirection is wrong btw, Windows doesn't redirect them)

What does this mean? Is it wrongly implemented? Is it wrong in the docs? 👎
Because that is exactly how my patch should work: 317c70c#diff-059176e8293a79703c0e08f26fad62aeR64

Also, wine may have bugs as well?

@Ghabry

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

I mean MSDN says that HKCU is shared and not redirected but Wine does (but not always? look at my screenshot), so our implementation was wrong.
Could be a Wine bug, but I can't search through thousand registry bugs in Wine.
The new solution is to look now in Wow6432Node and in the not redirected folder.

@carstene1ns

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

I am not really against testing both, but this looks like a bug, and we should not emulate "not an emulator" bugs.

src/registry_wine.cpp Outdated Show resolved Hide resolved
@Ghabry

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

When I install the RTP before the Editor all registry values appear in Wow6432Node, must be some Wine bug, but can't figure it out.

Keys created through a small test program always appear in Wow6432Node and the RTP also always appears always in Wow6432Node, therefore I will only check this node.

(Our current code is incorrect, as we check HKCU\Software\Kadokawa and not HKCU\Software\Wow6432Node\Kadokawa, I wonder if this worked years before and Wine changed this behaviour or if our code was always wrong o.O)

Please use regedit and check where the RuntimePackagePath key is.

@carstene1ns

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Then again: HKLM/Software is redirected, HKCU/Software is shared. Either wine or the MSDN docs are wrong here.
https://docs.microsoft.com/en-us/windows/desktop/WinProg64/shared-registry-keys


Feel free to remove the regview bs then, if even wine does not care, we can simplify this...

@Ghabry

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

Wine doesn't follow the MSDN docs, they redirect HKCU, Windows shares it.

EDIT:
The Wine redirection unit tests only test HKLM.
By now I found the location where Wine actually creates the keys, maybe helps me to resolve now this mystery in a debugger ^^

@Ghabry

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

Okay I finally found the code, this proofs that currently Wine only shares Software\Classes, everything else is just redirected.

https://github.com/wine-mirror/wine/blob/master/server/registry.c#L1853-L1863

Introduced via wine-mirror/wine@178cd20#diff-f4564e991742bef0d9f6de588887a906

    /* set the shared flag on Software\Classes\Wow6432Node */
    if (prefix_type == PREFIX_64BIT)
    {
        if ((key = create_key_recursive( hklm, &classes_name, current_time )))
        {
            key->flags |= KEY_WOWSHARE;
            release_object( key );
        }
        /* FIXME: handle HKCU too */
    }

@Ghabry Ghabry force-pushed the Ghabry:rtp-table branch from 5457e43 to 08f3ecb Apr 26, 2019

src/filefinder.cpp Outdated Show resolved Hide resolved
src/filefinder.cpp Outdated Show resolved Hide resolved

@Ghabry Ghabry force-pushed the Ghabry:rtp-table branch from 6d61ffe to 4afce9b Jun 8, 2019

@Ghabry

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

(just rebased to master)

src/rtp.cpp Outdated Show resolved Hide resolved
src/rtp.cpp Outdated Show resolved Hide resolved

@EasyRPG EasyRPG deleted a comment from Ghabry Jun 8, 2019

@Ghabry Ghabry force-pushed the Ghabry:rtp-table branch from 4afce9b to cc74cf6 Jun 9, 2019

@Ghabry

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

Optimized the RTP in folder detection and added some unit tests (also contains a test if the generated tables are up-to-date)

@Ghabry Ghabry force-pushed the Ghabry:rtp-table branch 2 times, most recently from e8bf884 to 4879b9c Jun 9, 2019

Ghabry added some commits Jun 9, 2019

RTP: Use num_2k(3)_rtps as array bounds, will generate a compiler err…
…or when a new RTP is added and the numbers were not updated

@Ghabry Ghabry force-pushed the Ghabry:rtp-table branch from 4879b9c to 824dd2e Jun 9, 2019

@fdelapena fdelapena requested a review from carstene1ns Jun 30, 2019

@fdelapena fdelapena merged commit e499e94 into EasyRPG:master Jul 14, 2019

7 checks passed

Android (armeabi-v7a) Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Wii (SDL1) Build finished.
Details
Windows (x64) Build finished.
Details
Windows (x86) Build finished.
Details
web Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.