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

Fix for games with altered file extensions #2271

Merged
merged 2 commits into from Sep 2, 2020

Conversation

sorlok
Copy link
Contributor

@sorlok sorlok commented Jul 23, 2020

Ready to merge (from my point of view).

This is my implementation for Issue #1920; please see the initial discussion there.

Regarding @Ghabry's comments:

  1. Check only 2 files: Done, but see note
  2. Assume larger==LDB: Done
  3. LcfXXX checks: Done, but see note

Some notes on this fix:

  1. What's the easiest way to cheaply check if a directory might be a game project? Right now I check for RPG_RT.ini, but as you said that's not always present. I expect RPG_RT.exe might also be stripped. Is there any good file to check, or should I just disable this minor performance hack? ---Side note: it might be worth keeping if all games with hacked extensions have .ini files.
  2. The Meta file now has "LmtHeaderString=XXX", which allows us to pass the "XXX" string in to liblcf (i.e., to tell it what string to expect within the file header). As you said, errors are disabled if this string doesn't match --but it still shows a warning. Do we want to allow passing in the expected header string to liblcf when parsing to silence the warning, or is this not worth the trouble?
  3. We cannot apply the Meta file until after scanning the directory. Main reason: the Meta instance is stored in player.cpp. Sub reason: we don't know which file to calculate the CRC32 of until we scan (since they were renamed). This is not a huge problem, since the scanning is pretty fast.
  4. I scan the names of all mapXXXX.ext files to ensure the extension is consistent across files. This might be overkill; we could (a) just stop on the first file or (b) check for the starting map filename (pulled from LMT), assuming we've parsed LMT at this point (which I think we have).
  5. This fix assumes all "rpg_rt.XXX" files are renamed by extension ONLY --i.e., if you rename it to "abc.lmt" then this fix won't find it. Do any hacks do this, and do we care?
  6. Just reiterating that the meta file can be used to overcome most limitations as discussed (e.g., slow lmu scanning and small LDB files).

@Ghabry
Copy link
Member

Ghabry commented Jul 24, 2020

Thanks for your contribution. Really cool to have support for this. Reviewing can currently take longer because it is summer and vacation time, so don't worry if there is less feedback . We are more active during the cold months ;).

@Ghabry
Copy link
Member

Ghabry commented Jul 24, 2020

(I was mentioning std::regex before but this should work properly since gcc5.0 so can be ignored)

  • What does "Snh" mean?

What's the easiest way to cheaply check if a directory might be a game project? Right now I check for RPG_RT.ini, but as you said that's not always present. I expect RPG_RT.exe might also be stripped.

When a game tree is parsed the file list is already in memory so the expensive operation (Read directory and alloc all the strings) is already done. I don't think doing some string testing will be really noticable. Also your code will only execute for the "not a game" case which shouldn't happen too often so don't optimize too much.

Sometimes games rename both RPG_RT and the ini. When the Exe is renamed it also changes the ini file it searches for (Game.exe will search for Game.ini)

[LCF Header] As you said, errors are disabled if this string doesn't match --but it still shows a warning. Do we want to allow passing in the expected header string to liblcf when parsing to silence the warning, or is this not worth the trouble?

The "warning" is only printed on terminal so I don't consider this important

I scan the names of all mapXXXX.ext files to ensure the extension is consistent across files.

I agree on you this is overkill. Just using the first mapfile you can find is good enough, we can still adopt if any game ever breaks this heuristic... (because there is some Map0001.bak there)

[RPG_RT prefixed changed] Do any hacks do this, and do we care?

Not sure if any game does this. Is also a bit hard to analyse. When you provide me with some script I can use to scan hundreds of games easily I can tell you quickly ;)

@sorlok
Copy link
Contributor Author

sorlok commented Jul 26, 2020

What does "Snh" mean?

Just my initials; I usually prefix branches with this to minimize the chance of conflict. Let me know if you prefer an alternative naming scheme (I can file a new PR).

I don't think doing some string testing will be really noticeable.

Great, I removed the INI check.

The "warning" is only printed on terminal so I don't consider this important

Great, I removed this from the sample .ini file

Just using the first mapfile you can find is good enough

Done

When you provide me with some script I can use to scan hundreds of games easily I can tell you quickly

That's not a bad idea. I'll write a quick Python script that can check this stuff.

@sorlok
Copy link
Contributor Author

sorlok commented Jul 26, 2020

Ok, here's a script for scanning. Please be aware: it's very WIP:
https://drive.google.com/file/d/1iv2YHQ_DFXRWgixeojaWUbgxZkzJ7ODl/view?usp=sharing

Currently, in my limited RPG collection, it flags the following:

DIRECTORY                IS-RPG?   STANDARD?      STD-RPGs?      STD-MAPs?      GUESS-RPGs?    GUESS-MAPS?    EASYRPG-OK?    
DDII - MazeHack          rpg       non-standard   standard       non-standard   likely         unlikely       unlikely         
Phylomortis 1            rpg       non-standard   non-standard   non-standard   unlikely       unlikely       unlikely       
LaxiusPower              rpg       non-standard   standard       non-standard   likely         likely         likely         

The "DDII- MazeHack" directory include a MapXXX.rar file that messes this up. I think I'll add map scanning back in, since it shouldn't be too costly.

Laxius Power is good.

Unfortunately, Phylomortis 1 is... problematic. Leave it to RPG Advocate to haunt us from the past:

$ ls Phylomortis\ 1/ | grep dat | sort | head
eve00000.dat
eve00038.dat
eve00039.dat
eve00040.dat
eve00041.dat
eve00042.dat

...yeah, he renamed the entire file. Because he renamed the "RPG_RT" files, we can't even use the Meta file.

Unless you can think of a good heuristic for detecting this (without loading the file), I think I'll have to call this "out of scope" for the current branch.

@sorlok
Copy link
Contributor Author

sorlok commented Jul 26, 2020

Rebased and squashed. This branch is ready for merge (from my point of view --let me know if anything needs to change).

@sorlok sorlok changed the title Snh odd extensions Fix for games with altered file extensions Aug 11, 2020
@sorlok
Copy link
Contributor Author

sorlok commented Aug 11, 2020

Hey all,

Just got back from vacation; wanted to check if there's anything more needed with this branch. (Also if you are all still on vacation that's fine too.) Thanks!

@Ghabry
Copy link
Member

Ghabry commented Aug 15, 2020

I'm back from vacation and currently prepare running your script (extracting all the games takes a while *g*)

Minor patch (recurses in a subdirectory when the subdir only has one folder)

91,96c91
<   dirlist = list(os.listdir(full_dir))
<   dirs = list(filter(lambda x: os.path.isdir(os.path.join(full_dir, x)), dirlist))
<   if len(list(dirs)) == 1:
<     return scan_project(full_dir, dirs[0])
< 
<   for filepath in dirlist:
---
>   for filepath in os.listdir(full_dir):

@Ghabry
Copy link
Member

Ghabry commented Aug 15, 2020

This renaming looks unpopular in Germany.
Some games are molebox encrypted or have corrupted zip paths (\ instead of /) but for ones that work (around 1900): Only 2 are affected by this PR:

@sorlok
Copy link
Contributor Author

sorlok commented Aug 16, 2020

I'm back from vacation and currently prepare running your script (extracting all the games takes a while g)

Minor patch (recurses in a subdirectory when the subdir only has one folder)

91,96c91
<   dirlist = list(os.listdir(full_dir))
<   dirs = list(filter(lambda x: os.path.isdir(os.path.join(full_dir, x)), dirlist))
<   if len(list(dirs)) == 1:
<     return scan_project(full_dir, dirs[0])
< 
<   for filepath in dirlist:
---
>   for filepath in os.listdir(full_dir):

Updated, thanks

@Ghabry
Copy link
Member

Ghabry commented Aug 16, 2020

This needs now a minor rebase

@Ghabry Ghabry added the Awaiting Rebase Pull requests with conflicting files due to former merge label Aug 16, 2020
if(std::regex_search(item.first, m, rgx)) {
std::string ext = m[1].str();
if (ext != "exe" && ext != "ini") {
candidates.push_back({item.second, ext, GetFileSize(item.second)});
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't work when launched from the Gamebrowser (wrong relative path).

Use:

candidates.push_back({item.second, ext, GetFileSize(FileFinder::FindDefault(dir, item.second))});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9e361a9, thanks

@Ghabry
Copy link
Member

Ghabry commented Aug 16, 2020

In my first quick test it worked. Good job :)

@sorlok
Copy link
Contributor Author

sorlok commented Aug 20, 2020

Rebased; I'm running a few quick tests (and waiting on builds).

@fdelapena fdelapena removed the Awaiting Rebase Pull requests with conflicting files due to former merge label Aug 20, 2020
@sorlok
Copy link
Contributor Author

sorlok commented Aug 20, 2020

Tested again on Linux and Android; confirmed it's working.

@fdelapena fdelapena linked an issue Aug 23, 2020 that may be closed by this pull request
Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

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

This is a great addition, thanks!

I also think the two regex are simple enough can be replaced with StartsWith, EndsWith and Substr but there is a PR pending that invents StringView and deprecates some Utils:: functions so better ignore this for now as it will conflict.

if (rpgRTcount == 2) {
return true;
}

Copy link
Member

Choose a reason for hiding this comment

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

Wow. This is bad. Though not your fault: Why is the same function twice in Java o.O.
Could you: Delete the IsRpg2kGame from GameScanner and replace the one function call to it with GameBrowserHelper.isRpg2kGame.
And remove DATABASE_NAME/TREEMAP_NAME. Then you only have to make your change once :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -50,6 +50,9 @@ public static boolean isRpg2kGame(File dir) {
boolean databaseFound = false;
boolean treemapFound = false;

// Create a lookup by extension as we go, in case we are dealing with non-standard extensions.
int rpgRTcount = 0;
Copy link
Member

Choose a reason for hiding this comment

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

naming: rpgrtCount

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/game_map.cpp Outdated
ss << "Map" << std::setfill('0') << std::setw(4) << location.map_id << ".emu";

std::string map_file = FileFinder::FindDefault(ss.str());
std::string mapName = Game_Map::ConstructMapName(location.map_id, true);
Copy link
Member

Choose a reason for hiding this comment

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

map_name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/game_map.cpp Outdated

if (map_file.empty()) {
Output::Error("Loading of Map {} failed.\nThe map was not found.", ss.str());
Output::Error("Loading of Map {} failed.\nThe map was not found.", mapName.c_str());
Copy link
Member

Choose a reason for hiding this comment

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

the c_str() is not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks for the info (I don't know much about this output api).

src/game_map.cpp Outdated
@@ -1703,11 +1701,18 @@ int Game_Map::GetTargetPanY() {
return location.pan_finish_y;
}

FileRequestAsync* Game_Map::RequestMap(int map_id) {
std::string Game_Map::ConstructMapName(int map_id, bool isEasyRpg) {
Copy link
Member

Choose a reason for hiding this comment

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

is_easyrpg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/options.h Outdated
#define DATABASE_NAME "RPG_RT.ldb"
#define DATABASE_NAME_EASYRPG "EASY_RT.edb"
#define DATABASE_NAME RPG_RT_PREFIX "." SUFFIX_LDB
#define DATABASE_NAME_EASYRPG RPG_RT_PREFIX "." SUFFIX_EDB
Copy link
Member

Choose a reason for hiding this comment

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

You changed this from EASY_RT to RPG_RT. (the final name is not decided but this changes the behaviour)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Sorry about that, it was not intentional.

return !GetRPG2kProjectWithRenames(dir).Empty();
}

FileFinder::RPG2KNonStandardFilenameGuesser FileFinder::GetRPG2kProjectWithRenames(DirectoryTree const& dir) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you move the entire guess/remap logic in a new file? My problem here is that I have a huge PR touching FileFinder in the works and such a huge change is terrible to handle for me :(.

No idea. Make a name up: "filefinder_guesser.cpp/h", "file_guesser.cpp/h", "project_guesser.cpp/.h".

Move GetRPG2kProjectWithRenames, GuessAndAddLmuExtension and RPG2KNonStandardFilenameGuesser, RPG2KFileExtRemap to this

IsRPG2kProjectWithRenames can stay as it fits with the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@sorlok
Copy link
Contributor Author

sorlok commented Aug 25, 2020

Changes made and rebased. Tested and works on Linux. I'm still building on Android.

@sorlok
Copy link
Contributor Author

sorlok commented Aug 25, 2020

Android is working great. That's everything on my side.

@Ghabry Ghabry added the Has PR Dependencies This PR depends on another PR label Aug 25, 2020
Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

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

Except for my remaining nits this looks now fine for me.

Do you know how to "squash" in a rebase? If yes please squash the two "Code review" commits into one.

I marked it as "Has PR deps" because I want to merge this after #2293
(that PR changes some string functions and want to attempt a regex -> starts/ends_with change as I expect this to be faster here)


}

#endif
Copy link
Member

Choose a reason for hiding this comment

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

new line at end of file missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

/**
* FilextGuesser contains helper methods for guessing the extensions used on non-standard RPG Projects.
*/
namespace FilextGuesser {
Copy link
Member

Choose a reason for hiding this comment

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

I think there is a typo here. Should be FileExtGuesser and the file fileext_guesser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I was thinking "filext", but I now think that might be trademarked by the associated ".com" address. I'll change this after work.

@sorlok sorlok force-pushed the snh_odd_extensions branch 4 times, most recently from ce89b4b to 2bce9fb Compare August 26, 2020 00:41
@sorlok
Copy link
Contributor Author

sorlok commented Aug 26, 2020

Renamed filext to fileext (and similar).
Switched from regex to string_view.
Rebased, squashed.

Code seems like it's still working (tested loading two games on Linux).

@Ghabry Ghabry removed the Has PR Dependencies This PR depends on another PR label Aug 26, 2020
Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

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

Want to quickly test this but it looks now fine for me! Thanks! Very useful feature :)

The commit message can be improved. Instead of "Squashed commits" can you use something like "Support games with altered file extensions" (you can keep the verbose message part as is)

@@ -26,6 +26,7 @@
#include <string>
#include <vector>
#include <sstream>
#include <regex>
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary include (thanks for changing this 👍 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed (and you're welcome!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also fixed the commit message.

Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

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

I planned to merge it but I just found an important bug (sorry).

When starting a renamed game the output is "Debug: Encoding not detected". This means that for non-English games the language detection is broken (and they won't run). The encoding detection still only uses RPG_RT.ldb.

I think you must reorder the code a bit. The following must happen before "GetEncoding" is called:

  1. Load Meta file
  2. Guess extensions
  3. GetEncoding <- Use inside of this func the name of the guessed database file
  4. [...] stuff
  5. LoadDatabase

@sorlok
Copy link
Contributor Author

sorlok commented Aug 28, 2020

Ah, interesting. Glad you caught this bug before merging! I'm a bit busy this weekend, but I'll check it out next week.

@Ghabry
Copy link
Member

Ghabry commented Sep 1, 2020

About the build failures: That was a very bad timing for updating the PR. There is currently a breaking change from liblcf pending/rebuilding.
You will have to rebase again in ~1h ^^'.

@sorlok
Copy link
Contributor Author

sorlok commented Sep 1, 2020

About the build failures: That was a very bad timing for updating the PR. There is currently a breaking change from liblcf pending/rebuilding.
You will have to rebase again in ~1h ^^'.

No worries; I'm still working on the fix.

@Ghabry Ghabry added the Awaiting Rebase Pull requests with conflicting files due to former merge label Sep 1, 2020
@sorlok
Copy link
Contributor Author

sorlok commented Sep 1, 2020

Ok, the fix is in (tested on Laxius Power and a standard game).

  1. I left it as its own commit for now, since I think that makes it easier to review. Let me know if/when you want it squashed.
  2. Are there any EasyRPG format games I can test this on? I want to make sure I didn't break anything with the new format.

commit 1f15693
Author: Seth N. Hetu <seth.hetu@gmail.com>
Date:   Mon Aug 24 20:10:35 2020 -0400

    Move file extension guessing code into filext_guesser.h/cpp
    (Renamed to fileext_guesser and FileExtGuesser)
    (Also migrated to string_view)

commit 1c1b9ee
Author: Seth N. Hetu <seth.hetu@gmail.com>
Date:   Mon Aug 24 19:40:40 2020 -0400

    Fixes from code review:
    * Remove duplicate isRpg2kGame function in Game_Scanner and use the one in GameBrowserHelper.
    * Removes additional consts added earlier.
    * Fix case for rpgrtCount, is_easy_rpg.
    * Remove unneded c_str() calls
    * Fix Easy_RT prefix

commit 3cf47a5
Author: Seth N. Hetu <seth.hetu@gmail.com>
Date:   Wed Aug 19 21:14:04 2020 -0400

    Fix from code review

commit faf5437
Author: Seth N. Hetu <seth.hetu@gmail.com>
Date:   Sun Jul 26 13:32:58 2020 -0400

    Detection for non-standard LMT, LDB, and LMU files (original)
@Ghabry Ghabry removed the Awaiting Rebase Pull requests with conflicting files due to former merge label Sep 2, 2020
Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

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

This works perfect now. Tested it. Thanks for this great contribution!

@Ghabry Ghabry merged commit 85ea1b1 into EasyRPG:master Sep 2, 2020
@Ghabry
Copy link
Member

Ghabry commented Sep 2, 2020

@sorlok
FYI
Made an announcement: https://twitter.com/EasyRPG/status/1301108541185548288

@sorlok
Copy link
Contributor Author

sorlok commented Sep 2, 2020

Cool, thanks!

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

Successfully merging this pull request may close these issues.

Games with altered file extensions
4 participants