During 2.3 development, there's been a gradual shift to using SDL stdlib
functions instead of libc functions, but there are still some libc
functions (or the same libc function but from the STL) in the code.
Well, this patch replaces all the rest of them in one fell swoop.
SDL's stdlib can replace most of these, but its SDL_min() and SDL_max()
are inadequate - they aren't really functions, they're more like macros
with a nasty penchant for double-evaluation. So I just made my own
VVV_min() and VVV_max() functions and placed them in Maths.h instead,
then replaced all the previous usages of min(), max(), std::min(),
std::max(), SDL_min(), and SDL_max() with VVV_min() and VVV_max().
Additionally, there's no SDL_isxdigit(), so I just implemented my own
VVV_isxdigit().
SDL has SDL_malloc() and SDL_free(), but they have some refcounting
built in to them, so in order to use them with LodePNG, I have to
replace the malloc() and free() that LodePNG uses. Which isn't too hard,
I did it in a new file called ThirdPartyDeps.c, and LodePNG is now
compiled with the LODEPNG_NO_COMPILE_ALLOCATORS definition.
Lastly, I also refactored the awful strcpy() and strcat() usages in
PLATFORM_migrateSaveData() to use SDL_snprintf() instead. I know save
migration is getting axed in 2.4, but it still bothers me to have
something like that in the codebase otherwise.
Without further ado, here is the full list of functions that the
codebase now uses:
- SDL_strlcpy() instead of strcpy()
- SDL_strlcat() instead of strcat()
- SDL_snprintf() instead of sprintf(), strcpy(), or strcat() (see above)
- VVV_min() instead of min(), std::min(), or SDL_min()
- VVV_max() instead of max(), std::max(), or SDL_max()
- VVV_isxdigit() instead of isxdigit()
- SDL_strcmp() instead of strcmp()
- SDL_strcasecmp() instead of strcasecmp() or Win32 strcmpi()
- SDL_strstr() instead of strstr()
- SDL_strlen() instead of strlen()
- SDL_sscanf() instead of sscanf()
- SDL_getenv() instead of getenv()
- SDL_malloc() instead of malloc() (replacing in LodePNG as well)
- SDL_free() instead of free() (replacing in LodePNG as well)
This patch cleans up unnecessary exports from header files (there were
only a few), as well as adds the static keyword to all symbols that
aren't exported and are specific to a file. This helps the linker out in
not doing any unnecessary work, speeding it up and avoiding silent
symbol conflicts (otherwise two symbols with the same name (and
type/signature in C++) would quietly resolve as okay by the linker).
strcat()s and strcpy()s have been replaced with SDL_snprintf() where
possible, to clearly convey the intent of just building a string that
looks a certain way, instead of spanning it out over multiple lines.
Where there's not really a good way to avoid strcat()/strcpy() (e.g. in
PLATFORM_getOSDirectory()), they will at least be replaced with
SDL_strlcat() and SDL_strlcpy(), which are safer functions and are less
likely to have issues with null termination.
I decided not to bother with PLATFORM_migrateSaveData(), because it's
going to be axed in 2.4 anyways.
There's no need to call a string function and have function call
overhead if you remember how C strings work: they have a null
terminator. So if the first char in a string is a null terminator, then
the string is completely empty. So you don't need to call that function.
The previous check by mwpenny had a few issues:
(a) It was completely overcomplicated for no good reason, and was
basically a Rube Goldberg machine. The original check was...
(1) Creating an std::string of the last char of 'output'...
(2) ...except instead of using the normal std::string constructor, it
was using the one where you pass in a number and a char to create
a string that's just that char repeated N times... except this
was only used to create a 1-length string.
(3) Converted that std::string to a C string.
(4) Then passed it to strcmp(), despite the string at this point
being only one byte and you could just compare the char values
directly.
The original check could've just been:
output[SDL_strlen(output) - 1] == *pathSep
(b) Use of libc strcmp() and strlen() instead of SDL_strcmp() and
SDL_strlen().
Now, actually, PHYSFS_getDirSeparator() happens to be a char array and
not a single char, so mwpenny was going in the right direction by using
strcmp() after all. Except it doesn't seem like he thought about the
fact that PHYSFS_getDirSeparator() could be multiple bytes instead of
one, and so he ended up making the first argument to strcmp() always be
a one-byte char array.
So there's issue (c), which is that it assumes the path separator is one
byte instead of multiple.
This commit fixes all of these issues with the trailing path separator
check.
Okay, so basically here's the include layout that this game now
consistently uses:
[The "main" header file, if any (e.g. Graphics.h for Graphics.cpp)]
[blank line]
[All system includes, such as tinyxml2/physfs/utfcpp/SDL]
[blank line]
[All project includes, such as Game.h/Entity.h/etc.]
And if applicable, another blank line, and then some special-case
include screwy stuff (take a look at editor.cpp or FileSystemUtils.cpp,
for example, they have ifdefs and defines with their includes).
Including a header file inside another header file means a bunch of
files are going to be unnecessarily recompiled whenever that inner
header file is changed. So I minimized the amount of header files
included in a header file, and only included the ones that were
necessary (system includes don't count, I'm only talking about includes
from within this project). Then the includes are only in the .cpp files
themselves.
This also minimizes problems such as a NO_CUSTOM_LEVELS build failing
because some file depended on an include that got included in editor.h,
which is another benefit of removing unnecessary includes from header
files.
This is just in case these values happen to be used without being
initialized or anything. I vaguely recall someone reporting an issue
where they didn't have a "Documents" folder on Windows and their level
folder ended up being a garbage path, so it's good to do this.
Some levels (like Unshackled) have decided to manually re-color the
one-way tiles on their own, and us overriding their re-color is not
something they would want. This does mean custom levels with custom
assets don't get to take advantage of the re-color, but it's the exact
same behavior as before, so it shouldn't really matter that much.
I would've liked to specifically detect if a custom tiles.png or
tiles2.png was in play, rather than simply disabling it if any asset was
mounted, but it seems that detecting if a specific file was mounted from
a specific zip isn't really PHYSFS's strong suit.
The assets mounting code was put directly in editorclass::load(), but
now it's in a neat little function so it can be called from multiple
places without having to call editorclass::load().
This removes the TinyXML source files, removes it from CMakeLists.txt,
removes all the includes, and removes the functions
FILESYSTEM_saveTiXmlDocument() and FILESYSTEM_loadTiXmlDocument() (use
FILESYSTEM_saveTiXml2Document() and FILESYSTEM_loadTiXml2Document()
instead).
Additionally I've cleaned up the tinyxml2.h include in FileSystemUtils.h
so that it doesn't actually include tinyxml2.h unnecessarily, meaning a
change to TinyXML2 shouldn't rebuild all files that include
FileSystemUtils.h.
Whenever I compile with -O2, GCC gives me a warning that the return
value of fread() is being ignored. Which is a fair point, it means that
we're not doing proper error handling if something goes wrong. But I'm
also going to check the return value of fwrite() for good measure.
I believe that checking that this number is not equal to length is the
way to catch all errors and output an error message accordingly. I
didn't use ferror() or feof() mostly because I think it takes up too
much code. Also an error from fwrite() only says "Warning" because I
don't think there's much we can do if we don't fully write all bytes to
the intended file.
Previously:
- Linux: xdg-open
- Everything else: open
Now:
- macOS and Haiku: open
- Everything else: xdg-open
This is all according to a comment by leo60228 in PR #203.
The problem here is that we're directly using the C stdio library,
instead of using PHYSFS's stuff. So I've added a function
FILESYSTEM_delete() that does exactly that.
The environment variable SteamTenfoot corresponds with the game running
in Steam Big Picture mode or SteamOS if it is defined. There's a
certification process for both full controller support and Big Picture
mode, and being able to launch a file window in Big Picture mode is an
instant cert failure.
Have to add some includes and put these behind some ifdefs, of course.
I'm pretty sure FreeBSD and OpenBSD and Haiku are POSIX enough that the
"open" command will work on them, too.
I would've loved to make FILESYSTEM_openDirectoryEnabled a simple bool
instead of a function, but I ran into issues with putting it in the
FileSystemUtils header file, so I'll just make it a function and call it
a day.
This fixes a bug where levels in the levels list duplicate if there's an
invalid file (such as a folder) in the levels directory.
It looks like it happens because we don't free the memory if
PHYSFS_readBytes() encounters an error, even though we should. Then we
get into Undefined Behavior territory and end up reusing memory, and
here it just happens that previously, parsing the entire XML document
for each level file was enough to make the loaded file pointer point to
garbage that would fail the metadata check, but if we optimize it so we
don't parse the entire XML document, it starts reusing memory instead.
This is useful for distributions, which may not want to put data.zip in
the same directory as the binary. This can't be distribution-specific
due to the license ("Altered source/binary versions must be plainly
marked as such, and must not be misrepresented as being the original
software.").
This uses utfcpp combined with a custom font, in the form of a PNG and text file. By default, the game acts exactly as it did before; custom fonts can be provided by third parties.
* Add a null terminator to loaded TinyXML files
The TinyXML parse() function expect a C-like string, including terminator.
When xml loading was changed, it loaded the file, but included no such thing.
Thus, we load the file, then reallocate the memory so that we can insert a
null terminator to it, before passing it to parse().
* Tweak TinyXML file loading
Instead of first loading the file content into memory, then reallocate it
to add a null pointer, add an argument to the file load function for whether
to append a null terminator or not, defaulting to false. It still returns the
length without the null pointer in case a length ptr is passed.
The TinyXml functions to load and save files don't properly support
unicode file paths on Windows, so in order to support that properly, I
saw no other option than to do the actual loading and saving via PHYSFS
(or to use the Windows API on Windows and retain doc.LoadFile and
doc.SaveFile on other OSes, but that'd be more complicated and
unnecessary, we already have PHYSFS, right?).
There are two new functions in FileSystemUtils:
bool FILESYSTEM_saveTiXmlDocument(const char *name, TiXmlDocument *doc)
bool FILESYSTEM_loadTiXmlDocument(const char *name, TiXmlDocument *doc)
Any instances of doc.SaveFile(<FULL_PATH>) have been replaced by
FILESYSTEM_saveTiXmlDocument(<VVVVVV_FOLDER_PATH>, &doc), where
<FULL_PATH> included the full path to the saves or levels directory,
and <VVVVVV_FOLDER_PATH> only includes the path relative to the VVVVVV
directory.
When loading a document, a TiXmlDocument used to be created with a full
path in its constructor and doc.LoadFile() would then be called, now a
TiXmlDocument is constructed with no path name and
FILESYSTEM_loadTiXmlDocument(<VVVVVV_FOLDER_PATH>, &doc) is called.
There's now a thin layer of UTF-16 around the WinAPI functions to get
the path to the Documents folder and to create a new directory, so that
account usernames with non-ASCII characters do not result in no VVVVVV
folder being created or used.
The next official VVVVVV build removes 32-bit Linux (like all my other games),
and I need to get rid of the shell script on macOS at some point, so this
basically brings it up to what my other games are doing. Plus, this probably
fixes a bug where someone tries to run their executable away from the root...
Misa
Ethan Lee
Matt Aaldenberg
Pierre-Alain TORET
leo60228
Matt Penny
Fredrik Ljungdahl
Dav999-v
leo60228
Brian Callahan