Just like is_positive_num(), an empty string is not a number.
I've also decided to unroll iteration 0 of the loop here so readability
is improved; this happens to also knock out the whole "accepting empty
string" thing, too.
To account for empty strings, we simply have to special-case them.
Simple as that.
This was also a problem with the previous std::string implementation of
this function; regardless, this is fixed now.
The current way "arrays" from XML files are loaded (before this commit
is applied) goes something like this:
1. Read the buffer of the contents of the tag using TinyXML-2.
2. Allocate a buffer on the heap of the same size, and copy the
existing buffer to it. (This is what the statement `std::string
TextString = pText;` does.)
3. For each delimiter in the heap-allocated buffer...
a. Allocate another buffer on the heap, and copy the characters from
the previous delimiter to the delimiter you just hit.
b. Then allocate the buffer AGAIN, to copy it into an std::vector.
4. Then re-allocate every single buffer YET AGAIN, because you need to
make a copy of the std::vector in split() to return it to the caller.
As you can see, the existing way uses a lot of memory allocations and
data marshalling, just to split some text.
The problem here is mostly making a temporary std::vector of split text,
before doing any actual useful work (most likely, putting it into an
array or ANOTHER std::vector - if the latter, then that's yet another
memory allocation on top of the memory allocation you already did; this
memory allocation is unavoidable, unlike the ones mentioned earlier,
which should be removed).
So I noticed that since we're iterating over the entire string once
(just to shove its contents into a temporary std::vector), and then
basically iterating over it again - why can't the whole thing just be
more immediate, and just be iterated over once?
So that's what I've done here. I've axed the split() function (both of
them, actually), and made next_split() and next_split_s().
next_split() will take an existing string and a starting index, and it
will find the next occurrence of the given delimiter in the string. Once
it does so, it will return the length from the previous starting index,
and modify your starting index as well. The price for immediateness is
that you're supposed to handle keeping the index of the previous
starting index around in order to be able to use the function; updating
it after each iteration is also your responsibility.
(By the way, next_split() doesn't use SDL_strchr(), because we can't get
the length of the substring for the last substring. We could handle this
special case specifically, but it'd be uglier; it also introduces
iterating over the last substring twice, when we only need to do it
once.)
next_split_s() does the same thing as next_split(), except it will copy
the resulting substring into a buffer that you provide (along with its
size). Useful if you don't particularly care about the length of the
substring.
All callers have been updated accordingly. This new system does not make
ANY heap allocations at all; at worst, it allocates a temporary buffer
on the stack, but that's only if you use next_split_s(); plus, it'd be a
fixed-size buffer, and stack allocations are negligible anyway.
This improves performance when loading any sort of XML file, especially
loading custom levels - which, on my system at least, I can noticeably
tell (there's less of a freeze when I load in to a custom level with
lots of scripts). It also decreases memory usage, because the heap isn't
being used just to iterate over some delimiters when XML files are
loaded.
These comments were probably remnants of some late-night coding session
or something. Anyway, they're not needed; there's nothing to do with SDL
here, and the "Init" is obvious because the function is a constructor.
Contents and scripts should be reset in editorclass::reset(); there's no
reason to reset them again right before you load them from an XML file
in editorclass::load().
Additionally, the resets now consistently use SDL_zeroa() (for contents)
and scriptclass::clearcustom() (for scripts).
I'm partial to slash-asterisk-style comments, so I'll use those here.
Also, having a space after the start of comments is good. I've also
removed the "Add the script if we have a preceding header" comments
since it can be inferred by reading the surrounding code.
Instead of checking the length() of an std::string, just check if
pText[0] is equal to '\0'.
This will have to be done anyway, because I'm going to get rid of the
std::string allocation here, and I noticed this inefficiency in the
indentation, so I'm going to remove it.
The actual unindent will be done in the next commit.
This now means every XML array loading is done with common,
re-duplicated code. The only exceptions to this are special cases other
than the the majority of cases; the majority being a simple matter of
reading an array of integers and putting it into another array.
Seems like the only reason I hadn't caught the <customlevelscore> tag
until now was because I was focused on de-duplicating all the array
loads in Game::loadstats() and below, forgetting about
Game::loadcustomlevelstats().
In order to be able to use the LOAD_ARRAY() and LOAD_ARRAY_RENAME()
macros in Game::loadcustomlevelstats(), they have to be moved to earlier
in the file.
Even if split() didn't use the STL, using this function here is a bit
unnecessary, because a simple SDL_strchr() suffices. Refactoring split()
to not use the STL will break this caller anyway, so I might as well
just refactor this to not use split() in the first place.
This refactor also properly checks if the inputs are valid integers. And
since split() is no longer used, it also rejects inputs ending with a
trailing comma as being invalid, too; this didn't happen previously.
It's intentional that I used is_number() here instead of
is_positive_num(), thus accepting negative numbers; in the future it
might be possible to have negative room coordinates.
Valgrind reported this.
The error here is that the buffer here is only guaranteed to be
initialized up until (and including) the null-terminator, by
SDL_snprintf(). Iterating over the entire allocated buffer is bad and I
should feel bad as the girl who wrote this code; doing that reads
uninitialized memory and passes it to SDL_tolower().
As a bonus, the iterator increment is now a preincrement instead of a
postincrement.
This fixes memory leaking every single time a file gets loaded(!) when
the list of custom levels gets loaded(!!!), which Valgrind reports. This
memory leak is completely my bad; 2.2 properly frees the loaded file,
and VCE uses an std::unique_ptr - which I decided to ignore and not
think about why it would be there.
It's safe to do this free after uMem gets copied into std::string;
although, in the future, I *am* thinking about refactoring this function
(and the tag finder function) to not use std::strings, and I'll have to
be careful to make sure that the memory management with the file is
correct when I do so.
This makes the freesrc argument of Mix_LoadMUS_RW() 1 instead of 0. If
the argument is nonzero, then the passed SDL_RWops will be automatically
freed when m_music is freed, too.
I don't know why this was 0 before. Setting it to 1 fixes a memory leak
that Valgrind reports (which turns into an actual leak every time custom
assets are mounted or unmounted).
This adds a check that the pointer passed to
FILESYSTEM_loadFileToMemory() isn't NULL, and if it is, just returns
early in the function, instead of continuing later and producing a
different, slightly-misleading error message.
Previously, it was guarded behind a check for the length, which is... I
guess still perfectly fine behavior, but there's no reason to have a
length check here; FILESYSTEM_freeMemory() uses SDL_free(), which does a
check that the pointer passed is non-NULL (the pointer that is passed
here, despite not being initialized upon declaration, is guaranteed to
be initialized by FILESYSTEM_loadFileToMemory() anyway, so).
Following Ethan's example of bailing (calling VVV_exit()) if
binaryBlob::unPackBinary() couldn't allocate memory, I've searched
through and found every SDL_malloc(), then made sure that if it returned
NULL, the caller would bail (because you can't do much when you're out
of memory).
There should probably be an error message printed when the process is
out of memory, but unPackBinary() doesn't print an error message for
being out of memory, so this can probably be added later. (Also we don't
really have a logging system, I'd like to have something like that added
in first before adding more messages.)
Also, this doesn't account for any allocators used by STL stuff, but
we're working on removing the STL, and allocation failure just results
in an abort anyway, so there's not really a problem there.
Wow, there are a lot of these. All of these exit paths now use
VVV_exit() instead, which attempts to save unlock.vvv and settings.vvv,
and also frees all resources so Valgrind is happy. This is a good thing,
because previously unlock.vvv/settings.vvv wouldn't be written to if we
decided to bail for a given reason.
It should be between the include of the corresponding header file for
the source file (Script.h) and the includes of other local header files
(the files that are specific to this codebase only); this is the
convention that includes in all other source files follow.
However, it seems like I misplaced this, so I'm fixing it now.
This is just a function that calls the cleanup() in main.cpp, as well as
calls exit().
I would have liked to use SDL_ExitProcess() here, because that function
has ifdefs for different runtime environments. But alas, it's an
internal function and isn't exported. Ah well; exit() seems to be fine
anyway.
If there's a resource that doesn't otherwise need to be cleaned up and
is still alive upon program shutdown, then it should go in cleanup().
This cleans up Screen, GraphicsResources, Graphics buffers, Graphics
tiles, and musicclass audio upon program shutdown.
Even we technically don't NEED to clean these resources up ourselves
(the kernel is going to get rid of all of it anyway, else it'd be a
security problem), I'm doing this because otherwise Valgrind will
complain about these, and then it'd be difficult to see which memory
leaks are real and which are just "well this isn't really a leak but you
haven't freed this thing when the process exited, and that's technically
what a memory leak is".
These are all resources whose cleanup functions can be safely called
even if they haven't initialized anything yet.
This isn't a memory leak (not even Valgrind complains), because it gets
properly cleaned up in GraphicsResources::destroy(). Still, it's memory
that is just laying around not being used, and in the name of
deallocating things as soon as you no longer need them, we should
deallocate the base tilesheet images after we split all of them into
tiles.
This reduces the memory cost of all tilesheet images by half, since we
were essentially keeping around duplicates for nothing; this doesn't
really have much of an impact with conventional tilesheet sizes, since
they're usually small enough, but since 2.3 allowed for tilesheet images
of any size, this is a pretty big deal for really big tilesheet images.
It's okay to do this, even though they also get freed in
GraphicsResources::destroy(), because SDL_FreeSurface() does a NULL
check on the pointer passed to it, and we set the pointer to NULL after
freeing the surfaces.
A quick glance at PhysFS source code will show that PhysFS will bail if
PHYSFS_deinit() is called if it's not initialized.
"Bail" here just means setting an error code and returning early, so
it's not that bad. Still, it's the principle of the thing, and I just
want to ensure that FILESYSTEM_deinit() can be safely called no matter
if the filesystem hasn't initialized yet; having an error set by PhysFS
kind of taints the whole safety thing, even if it does nothing wrong,
no?
(although, speaking of which, we should be handling all errors by
PhysFS, but that's for later...)
These FIXME comments are still correct about code duplication, but
they're incorrect about where exactly the original code is after the
original code got moved around. So I've fixed them to refer to the
correct locations.
We really should get around to de-duplicating the code mentioned in
these comments...
Since musicWriteBlob is a temporary object that gets destroyed at the
end of musicclass::init(), in order to make sure we don't leak memory
and lose all the pointers to the blocks we just allocated in
musicWriteBlob, we need to call its clear() method after writing
BinaryMusic.vvv.
musicReadBlob was used for both MMMMMM and PPPPPP soundtracks. This
causes a memory leak if you have mmmmmm.vvv installed, because the
pointers holding each allocated block of MMMMMM would be lost when
PPPPPP got loaded. Valgrind complains about this memory leak.
This is in contrast to 2.2 and previous behavior, where musicReadBlob
was only a temporary object instead of being held in musicclass.
However, this wasn't really a memory leak (moreso something that just
didn't get cleaned up when closing the game), but it did get turned into
a leak when per-level assets mounting and unmounting got introduced in
2.3 (loading a level with custom assets after starting the game with an
mmmmmm.vvv, or exiting out of a level that had an mmmmmm.vvv, would
cause the game to leak memory). Leo recognized this, and moved
musicReadBlob onto musicclass in a separate 2.3 PR, but either he didn't
think about what was happening here too closely, or he didn't use
Valgrind, because he forgot about the memory leak caused by re-using the
same binaryBlob for PPPPPP and MMMMMM.
So instead, just use two different binaryBlob objects for MMMMMM and
PPPPPP. That way, no memory leaks happen.
I'm going to introduce another binaryBlob object in to the mix, and I
want to be able to re-use an existing FOREACH_TRACK #define without
having to copy-paste it again. So, TRACK_NAMES now takes in a blob
parameter, which will be passed to the temporary FOREACH_TRACK #define.
This removes the music cleanup code from musicclass::init(), and
requires that we also call destroy() in Graphics::reloadresources().
This is because we'll need to re-use the musicclass cleanup code
elsewhere, and we don't want to copy-paste the cleanup code. Or at
least, I don't (but I'm not a game dev, game devs copy-paste all the
friggin' time).
It doesn't feel quite write leaving all the buffer creation code in
main(), even though it's perfectly okay to do so and it doesn't result
in any memory mismanagement that Valgrind can report; so I'm factoring
all of it out to a separate function, Graphics::create_buffers().
As a bonus, we no longer have to keep qualifying with `graphics.` in the
buffer creation code, which is nice.
These destroy all the buffers that are created on the Graphics class.
Since these buffers can't be created at the same time as the rest of
Graphics is (due to the fact that they require knowing the pixel format
of the game screen), they can't be destroyed at the same as the rest of
Graphics is, either.
This is a very complicated way of zeroing out grphx (instead of using
SDL_zero()), which itself is completely unnecessary because grphx.init()
gets called immediately afterwards anyway.
It should be next-line brace, not same-line brace. Even in a codebase
that uses same-line braces everywhere, I still prefer having next-line
braces inside functions (because they're at the top level, and you can't
next them). But regardless, this should still be next-line brace like
(most of) the rest of the codebase.
The function previously conditionally freed a m_memblocks pointer if its
corresponding m_headers was valid. This makes me slightly worried about
the possibility that memory would be allocated, but the header would
still be marked as invalid.
I don't see how that could happen, but it's better to be safe than
sorry. SDL_free() does a guaranteed NULL pointer check (like most SDL
functions), so it's okay to pass NULL pointers to it.
Just to be sure, I'm also zeroing m_memblocks and m_headers after
freeing everything in the function.
MSVC complains about these, doesn't seem like GCC does. These can be
safely removed because they're unreachable, and they always follow a
case-switch or similar that has a default case which this code is a
duplicate of anyway. (Unless it isn't, in which case all the better to
remove it, becausee otherwise it looks misleading or confusing to casual
glances at the code.)
find_tag() would commit out-of-bounds indexing if someone made a level
file with malformed XML entity encodings in the metadata tags.
This would happen if the end of the string followed immediately after an
ampersand and hash, or if there wasn't a semicolon ending an XML entity.
Valgrind complains about these, so I've fixed it.
This fixes a bug where "12" gets properly evaluated as 12, but "148"
gets evaluated as 1408. It's because `place` gets multiplied by `radix`
again, so `retval` gets multipled by 100 instead of 10.
There's no reason to have a `place` variable, so I've removed it
entirely. This simplifies the function a little bit.
The previous person who wrote this (a girl named Misa) clearly didn't
understand the reason why you couldn't compare line[line.length()-1]
directly to a string literal. It's because the former is a char, and the
latter is a pointer to a char. Both are ints, so it compiles fine, but
it doesn't do what you want it to.
Why not just make the latter a char instead of a string literal? Well,
because you can, but also I clearly didn't think this through earlier,
so that's why I didn't do it in the first place.
But this is fixed now.
This avoids an unnecessary copy of the input std::vector, since we don't
need to modify it for anything. This cuts down on unnecessary memory
operations.
Apart from the std::string, this function no longer uses the STL.
ss_toi() is a simple function - it converts the input into an int,
taking as many digits as possible until it reaches a non-digit
character, at which point it stops. It's trivial to implement this
without the STL.
I could've used Int() here, but that would've required copying the
string to a temporary buffer to insert a null-terminator (we can't just
use a pointer-and-length data type either, the string functions don't
operate like that - one disadvantage of C strings!). Instead, I decided
to implement my own conversion to int here, because I don't think the
way we humans write our Arabic numerals is going to change anytime soon.
Also, the std::string input is now passed by const reference, instead of
making a copy - cutting down on unnecessary memory operations.
I personally like putting the asterisk with the type, because despite
the language parsing the asterisk as a part of the name, the pointer
part is clearly a part of the return type of the function. Also,
put constness here, to indicate that the input won't be modified inside
the function.
This comment indicates that the function is used by
UtilityClass::GCString(). Which is unnecessary, because the reader can
trivially search for usages of GCChar in the file itself (the 'static'
preceding the function should be a good enough hint) - and if there
aren't any, then the reader will know the function is unused, whereas if
they read the comment, they would have been under the assumption that it
wasn't used. (There might also a compiler warning about it being unused,
which would be more confusing if the comment was still there.)
Point is, comments can get outdated, so removing the comment here makes
the code more self-documenting.
This is a re-do of 942217f871 (#509), but
with a more conservative fix that only resets the player's newxp and
newyp when they respawn from a checkpoint or spawn in to the map.
Unlike the previous patch, if the player were to suddenly collide with a
conveyor or horizontally-moving platform during gameplay, their
y-position would revert back to the intended next y-position of the
previous frame. But this is the same behavior as before, I haven't ever
seen such a contrived situation come up, and this behavior is probably
more preferable for gameplay than actually going to the conveyor, so
it's fine.
I also decided to reset newxp here, and not just newyp, because while
resetting newyp seems to be enough, it's safer to also reset newxp (and
so future readers won't question why only newyp is reset but not newxp).
I tested this and it once again fixes the death loop issue from earlier,
while also still allowing for that Trench Warfare trick to be possible
(I tested it with the libTAS movie I mentioned in #606; it syncs fine).
There are no other known regressions resulting from this fix
(hopefully).
This reverts commit 942217f871.
This fix (of a regression of a fix) has a regression where immediately
flipping off of horizontally-moving platforms or conveyors will no
longer provide you with a "boost" given certain vertical pixel
alignments.
The regression that this fix fixed will be fixed another way.
Fixes#606.
This works on macOS, Wayland, and a few more esoteric platforms. X11
doesn't have a concept of DPI-awareness. Note that with this flag
SDL_GetWindowSize() isn't guaranteed to return the actual window size.
Retextured checkpoints have always been in the game, but clicking on
them in the editor would lead to them losing their retextured-ness. So,
checkpoints should be left alone if their p1 isn't either 0 or 1. Also,
they don't show up properly in the editor, so that's fixed, too.
Retextured and flipped terminals were added in 2.3, and show up properly
in-game, but don't properly show up in the editor, either. So now they
show up in the editor. Additionally, clicking on them will flip the
terminal as well, but only if its p1 is 0 or 1, just like checkpoints
now do.
This call to Makebfont() always existed, but ever since 2.3's per-level
custom assets were added, graphics.reloadresources() also calls
graphics.Makebfont(), meaning this call is unnecessary. Calling it twice
results in graphics.bfont and graphics.flipbfont having twice the number
of elements, until custom assets get mounted (or unmounted, technically).
This does the same thing as the last commit, but for No Death Mode
instead of Time Trials. Whenever you die in No Death Mode, or complete
it, all the relevant variables get copied to variables prefixed with
'ndmresult' that never get reset by script.hardreset(), and these
variables are what titlerender() use, instead of the "live" ones.
This makes it so when a Time Trial gets completed, all the relevant
variables get copied onto variables prefixed with 'timetrialresult',
which never get reset by script.hardreset(). Then titlerender() will use
those variables accordingly.
There are multiple different exit paths to the main menu. In 2.2, they
all had a bunch of copy-pasted code. In 2.3 currently, most of them use
game.quittomenu(), but there are some stragglers that still use
hand-copied code.
This is a bit of a problem, because all exit paths should consistently
have FILESYSTEM_unmountassets(), as part of the 2.3 feature of per-level
custom assets. Furthermore, most (but not all) of the paths call
script.hardreset() too, and some of the stragglers don't. So there could
be something persisting through to the title screen (like a really long
flash/shake timer) that could only persist if exiting to the title
screen through those paths.
But, actually, it seems like there's a good reason for some of those to
not call script.hardreset() - namely, dying or completing No Death Mode
and completing a Time Trial presents some information onscreen that
would get reset by script.hardreset(), so I'll fix that in a later
commit.
So what I've done for this commit is found every exit path that didn't
already use game.quittomenu(), and made them use game.quittomenu(). As
well, some of them had special handling that existed on top of them
already having a corresponding entry in game.quittomenu() (but the path
would take the special handling because it never did game.quittomenu()),
so I removed that special handling as well (e.g. exiting from a custom
level used returntomenu(Menu::levellist) when quittomenu() already had
that same returntomenu()).
The menu that exiting from the level editor returns to is now handled in
game.quittomenu() as well, where the map.custommode branch now also
checks for map.custommodeforreal. Unfortunately, it seems like entering
the level editor doesn't properly initialize map.custommode, so entering
the level editor now initializes map.custommode, too.
I've also taken the music.play(6) out of game.quittomenu(), because not
all exit paths immediately play Presenting VVVVVV, so all exit paths
that DO immediately play Presenting VVVVVV now have music.play(6)
special-cased for them, which is fine enough for me.
Here is the list of all exit paths to the menu:
- Exiting through the pause menu (without glitchrunner mode)
- Exiting through the pause menu (with glitchrunner mode)
- Completing a custom level
- Completing a Time Trial
- Dying in No Death Mode
- Completing No Death Mode
- Completing an Intermission replay
- Exiting from the level editor
- Completing the main game
Comments in general don't get verified by the compiler, but
commented-out code is even worse. Especially since this looks to be
outdated code.
As always, if we need some of this code, then we can just look back in
the Git history.
This fixes a segfault, because we would then pass compressed image data
to SDL_ConvertSurfaceFormat() in LoadImage(). I didn't test my previous
PR. Whoops.
Implicit conversion warnings happen all over the codebase, but there's
no reason to warn on all of them, and adding casts everywhere is
annoying to read and patently unnecessary.
MSVC is the only compiler that has this warning (GCC even on -Wall
-Wextra doesn't warn about implicit conversions), so disable it for
MSVC.
While compiling in release mode, GCC warns about these two potentially
being used uninitialized further down. The only way this could happen is
if the case-switches below didn't match up with a case, which would
require the game to be in an invalid state (and have invalid values for
rcol and spcol), but it's better to be safe than sorry.
The only thing we need LodePNG for is to decode a PNG that we've already
loaded into memory. We handle the filesystem part ourselves, so we don't
need LodePNG's filesystem functions; we don't encode images, and we
don't use the zlib functions. So disable all of those.
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 de-duplicates the tool drawing code a bit in the menu that
gets brought up when you press Space in the level editor, as well as
fixes several bugs related to the fact that the original author(s) of
the code decided to copy-paste everything. (It was most likely Terry,
judging by the distinct lack of whitespace between tokens in the code.)
There are two "pages" of tools that get shown when you open the tool
menu, according to your currently-selected tool.
1. On the first page, your currently-selected tool gets a brighter
outline. However, on the second page, the code to draw the outline over
your currently-selected tool is missing. So I've fixed that.
2. On the first page, the glyph indicator next to the tool icon also
gets brighter when you have that tool selected. However, on the
second page, the code that drew the brighter-colored indicator got
ran before the code that drew the normal-colored indicator, so this
was never shown. This is also fixed.
3. The glyph indicator of the gravity line tool didn't get brighter when
you had it selected, due to its special-cased copy-pasted code
drawing its brighter color before drawing its normal color. This has
also been fixed.
4. Lastly, the tool menu no longer draws the brighter-colored glyphs on
top of the normal-colored glyphs. Instead, the menu will simply draw
the brighter-colored glyphs and will not draw the normal-colored
glyphs in the first place. This is because double-drawing text like
this will look bad if the user has a custom font.png that has
translucent pixels, like I do.
All of these bugs have been fixed by paying off the technical debt of
copy-pasting code.
This variable seems to have been intended to make sure
game.savestatsandsettings() was called at the end of the frame, or make
sure that it didn't get called more than once per frame. I don't see any
frame ordering-related reason why it needs to be called specifically at
the end of the frame (the function doesn't modify any state), so it's
more plausible that it was added to make sure it didn't get called more
than one per frame.
However, upon further analysis, none of the code paths where
game.savemystats is used ever calls or sets game.savemystats more than
once, and a majority of the code directly calls
game.savestatsandsettings() anyway, so there's no reason for this
variable to exist. If we ever need to make sure it doesn't get called
more than once, and there's no way to change the code paths around to
prevent it otherwise, we can use the defer callbacks system that I added
to #535, when it gets merged.
These variables basically serve no purpose. map.customx and map.customy
are clearly never used. map.finalx and map.finaly, on the other hand,
are basically always game.roomx and game.roomy respectively if
map.finalmode is on, and if it's off, then they don't matter.
Also, there are some weird and redundant variable assignments going on
with these; most notably in map.gotoroom(), where rx/ry (local
variables) get assigned to finalx/finaly, then finalx/finaly get
assigned to game.roomx/game.roomy, then finalx/finaly get assigned to
rx/ry. If finalx/finaly made a difference, then there'd be no need to
assign finalx/finaly back to rx/ry. So it makes the code clearer to
remove these weird bits of code.
2.3's per-level assets feature also added a hotkey to reload the custom
assets of the level you're currently editing in the editor, so you
wouldn't have to re-load the level yourself. This hotkey is F9, but
however, it hasn't been documented in the hotkey list brought up by
pressing Shift, until now.
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).
Line clipping and second-frame edge-flipping have been broken since #539
was merged (d910c5118d). The cause of this
is moving the onground/onroof code around.
A proper loop order fix is going to come once #535 gets finalized and
merged, so this is a stopgap measure just to make sure people don't
report that line clipping or second-frame edge-flipping are broken in
current builds of 2.3.
There is a certain ordering of which corners you click on to place enemy
and platform boundaries, and script boxes. You must first click on the
top-left corner, then click on the bottom-right corner. The visual box
that is drawn after you've first clicked on the top-left corner clearly
shows this intended way of doing things.
However, it seems like despite the visuals, the game didn't properly
prevent you from clicking on the corners in the wrong way. If you placed
it from top-right to bottom-left, or bottom-left to top-right, then the
game would place the boxes accordingly, and they would have a weird
shape where two of its opposite sides would just be missing. But,
placing them from bottom-right to top-left is prevented accordingly.
The bug comes down to a simple use of "or", instead of the correct
"and". This isn't the first time the wrong conjunction was used in a
conditional... (8260bb2696, #136)
Since the code block that the if-statement guards is the code that will
execute if the corners placed were correct, the if-statement thus should
be written in the positive case and use a more restrictive "and",
instead of the negative case, which would use a more looser "or". There
are less cases that are correct than cases which are incorrect - in this
case, there is only 1 correct way of doing things (top-left to
bottom-right), compared to 3 incorrect ways of doing things (top-right
to bottom-left, bottom-left to top-right, bottom-right to top-left) - so
when thinking of positive cases, you should be using "and".
Or, you can always just test it. This bug has been in the game since
2.0, so it seems like no one just tested that incorrect input actually
didn't work.
Ever since 2.0, the colors of some of the Time Trial trophies in the
Secret Lab don't correspond to the crewmate of the given level. The
trophy for the Tower uses Victoria's color, and the Lab trophy uses
Vermilion's color. The Space Station 2 trophy uses Viridian's color, and
the Final Level trophy uses Vitellary's color.
This doesn't appear to be intentional, and it would be odd if it was,
since this game matches the colors everywhere else (each zone on the map
is colored with their respective crewmate in mind, for instance). Also,
the Lab trophy has the sad expression, which is Victoria's trait - it
would be weird if this was intended for Vermilion instead.
But the biggest piece of evidence that this was unintentional is the
corresponding comment for each color in Graphics::setcol(). It mislabels
yellow as cyan, cyan as yellow, blue as red, and red as blue.
To fix this, I simply have to set the correct color for each trophy in
case 25 of entityclass::createentity(). I could fix it in
Graphics::setcol() itself, but custom levels might depend on those
certain colors being the way they are, so it's a safer bet to just fix
it in the trophy creation case itself.
The diff of this might look weird. Even though all I'm doing is changing
some value assignments around, it looks like the "patience" algorithm
thinks I'm moving a whole case of the trophy switch-case around.
So... it looks like being able to switch through tilesets backwards has
been in 2.3 for a while, guess no one just uses 2.3 or the level editor
that much. It seems like it's always been broken, too.
If you were on the Space Station tileset (tileset 0), pressing Shift+F1
would keep you on the Space Station tileset instead of switching to the
Ship (tileset 4).
It looks like the problem here was mixing size_t and int together - so
the modulus operation promoted the left-hand side to size_t, which is
unsigned, so the -1 turned into SIZE_MAX, which is 18446744073709551615
on my system. You'll note that that ends in a 5, so the number is
divisible by 5, meaning taking it modulo 5 leads to 0. So the tileset
would be kept at 0.
At least unsigned integer underflow/overflow is properly defined, so
there's no UB here. Just careless type mixing going on.
The solution is to make the modulus an int instead of a size_t. This
introduces an implicit conversion, but I don't care because my compiler
doesn't warn about it, and implicit conversion warnings ought to be
disabled on MSVC anyway.
This fixes a bug where if you entered a tower before watching the
credits sequence, the credits sequence would have mismatched text and
background colors.
This bug happened because entering a tower modified the r/g/b attributes
of mapclass, and updated graphics.towerbg, without updating
graphics.titlebg too. Then gamecompleterender() uses the r/g/b
attributes of mapclass.
The solution is to put the r/g/b attributes on TowerBG instead. That
way, entering a tower will only modify the r/g/b attributes used to
render towers, and won't affect the r/g/b attributes used to render the
credits sequence.
Additionally, I also de-duplicated the case-switch that updated the
r/g/b attributes based off of the current colstate, because it got
copy-pasted twice, leading to three instances of one piece of code.
This fixes a bug where if you completed a custom level during
command-line playtesting, when returning to the title screen, the
background would be red and the text would be white.
This is because playtesting skips over the code path of pressing ACTION
to start the game and advance to the title screen, and the code path of
that ACTION press specifically initializes the title screen colors to
cyan.
This is also caused by the fact that completing a custom level doesn't
call map.nexttowercolour(), but my guess is that the intent there was
that the player would select a custom level, complete it, and return to
the title screen on the same screen with the same colors, so I decided
not to add a map.nexttowercolour() there.
Instead, I've moved the cyan color initialization to main(), so that it
is always executed no matter what, and doesn't require you to take a
specific code path to do it.
With commit 48313169b6 (PR #453),
AllyTally added a single-case patch for a regression, instead of fixing
it at its root cause.
In fact, that commit only fixes the music if Presenting VVVVVV is
playing while exiting to the menu, not if you enter a level that plays
Presenting VVVVVV - so it only fixes it going one way, and not going the
other way around; neither fixing also all the other cases this could
happen.
It doesn't, say, fix the case where you are exited to the menu
automatically after collecting the last crewmate in the level (or if the
level calls gamestate 1013 itself), which is what happens in my MIRA-VIU
TAS video at the end, and which I noted in the description of that video
( https://www.youtube.com/watch?v=OYQO4ePbYW4&t=111 ).
So, the problem here is that when musicclass::play() is called, it sees
that currentsong is the same as its input, and decides that since the
music is already playing, it shouldn't play the music again. Thus, the
music fades out, and we get silence instead of the music playing again.
But I said this was a regression. Why didn't this happen in 2.2? Well,
it's because of the fact that 2.2 sets currentsong to -1 (no music
playing at all) immediately when starting a fadeout, and not when the
fadeout completes (commit facb079b35,
PR #316). As you can imagine, this discrepancy could lead to bugs, given
that the game would think that music wasn't playing when in actuality it
was, but fixing this bug could also break code that expected this wrong
behavior. And in this case, it has.
So to properly fix the root cause of this, instead of naïvely
single-case patching out every case that comes up randomly, in
musicclass::play(), the function will now ignore if the input given is
the same as currentsong if the music is currently fading out.
This reverts commit 48313169b6, "Don't
fade music out when returning to the menu if it's Presenting VVVVVV".
This commit is being reverted because it is only a single-case patch -
that is, it fixes only a single symptom of the bug, and not its
underlying cause.
This is also another conditional where the rest of the function is
nested. Furthermore, in order to not repeat ourselves, I've also decided
to unconditionally assign currentsong to t, because if t is -1
currentsong gets assigned to -1 anyway - so it's the same thing, but
it's much easier to see and think about.
This removes an indentation level, and makes it easier to reason about
the function since you essentially now view it as the function returning
right there.
