Ancurio / mkxp

Free Software implementation of the Ruby Game Scripting System (RGSS)
GNU General Public License v2.0
512 stars 130 forks source link

Adapt to physfs 3.0 API #174

Closed carstene1ns closed 6 years ago

carstene1ns commented 6 years ago

Please carefully review this, as I did not thoroughly test this (only did a test run with a game I try porting).

Basically the API changes are to ease error reporting and return early, when the needed file is found. However, it was not always obvious to me, when to return 0 or -1 in the enumeration callbacks.

Ancurio commented 6 years ago

Sent a mail on the physfs list asking for an enum return type to that callback x.x If that idea doesn't go over well, we should at least have it internally, something like

enum {
    PHYSFS_CONTINUE = 1,
    PHYSFS_STOP = 0,
    PHYSFS_ERROR = -1
}

In any case, I'll hold off on merging this until 3.0.0 is released, shouldn't be too long.

Ancurio commented 6 years ago

The API has been adapted, and should be clearer now: https://hg.icculus.org/icculus/physfs/rev/97d3641bfba3

I'll still wait for 3.0.0 though

hanetzer commented 6 years ago

@Ancurio A question regarding physfs on osx, do you have issue grabbing it for lack of a pkg-config file on the apple platform (according to my reading of their CMakeLists.txt)? I had sent an email to the physfs mailing list with a patch to produce pkg-config files on windows (not msvc, though) and apple (for things like homebrew and gentoo prefix). No response, however.

Ancurio commented 6 years ago

@hanetzer What do you mean by "grabbing"? At the time when I wrote the patch that originally brought pkg-config support into PhysicsFS, I didn't know it could be used on OSX.

I didn't author the CMakeLists.txt, so I can't comment on what's going on there OSX-wise..

hanetzer commented 6 years ago

@Ancurio https://hg.icculus.org/icculus/physfs/file/tip/CMakeLists.txt#l261 Unsure, its just this and the next 10 lines indicate pkg-config files are only generated on non-apple 'unix' platforms, so afaik it wouldn't be created on homebrew or when built on a mingw-w64 toolchain

hanetzer commented 6 years ago

@Ancurio basically I mean, can you build on osx with a lack of a physfs.pc file for this line: https://github.com/Ancurio/mkxp/blob/master/CMakeLists.txt#L88

Ancurio commented 6 years ago

@hanetzer I've never built mkxp on OSX, so can't comment

hanetzer commented 6 years ago

@Ancurio ah ok. Its moot point now, dude just merged in my changes to support pkg-config files on mingw-w64 and osx, so it shouldn't be an issue in the future, if it ever was.

carstene1ns commented 6 years ago

@hanetzer: Your patch has been merged now.

Edit: too slow.

queengooborg commented 6 years ago

Seems that 3.0.0 is now officially out since September 27th. Unfortunately, I attempted to compile with your fixes, @carstene1ns, and I have some errors from src/filesystem.cpp, all of them reading as:

error: no matching function for call to 'PHYSFS_enumerate'
                PHYSFS_enumerate(fullPath, cacheEnumCB, d);
                ^~~~~~~~~~~~~~~~
/usr/local/include/physfs.h:2746:17: note: candidate function not viable: no known conversion from 'int (void *, const char *, const char *)' to 'PHYSFS_EnumerateCallback' (aka 'PHYSFS_EnumerateCallbackResult (*)(void *, const char *, const char *)') for 2nd argument
PHYSFS_DECL int PHYSFS_enumerate(const char *dir, PHYSFS_EnumerateCallback c,
                ^

Seems that the PHYSFS_EnumerateCallbackResult type doesn't have a conversion to an int. If it helps, I'm compiling on macOS!

kforney commented 6 years ago

@vinyldarkscratch I'm compiling on Arch Linux, and I get the same errors.

carstene1ns commented 6 years ago

Thanks for the notify, I did not know about the 3.0 release, will update this patch soon. (in the meanwhile my acquaintance @Ghabry had also a look at it https://github.com/Ancurio/mkxp/pull/163#issuecomment-329967823)

carstene1ns commented 6 years ago

Updated to 3.0 and tested with another game, seems fine so far.

queengooborg commented 6 years ago

Just attempted to compile with the new updates, and it works! 😄

kforney commented 6 years ago

Works on Arch as well.

Ancurio commented 6 years ago

Thank you Carsten