Previously, this function had a bug due to failing to account for array
decay. My solution was to just repeat the MAX_PATH again. But in
hindsight I realize that's bad because it hardcodes it, and introduces
the opportunity for an error where we update the size of the original
path but not the size in the function.
So instead, just pass the size through to the function.
FILESYSTEM_loadFileToMemory() dereferenced pointers without checking if
they were valid... I don't know of any cases where they could have been
NULL, but better safe than sorry.
So, the codebase was kind of undecided about who is responsible for
initializing the parameters passed to FILESYSTEM_loadFileToMemory() - is
it the caller? Is it FILESYSTEM_loadFileToMemory()? Sometimes callers
would initialize one variable but not the other, and it was always a
toss-up whether or not FILESYSTEM_loadFileToMemory() would end up
initializing everything in the end.
All of this is to say that the game dereferences an uninitialized
pointer if it can't load a sound effect. Which is bad. Now, I could
either fix that single case, or fix every case. Judging by the title of
this commit, you can infer that I decided to fix every case - fixing
every case means not just all cases that currently exist (which, as far
as I know, is only the sound effect one), but all cases that could exist
in the future.
So, FILESYSTEM_loadFileToMemory() is now guaranteed to initialize its
parameters even if the file fails to be loaded. This is better than
passing the responsibility to the caller anyway, because if the caller
initialized it, then that would be wasted work if the file succeeds
anyway because FILESYSTEM_loadFileToMemory() will overwrite it, and if
the file fails to load, well that's when the variables get initialized
anyway.
My next commit will involve using goto to jump to the end of a function
to initialize the variables to NULL, but that results in a compiler
error if we have initializations in the middle of the function. We might
as well put all declarations at the top of each block anyway, to help
the move to C, so I'm doing this now.
Since the length variable in the STDIN block now overshadows the length
variable in the outer block, I've renamed the length variable in the
block to stdin_length.
This seems to be a comment left by Ethan that he never got around to. So
I did it for him.
What I've done is made it so FileSystemUtils.cpp knows what a binary
blob is, and moved the binary blob loading code directly to
FileSystemUtils.cpp. To do this, I removed the private access modifier
from binaryBlob - I don't think we'll need it, and anyways when we move
to C we can't use it.
Along the way, I also cleaned up the style of the function a bit - the
null termination offset is no longer hardcoded, and the function no
longer mixes code and declarations together in the same block.
I also noticed that when printing all the filenames at the end, a single
invalid header would stop the whole loop instead of just being skipped
over... this seems to be a bug to me, so I've made it so invalid headers
just get skipped over instead of stopping the whole loop.
In FileSystemUtils.h, I used a forward declaration. In hindsight,
incomplete forward declarations should basically always be done in
header files if possible, otherwise this introduces the possibility of
transitive includes - if a file includes this header and it does a full
include, the file is silently able to use the full header, whereas if
it's a forward declaration, then the moment the file tries to use the
full header it fails, and then it's forced to include the full header
for itself. But uh, that's a code cleanup for later.
It's not really used because CreateDirectory doesn't support setting
chmod values, but it does clarify intent of the argument.
Co-authored-by: Ethan Lee <flibitijibibo@gmail.com>
In #52 I fixed VVVVVV not being able to handle filepaths with non-ASCII
characters on Windows. 2f0a0bce4c and
aa5c2d9dc2 reintroduce this problem,
however, by reverting the definition of mkdir to how it was before the
fix and using the non-Unicode version of CreateDirectory. And I can
confirm that VVVVVV indeed doesn't make its folder anymore with a
Windows username of "тест". This commit fixes that issue.
PhysFS requires a write dir to create a directory, so the first PHYSFS_mkdir
never could have worked. Because of that we need to go back to the old mkdir,
and since we're bringing that back we can reuse it for saves/levels, because we
know it works and we don't have to worry about middlewares ruining anything.
This fixes an issue where you would be able to mount things other than
custom assets in per-level custom asset directories and zips.
To be fair, the effects of this issue were fairly limited - about the
only thing I could do with it was to override a user-made quicksave of a
custom level with one of my own. However, since the quicksave check
happens before assets are mounted, if the user didn't have an existing
quicksave then they wouldn't be able load my quicksave. Furthermore,
mounting things like settings.vvv simply doesn't work because assets
only get mounted when the level gets loaded, but the game only reads
from settings.vvv on startup.
Still, this is an issue, and just because it only has one effect doesn't
mean we should single-case patch that one effect only. So what can we
do?
I was thinking that we should (1) mount custom assets in a dedicated
directory, and then from there (2) mount each specific asset directly -
namely, mount the graphics/ and sounds/ folders, and mount the
vvvvvvmusic.vvv and mmmmmm.vvv files. For (1), assets are now mounted at
a (non-existent) location named .vvv-mnt/assets/. However, (2) doesn't
fully work due to how PhysFS works.
What DOES work is being able to mount the graphics/ and sounds/ folders,
but only if the custom assets directory is a directory. And, you
actually have to use the real directory where those graphics/ and
sounds/ folders are located, and not the mounted directory, because
PHYSFS_mount() only accepts real directories. (In which case why bother
mounting the directory in the first place if we have to use real
directories anyway?) So already this seems like having different
directory and zip mounting paths, which I don't want...
I tried to unify the directory and zip paths and get around the real
directory limitation. So for mounting each individual asset (i.e.
graphics/, sounds/, but especially vvvvvvmusic.vvv and mmmmmm.vvv), I
tried doing PHYSFS_openRead() followed by PHYSFS_mountHandle() with that
PHYSFS_File, but this simply doesn't work, because PHYSFS_mountHandle()
will always create a PHYSFS_Io object, and pass it to a PhysFS internal
helper function named openDirectory() which will only attempt to treat
it as a directory if the PHYSFS_Io* passed is NULL. Since
PHYSFS_mountHandle() always passes a non-NULL PHYSFS_Io*,
openDirectory() will always treat it like a zip file and never as a
directory - in contrast, PHYSFS_mount() will always pass a NULL
PHYSFS_Io* to openDirectory(), so PHYSFS_mount() is the only function
that works for mounting directories.
(And even if this did work, having to keep the file open (because of the
PHYSFS_openRead()) results in the user being unable to touch the file on
Windows until it gets closed, which I also don't want.)
As for zip files, PHYSFS_mount() works just fine on them, but then we
run into the issue of accessing the individual assets inside it. As
covered above, PHYSFS_mount() only accepts real directories, so we can't
use it to access the assets inside, but then if we do the
PHYSFS_openRead() and PHYSFS_mountHandle() approach,
PHYSFS_mountHandle() will treat the assets inside as zip files instead
of just mounting them normally!
So in short, PhysFS only seems to be able to mount directories and zip
files, and not any loose individual files (like vvvvvvmusic.vvv and
mmmmmm.vvv). Furthermore, directories inside directories works, but
directories inside zip files doesn't (only zip files inside zip files
work).
It seems like our asset paths don't really work well with PhysFS's
design. Currently, graphics/, sounds/, vvvvvvmusic.vvv, and mmmmmm.vvv
all live at the root directory of the VVVVVV folder. But what would work
better is if all of those items were organized into a subfolder, for
example, a folder named assets/. So the previous assets mounting system
before this patch would just have mounted assets/ and be done with it,
and there would be no risk of mounting extraneous files that could do
bad things. However, due to our unorganized asset paths, the previous
system has to mount assets at the root of the VVVVVV folder, which
invites the possibility of those extraneous bad files being mounted.
Well, we can't change the asset paths now, that would be a pretty big
API break (maybe it should be a 2.4 thing). So what can we do?
What I've done is, after mounting the assets at .vvv-mnt/assets/, when
the game loads an asset, it checks if there's an override available
inside .vvv-mnt/assets/, and if so, the game will load that asset
instead of the regular one. This is basically reimplementing what PhysFS
SHOULD be able to do for us, but can't. This fixes the issue of being
able to mount a quicksave for a custom level inside its asset directory.
I should also note, the unorganized asset paths issue also means that
for .zip files (which contain the level file), the level file itself is
also technically mounted at .vvv-mnt/assets/. This is harmless (because
when we load a level file, we never load it as an asset) but it's still
a bit ugly. Changing the asset paths now seems more and more like a good
thing to do...
This will clarify which directory, exactly, failed to mount. I know it
gets printed earlier in the mounting process, but it can't hurt to print
it twice, just to be sure. Also this is for consistency.
PHYSFS_getDirSeparator() already gets called and stored in pathSep at
the top of FILESYSTEM_init(). So clearly, two people worked on this
function and forgot that both pieces of code existed at the same time
(or it was one person independently forgetting both).
PhysFS uses platform-independent notation, so we really don't need to
care about getting the correct dir separator here. Especially since we
don't ever do so anywhere else (e.g. load/saveTiXml2Document()), either.
This is to make it clear that this is not a general-purpose mounting
function; it is a helper function for FILESYSTEM_mountAssets()
specifically for treating a directory or file as an assets directory,
and mounting assets from there.
There's no reason to handle mounting .zip files differently than
mounting a directory... we already mount .data.zip files using
FILESYSTEM_mount(), so why go through the trouble of opening a .zip
manually (which means on Windows the .zip can't be touched for the
duration of playing the custom level), making up a place to mount it at,
and then mount that made-up name, instead of just using
FILESYSTEM_mount()?
Whoever cobbled this asset mounting thing together really didn't fully
understand what they were doing.
This way, we avoid the unnecessary graphics.reloadresources() call - if
we can't mount assets, why bother reloading resources?
The return type of FILESYSTEM_mount() has been changed from void to bool
to indicate success, accomodating its callers accordingly.
Ethan, you forgot this other one.
I do have to rejiggle the control flow of the function a bit, so it
doesn't leak memory upon failure. (Although the SDL message box leaks
memory anyway because of X11 so... whatever.) Also, there's a NULL check
for if SDL_GetBasePath() fails now.
So, 2.3 added recoloring one-way tiles to no longer make them be always
yellow. However, custom levels that retexture the one-way tiles might
not want them to be recolored. So, if there are ANY custom assets
mounted, then the one-ways will not be recolored. However, if the XML
has a <onewaycol_override>1</onewaycol_override> tag, then the one-way
will be recolored again anyways.
When I added one-way recoloring, I didn't intend for any custom asset to
disable the recoloring; I only did it because I couldn't find a way to
check if a specific file was customized by the custom level or not.
However, I have figured out how to do so, and so now tiles.png one-way
recolors will only be disabled if there's a custom tiles.png, and
tiles2.png one-way recolors will only be disabled if there's a custom
tiles2.png.
In order to make sure we're not calling PhysFS functions on every single
deltaframe, I've added caching variables, tiles1_mounted and
tiles2_mounted, to Graphics; these get assigned every time
reloadresources() is called.
This function will check if a specific file is a mounted per-level
custom asset, instead of being a variable that's true if ANY file is a
mounted asset.
It's only used in FileSystemUtils and never anywhere else, especially
not Graphics. Why is this on Graphics again?
It's now a static variable inside FileSystemUtils. It has also been
renamed to assetDir for consistency with saveDir and levelDir. Also,
it's a C string now, and is no longer an STL string.
This returns if the file given is mounted or not. 2.3 added level zip
support, so whenever the game loads level metadata, it will mount any
zip files in the levels directory; this function can be used to check if
any of those files have been mounted, and ignore them if so.
This function will be used to differentiate files from directories.
Or at least that was the hope. Symlink support was added in 2.3, but it
doesn't seem like PHYSFS_stat() lets you follow the symlink to check if
what it points to is itself a file or directory. And there doesn't seem
to be any function to follow the symlink yourself...
So for now, this function considers symlinks to directories to be files.
PHYSFS_readBytes() returns a PHYSFS_sint64, but we forcefully shove it
into a 32-bit signed integer.
Fixing the type of this doesn't have any immediate consequences, but
it's good for the future in case we want to use the return value for
files bigger than 2 gigabytes; it doesn't harm us in any way, and it's
just better housekeeping.
PHYSFS_fileLength() returns -1 if the file size can't be determined. I'm
going to set it to 0 instead, because it seems like that's more
well-behaved with consumers.
Take lodepng_decode24() or lodepng_decode32(), for example - from a
quick glance at the source, it only takes in a size_t (an unsigned
integer) for the filesize, and one of the first things it does is malloc
with the given filesize. If the -1 turns into SIZE_MAX and LodePNG
attempts to allocate that many bytes... well, I don't know of any
systems that have 18 exabytes of memory. So that seems pretty bad.
The function returns a PHYSFS_sint64, but we forcefully shove it into a
PHYSFS_uint32. This means we throw away all the negative numbers, which
is bad because the function returns -1 if the size of the file can't be
determined; plus, we also throw away 32 bits of information, reducing
our range of supported file sizes from 9 exabytes to 4 gigabytes.
File size support is only as good as the weakeast link, and it looks
like one of the consumers of FILESYSTEM_loadFileToMemory(),
SDL_RWFromConstMem(), only takes in a signed 32-bit integer of size;
however, I would still like to do at least the bare minimum to support
as many file sizes as we can, and changing types around is one of those
bare minimums.
After reasoning about it for a bit, there's no reason for these checks
to be here. `zip_normal` will either be
/home/infoteddy/.local/share/VVVVVV/levels if the asset directory is a
directory, or levels/levelname.zip if the asset directory is inside the
same zip as the level is. I don't see how they could ever be data.zip.
My guess is because of the VCE bug where it messed up its search path,
and before that bug was fixed, it had to be worked around here by
explicitly blacklisting data.zip here. When the assets mounting stuff
was ported from VCE to vanilla, vanilla didn't have the problem, and so
this data.zip blacklisting stuff was unnecessary.
Either way, I see no reason for this, so I'm going to remove it.
There is no need to use heap-allocated strings here, so I've refactored
them out. I've also cleaned up both of the functions a bit, because the
line spacing of the previous version was completely non-existent, brace
style was same-line instead of next-line, and the variable names were a
bit misleading (in FILESYSTEM_mountassets(), there is a `zippath` AND a
`zip_path`, which are two completely different variables).
Also, FILESYSTEM_mount() now prints an error message and bails if
PHYSFS_getRealDir() returns NULL, whereas it didn't do that before.
The function is literally just an alias for PHYSFS_exists(), which does
not exclusively check for directories. Plus, the function is also used
to check if a non-directory file exists. Why is this function named
"directoryExists"?!
The info message when a .data.zip file is mounted is now differentiated
from the message when an actual directory is mounted (the .data.zip
message specifies ".data.zip").
The error message for an error occurring when loading or mounting a .zip
is now capitalized.
The "Custom asset directory does not exist" now uses puts(), because
there's no need to use printf() here.
There's not really any reason for this function to use heap-allocated
strings. So I've refactored it to not do that.
I would've used SDL_strrstr(), if it existed. It does not appear to
exist. But that's okay.
PhysFS by default just uses system malloc(), realloc(), and free(); it
provides a way to change them, with a struct named PHYSFS_Allocator and
a function named PHYSFS_setAllocator().
According to PhysFS docs, this function should be called before
PHYSFS_init(), which is why this allocator stuff is handled in
FileSystemUtils.cpp.
Also, I've had to make two "bridge" functions, because PHYSFS_Allocator
wants pointers to functions taking in `PHYSFS_uint64`s, not `size_t`s.
Apparently in C, if you have `void test();`, it's completely okay to do
`test(2);`. The function will take in the argument, but just discard it
and throw it away. It's like a trash can, and a rude one at that. If you
declare it like `void test(void);`, this is prevented.
This is not a problem in C++ - doing `void test();` and `test(2);` is
guaranteed to result in a compile error (this also means that right now,
at least in all `.cpp` files, nobody is ever calling a void parameter
function with arguments and having their arguments be thrown away).
However, we may not be using C++ in the future, so I just want to lay
down the precedent that if a function takes in no arguments, you must
explicitly declare it as such.
I would've added `-Wstrict-prototypes`, but it produces an annoying
warning message saying it doesn't work in C++ mode if you're compiling
in C++ mode. So it can be added later.
One of these days, I need to get around to running Include What You Use
on this codebase. Until then, while I was working on #624, I noticed
these; I'm removing them now.
The recently released SDL 2.0.14 adds a native function for opening URIs
from the host system, superseding the OS-specific implementations of
FILESYSTEM_openDirectory.
If PHYSFS_enumerate() isn't successful, we now print that it wasn't
successful, and print the PhysFS error message. (We should get that
logging thing going sometime...)
Note that level dir listing still uses plenty of STL (including the end
product - the `LevelMetaData` struct - which, for the purposes of 2.3,
is okay enough (2.4 should remove STL usage entirely)); it's just that
the initial act of iterating over the levels directory no longer takes
four or SIX(!!!) heap allocations (not counting reallocations and other
heap allocations this patch does not remove), and no longer does any
data marshalling.
Like text splitting, and binary blob extra indice grabbing, the current
approach that FILESYSTEM_getLevelDirFileNames() uses is a temporary
std::vector of std::strings as a middleman to store all the filenames,
and the game iterates over that std::vector to grab each level metadata.
Except, it's even worse in this case, because PHYSFS_enumerateFiles()
ALREADY does a heap allocation. Oh, and
FILESYSTEM_getLevelDirFileNames() gets called two or three times. Yeah,
let me explain:
1. FILESYSTEM_getLevelDirFileNames() calls PHYSFS_enumerateFiles().
2. PHYSFS_enumerateFiles() allocates an array of pointers to arrays of
chars on the heap. For each filename, it will:
a. Allocate an array of chars for the filename.
b. Reallocate the array of pointers to add the pointer to the above
char array.
(In this step, it also inserts the filename in alphabetically -
without any further allocations, as far as I know - but this is a
COMPLETELY unnecessary step, because we are going to sort the list
of levels by ourselves via the metadata title in the end anyways.)
3. FILESYSTEM_getLevelDirFileNames() iterates over the PhysFS list, and
allocates an std::vector on the heap to shove the list into. Then,
for each filename, it will:
a. Allocate an std::string, initialized to "levels/".
b. Append the filename to the std::string above. This will most
likely require a re-allocation.
c. Duplicate the std::string - which requires allocating more memory
again - to put it into the std::vector.
(Compared to the PhysFS list above, the std::vector does less
reallocations; it however will still end up reallocating a certain
amount of times in the end.)
4. FILESYSTEM_getLevelDirFileNames() will free the PhysFS list.
5. Then to get the std::vector<std::string> back to the caller, we end
up having to reallocate the std::vector again - reallocating every
single std::string inside it, too - to give it back to the caller.
And to top it all off, FILESYSTEM_getLevelDirFileNames() is guaranteed
to either be called two times, or three times. This is because
editorclass::getDirectoryData() will call editorclass::loadZips(), which
will unconditionally call FILESYSTEM_getLevelDirFileNames(), then call
it AGAIN if a zip was found. Then once the function returns,
getDirectoryData() will still unconditionally call
FILESYSTEM_getLevelDirFileNames(). This smells like someone bolting
something on without regard for the whole picture of the system, but
whatever; I can clean up their mess just fine.
So, what do I do about this? Well, just like I did with text splitting
and binary blob extras, make the final for-loop - the one that does the
actual metadata parsing - more immediate.
So how do I do that? Well, PhysFS has a function named
PHYSFS_enumerate(). PHYSFS_enumerateFiles(), in fact, uses this function
internally, and is basically just a wrapper with some allocation and
alphabetization.
PHYSFS_enumerate() takes in a pointer to a function, which it will call
for every single entry that it iterates over. It also lets you pass in
another arbitrary pointer that it leaves alone, which I use to pass
through a function pointer that is the actual callback.
So to clarify, there are two callbacks - one callback is passed through
into another callback that gets passed through to PHYSFS_enumerate().
The callback that gets passed to PHYSFS_enumerate() is always the same,
but the callback that gets passed through the callback can be different
(if you look at the calling code, you can see that one caller passes
through a normal level metadata callback; the other passes through a zip
file callback).
Furthermore, I've also cleaned it up so that if editorclass::loadZips()
finds a zip file, it won't iterate over all the files in the levels
directory a third time. Instead, the level directory only gets iterated
over twice - once to check for zips, and another to load every level
plus all zips; the second time is when all the heap allocations happen.
And with that, level list loading now uses less STL templated stuff and
much less heap allocations.
Also, ed.directoryList basically has no reason to exist other than being
a temporary std::vector, so I've removed it. This further decreases
memory usage, depending on how many levels you have in your levels
folder (I know that I usually have a lot and don't really ever clean it
up, lol).
Lastly, in the callback passed to PhysFS, `builtLocation` is actually no
longer hardcoded to just the `levels` directory, since instead we now
use the `origdir` variable that PhysFS passes us. So that's good, too.
