This patch restores some 2.2 behavior, fixing a regression caused by the
refactor of properly using std::vectors.
In 2.2, the game allocated 200 items in obj.entities, but used a system
where each entity had an `active` attribute to signify if the entity
actually existed or not. When dealing with entities, you would have to
check this `active` flag, or else you'd be dealing with an entity that
didn't actually exist. (By the way, what I'm saying applies to blocks
and obj.blocks as well, except for some small differing details like the
game allocating 500 block slots versus obj.entities's 200.)
As a consequence, the game had to use a separate tracking variable,
obj.nentity, because obj.entities.size() would just report 200, instead
of the actual amount of entities. Needless to say, having to check for
`active` and use `obj.nentity` is a bit error-prone, and it's messier
than simply using the std::vector the way it was intended. Also, this
resulted in a hard limit of 200 entities, which custom level makers ran
into surprisingly quite often.
2.3 comes along, and removes the whole system. Now, std::vectors are
properly being used, and obj.entities.size() reports the actual number
of entities in the vector; you no longer have to check for `active` when
dealing with entities of any sort.
But there was one previous behavior of 2.2 that this system kind of
forgets about - namely, the ability to have holes in between entities.
You see, when an entity got disabled in 2.2 (which just meant turning
its `active` off), the indices of all other entities stayed the same;
the indice of the entity that got disabled stays there as a hole in the
array. But when an entity gets removed in 2.3 (previous to this patch),
the indices of every entity afterwards in the array get shifted down by
one. std::vector isn't really meant to be able to contain holes.
Do the indices of entities and blocks matter? Yes; they determine the
order in which entities and blocks get evaluated (the highest indice
gets evaluated first), and I had to fix some block evaluation order
stuff in previous PRs.
And in the case of entities, they matter hugely when using the
recently-discovered Arbitrary Entity Manipulation glitch (where crewmate
script commands are used on arbitrary entities by setting the `i`
attribute of `scriptclass` and passing invalid crewmate identifiers to
the commands). If you use Arbitrary Entity Manipulation after destroying
some entities, there is a chance that your script won't work between 2.2
and 2.3.
The indices also still determine the rendering order of entities
(highest indice gets drawn first, which means lowest indice gets drawn
in front of other entities). As an example: let's say we have the player
at 0, a gravity line at 1, and a checkpoint at 2; then we destroy the
gravity line and create a crewmate (let's do Violet).
If we're able to have holes, then after removing the gravity line, none
of the other indices shift. Then Violet will be created at indice 1, and
will be drawn in front of the checkpoint.
But if we can't have holes, then removing the gravity line results in
the indice of the checkpoint shifting down to indice 1. Then Violet is
created at indice 2, and gets drawn behind the checkpoint! This is a
clear illustration of changing the behavior that existed in 2.2.
However, I also don't want to go back to the `active` system of having
to check an attribute before operating on an entity. So... what do we
do to restore the holes?
Well, we don't need to have an `active` attribute, or modify any
existing code that operates on entities. Instead, we can just set the
attributes of the entities so that they naturally get ignored by
everything that comes into contact with it. For entities, we set their
invis to true, and their size, type, and rule to -1 (the game never uses
a size, type, or rule of -1 anywhere); for blocks, we set their type to
-1, and their width and height to 0.
obj.entities.size() will no longer necessarily equal the amount of
entities in the room; rather, it will be the amount of entity SLOTS that
have been allocated. But nothing that uses obj.entities.size() needs to
actually know the amount of entities; it's mostly used for iterating
over every entity in the vector.
Excess entity slots get cleaned up upon every call of
mapclass::gotoroom(), which will now deallocate entity slots starting
from the end until it hits a player, at which point it will switch to
disabling entity slots instead of removing them entirely.
The entclass::clear() and blockclass::clear() functions have been
restored because we need to call their initialization functions when
reusing a block/entity slot; it's possible to create an entity with an
invalid type number (it creates a glitchy Viridian), and without calling
the initialization function again, it would simply not create anything.
After this patch is applied, entity and block indices will be restored
to how they behaved in 2.2.
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.
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.
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 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.
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 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.
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).
When gamemode(teleporter) gets run in a script, it brings up a read-only
version of the teleporter screen, intended only for displaying rooms on
the minimap.
However, ever since 2.3 allowed bringing up the map screen during
cutscenes (in order to prevent softlocks), bringing up the map screen
during this mode would (1) do an unnecessary animation of suddenly
switching back to the game and bringing up the menu screen again (even
though the menu screen has already been brought up), and (2) would let
you close the menu entirely and go back to GAMEMODE, thus
unintentionally closing the teleporter screen and kind of ruining the
cutscene.
To fix this, when you bring up the map screen, it will instead instantly
transition to the map screen. And when you bring it down, it will also
instantly transition back to the teleporter screen.
But that's not all. The previous behavior was actually kind of a nice
failsafe, in that if you somehow got stuck in a state where a script ran
gamemode(teleporter), but stopped running before it could take you out
of that mode by running gamemode(game), then you could return to
GAMEMODE yourself by bringing up the map screen and then bringing it
back down. So I've made sure to keep that failsafe behavior, only as
long as there isn't a script running.
When bringing up the map screen, the game does a small menu animation
where the menu comes in from the bottom. The code to calculate the menu
offset is copy-pasted everywhere, so I thought I'd de-duplicate it to
make my life easier when working with it. I also included the
game.gamestate assignment in the de-duplicated function, so it would be
easier for a future bugfix.
At the same time, I'm also removing all the BlitSurfaceStandard()s that
copied menubuffer to backBuffer. The red flag is that this blit happened
for every single entry point to MAPMODE and TELEPORTERMODE, except for
the script command gamemode(teleporter). Pressing Enter to bring up the
map screen, pressing Enter to quit the Super Gravitron, pressing Esc to
bring up the pause screen, and pressing Enter to bring up the teleporter
screen all do this blit, so if this blit was there to fix a bug, then
there's a bug with using the script command gamemode(teleporter)... but,
as far as I can tell, there isn't.
That's because the blit basically does nothing. All the blit does is
copy menubuffer onto backBuffer. Then the next thing that happens is
that either maprender() or teleporterrender() will be called, and the
first thing that those functions will always do is fill backBuffer with
solid black, completely overriding the previous blit. So that's why
removing this blit won't have any effect, and it can be safely removed
for code clarity.
To fix this bug, all we have to do is just pass the existing
ScreenSettings* that we have in loadstats() to savestats(), and in
loadsettings() to savesettings().
Fixes#556. Depends on #558.
Another step to fix the bug #556 is to allow Game::savestats() to accept
a pointer to an existing ScreenSettings struct. This entails refactoring
Game::savesettings() and Game::serializesettings() to accept the
function as well, along with adding Screen::GetSettings() so the
settings of the current Screen can be easily grabbed.
In order to be able to fix the bug #556, I'm planning on adding
ScreenSettings* to the settings.vvv write function. However, that
entails adding another argument to Game::savesettings(), which is going
to be really messy given the default argument of Game::savestats().
That, combined with the fact that the code comment at the site of the
implementation of Game::savestats() being wrong (!!!), leads me to
believe that using default function arguments here isn't worth it.
Instead, what I've done is made it so callers are explicit about whether
or not they're calling savestats(), savesettings(), or both at the same
time. If they are calling both at the same time, then they will be using
a new function named savestatsandsettings().
In short, these are the interface changes:
* bool Game::savestats(bool) has been removed
* bool Game::savestatsandsettings() has been added
* void Game::savestats_menu() has been renamed to
void Game::savestatsandsettings_menu()
* All previous callers of bool Game::savestats() are now using bool
Game::savestatsandsettings()
* The one caller of bool Game::savestats(bool) is now using bool
Game::savestats()
`success = success && savesettings();` is now changed to
`success &= savesettings();`. It's bitwise, and I think C++ should have
had a &&= for completeness, but it shouldn't matter here.
Changing settings would most of the time attempt to save unlock.vvv and
now also settings.vvv, but there would be no feedback whether the files
have been saved successfully or not. Now, if saving fails when changing
settings in the menu, a warning message will be shown. The setting will
still be applied of course, but the user will be informed it couldn't
be saved. This message can be silenced until the game is restarted,
because I can imagine this could get very annoying when someone already
knows their settings aren't writeable.
Also, some options didn't even save settings in the first place. These
are turning off invincibility, and by coincidence precisely all the
options in the advanced options menu. I made sure these options now do
so.
It wasn't a direct duplicate of key.sensitivity, but it was still
basically the same thing. Although to be fair, at least the case-switch
conversion didn't get duplicated everywhere unlike game.slowdown.
So now key.sensitivity functions the same as game.controllerSensitivity,
and it only gets converted to its actual value whenever a joystick input
happens in key.Poll(), unlike previously where it got converted every
single frame on the title screen (there was even a comment that said
"TODO bit wasteful doing this every poll").
game.gameframerate seems to exist for converting the value of
game.slowdown into an actual timestep value, when really the timestep
value should just use game.slowdown directly with a fast lookup table.
Otherwise, there's a bunch of duplicated game.slowdown case-switches
everywhere, which adds up to a large, annoying pile should the values be
changed in the future. But now the duplicate variable has been removed,
and with it, all the copy-pasted case-switches.
Also, the game speed text rendering in Menu::accessibility and
Menu::setslowdown has been factored out to a function and de-duplicated
as well.
There were some duplicate Screen configuration variables that were on
Game, when there didn't need to be.
- game.fullScreenEffect_badSignal is a duplicate of
graphics.screenbuffer->badSignalEffect
- game.fullscreen is a duplicate of !graphics.screenbuffer->isWindowed
- game.stretchMode is a duplicate of graphics.screenbuffer->stretchMode
- game.useLinearFilter is a duplicate of
graphics.screenbuffer->isFiltered
These duplicate variables have been removed now.
I put indentation when handling the ScreenSettings struct in main() so
the local doesn't live for the entirety of main() (which is the entirety
of the program).
Assigning these variables is now wholly unnecessary ever since #522 got
merged, and in fact setting graphics.backgrounddrawn to false actually
causes the warp background to "skip" when the map screen gets closed. So
this fixes that bug, too.
This kludge variable was used to re-set the warp background after coming
back from the in-game settings menus. But since #522 got merged, this
has no longer been necessary.
The game previously did this dumb thing where it lumped in all its
settings with its file that tracked your records and unlocks,
`unlock.vvv`. It wasn't really an issue, until 2.3 came along and added
a few settings, suddenly making a problem where 2.3 settings would be
reset by chance if you decided to touch 2.2.
The solution to this is to move all settings to a new file,
`settings.vvv`. However, for compatibility with 2.2, settings will still
be written to `unlock.vvv`.
The game will prioritize reading from `settings.vvv` instead of
`unlock.vvv`, so if there's a setting that's missing from `unlock.vvv`,
no worries there. But if `settings.vvv` is missing, then it'll read
settings from `unlock.vvv`. As well, if `unlock.vvv` is missing, then
`settings.vvv` will be read from instead (I explicitly tested for this,
and found that I had to write special code to handle this case,
otherwise the game would overwrite the existing `settings.vvv` before
reading from it; kids, make sure to always test your code!).
Closes#373 fully.
Whenever you delete all your save data, your settings aren't changed at
all, and you could resave them if you fiddled with a setting somewhere.
But the full game doesn't count Flip Mode as a setting, instead it
counts it as an unlock. This means deleting your save data would unset
Flip Mode in M&P, which would seem weird because in M&P it's just a
simple setting.
For consistency, Flip Mode shouldn't be unset when deleting save data in
M&P.
When I encountered this function, I asked myself, why make a dedicated
function instead of casting to int?
Well, as it turns out, you can't just cast to int, because you have to
convert it to a string by doing help.String(number).c_str(), like all
the other ints. So it's actually more wasteful to do it the normal way
instead of using BoolToString().
That is, until my recent changes came along and made it so that you can
just provide an int and it'll automatically be converted to a string, no
questions asked. Now, it's more optimal to do a straight cast to int
instead of having to go through a middleman function. So this function
is getting axed in favor of casting to int instead.
This file is probably the biggest one, as there will be more settings
added in the future and we don't want people's settings to be erased. Of
course, this file will be migrated to a settings.vvv sometime later in
2.3 in order to prevent 2.2 from erasing 2.3 settings.
Now that tower, title, and horizontal/veritcal warp backgrounds all use
separate buffers, there's no longer any need to temporarily store
variables as a workaround for the buffers stepping on each other.
With the previous commit in place, we can now simply move some usages of
the previous towerbg to use a separate object instead. That way, we
don't have to mess with a monolithic state, or some better way to phrase
what I just said, and we instead have two separate objects that can
coexist side-by-side.
Previously, the tower background was controlled by a disparate set of
attributes on Graphics and mapclass, and wasn't really encapsulated. (If
that's what that word means, I don't particularly care about
object-oriented lingo.) But now, all relevant things that a tower
background has has been put into a TowerBG struct, so it will be easy to
make multiple copies without having to duplicate the code that handles
it.
I was investigating a desync in my Nova TAS, and it turns out that
the gravity line collision functions check for the `oldxp` and `oldyp`
of the player, i.e. their position on the previous frame, along with
their position on the current frame. So, if the player either collided
with the gravity line last frame or this frame, then the player collided
with the gravity line this frame.
Except, that's not actually true. It turns out that `oldxp` and `oldyp`
don't necessarily always correspond to the `xp` and `yp` of the player
on the previous frame. It turns out that your `oldyp` will be updated if
you stand on a vertically moving platform, before the gravity line
collision function gets ran. So, if you were colliding with a gravity
line on the previous frame, but you got moved out of there by a
vertically moving platform, then you just don't collide with the gravity
line at all.
However, this behavior changed in 2.3 after my over-30-FPS patch got
merged (#220). That patch took advantage of the existing `oldxp` and
`oldyp` entity attributes, and uses them to interpolate their positions
during rendering to make everything look real smooth.
Previously, `oldxp` and `oldyp` would both be updated in
`entityclass::updateentitylogic()`. However, I moved it in that patch to
update right before `gameinput()` in `main.cpp`.
As a result, `oldyp` no longer gets updated whenever the player stands
on a vertically moving platform. This ends up desyncing my TAS.
As expected, updating `oldyp` in `entityclass::movingplatformfix()` (the
function responsible for moving the player whenever they stand on a
vertically moving platform) makes it so that my TAS syncs, but the
visuals are glitchy when standing on a vertically moving platform. And
as much as I'd like to get rid of gravity lines checking for whether
you've collided with them on the previous frame, doing that desyncs my
TAS, too.
In the end, it seems like I should just leave `oldxp` and `oldyp` alone,
and switch to using dedicated variables that are never used in the
physics of the game. So I'm introducing `lerpoldxp` and `lerpoldyp`, and
replacing all instances of using `oldxp` and `oldyp` that my over-30-FPS
patch added, with `lerpoldxp` and `lerpoldyp` instead.
After doing this, and applying #503 as well, my Nova TAS syncs after
some minor but acceptable fixes with Viridian's walkingframe.
By "unnecessary qualifiers to self", I mean something like using the
'game.' qualifier for a variable on the Game class when you're inside a
function on the Game class itself. This patch is to enforce consistency
as most of the code doesn't have these unnecessary qualifiers.
To prevent further unnecessary qualifiers to self, I made it so the
extern in each header file can be omitted by using a define. That way,
if someone writes something referring to 'game.' on a Game function,
there will be a compile error.
However, if you really need to have a reference to the global name, and
you're within the same .cpp file as the implementation of that object,
you can just do the extern at the function-level. A good example of this
is editorinput()/editorrender()/editorlogic() in editor.cpp. In my
opinion, they should probably be split off into their own separate file
because editor.cpp is getting way too big, but this will do for now.
You're intended to rescue Violet first, and not second, third, or
fourth, and especially not last.
If you rescue her second, third, or fourth, your crewmate progress will
be reset, but you won't be able to re-rescue them again. This is because
Vitellary, Verdigris, Victoria, and Vermilion will be temporarily marked
as rescued during the `bigopenworld` cutscene, so duplicate versions of
them don't spawn during the cutscene, and then will be marked as missing
later to undo it.
This first issue can be trivially fixed by simply toggling flags to
prevent duplicates of them from spawning during the cutscene instead of
fiddling with their rescue statuses.
However, there is still another issue. If you rescue Violet last, then
you won't be warped to the Final Level, meaning you can't properly
complete the game. This can be fixed by adding a `crewrescued() == 6`
check to the Space Station 1 Level Complete cutscene. There is
additionally a temporary unrescuing of Violet so she doesn't get
duplicated during the `bigopenworld` cutscene, and I've had to move that
to the start of the `bigopenworld` and `bigopenworldskip` scripts,
otherwise the `crewrescued() == 6` check won't work properly.
I haven't added hooks for Intermission 1 or 2 because you're not really
meant to play the intermissions with Violet (but you probably could
anyway, there'd just be no dialogue).
Oh, and the pre-Final Level cutscene expects the player to already be
hidden before it starts playing, but if you rescue Violet last the
player is still visible, so I've fixed that. But there still ends up
being two Violets, so I'll probably replace it with a special cutscene
later that's not so nonsensical.
It's better to do INBOUNDS_VEC(i, obj.entities) instead of 'i > -1'.
'i > -1' is used in cases like obj.getplayer(), which COULD return a
sentinel value of -1 and so correct code will have to check that value.
However, I am now of the opinion that INBOUNDS_VEC() should be used and
isn't unnecessary.
Consider the case of the face() script command: it's not enough to check
i > -1, you should read the routine carefully. Because if you look
closely, you'll see that it's not guaranteed that 'i' will be initialized
at all in that command. Indeed, if you call face() with invalid
arguments, it won't be. And so, 'i' could be something like 215, and
that would index out-of-bounds, and that wouldn't be good. Therefore,
it's better to have the full bounds check instead of checking only one
bounds. Many commands are like this, after some searching I can also
name position(), changemood(), changetile(), changegravity(), etc.
It also makes the code more explicit. Now you don't have to wonder what
-1 means or why it's being checked, you can just read the 'INBOUNDS' and
go "oh, that checks if it's actually inbounds or not".
For some reason it called obj.getplayer() and did nothing with the
result. Weird. But it does say "Test script for space station" above.
Removing this fixes an 'unused variable' warning.
With the scope of these variables reduced, it makes analyzing this
function easier, as you can now clearly see all temporary variables are
actually initialized before they're used.
The game has four different functions for the same XML schema:
Game::loadtele(), Game::savetele(), Game::loadquick(), and
Game::savequick(). This essentially means one XML schema has been
copy-pasted three different times.
I can at least trim that number down to being copy-pasted only one time
by de-duplicating the reading and writing part. So both Game::loadtele()
and Game::loadquick() now use Game::readmaingamesave(), and
Game::savetele() and Game::savequick() now use
Game::writemaingamesave().
This will make it take less work to add XML forwards compatibility
(#373).
This is a regression from 25779606b4
(PR #74).
On the one hand, I should've thought this through carefully when
implementing the fix for the lower-score overwrite bug. On the other
hand, flibit didn't seem to notice either. And on the third hand, no one
else seems to have noticed, when they have had over 8 months to do so.
Not even the person who originally reported the lower-score overwrite
bug and also reported other bugs I hadn't noticed (e.g. the "You have
rescued a crewmate!" in Flip Mode) noticed this bug, which I believe was
weee50. But to be fair, he does seem to be less active nowadays.
On the fourth hand, I only realized the cause of the duplicate bug after
stepping through it in GDB, instead of just looking at it and going "hey
wait a minute" earlier. I'm surprised it didn't take me longer to
realize the problem.
I'm not sure what all these hands mean anymore, or where I'm getting
extra hands from. Whatever. This regression is fixed now.
When I added the over-30-FPS mode, I kept running into this problem
where the special images of text boxes would render during the
deltaframes of fade-in/fade-out animations, even though they shouldn't
be. So I simply added a flag to the text box that enables drawing these
special images.
However, this doesn't solve the problem fully, and there's still a small
chance that a special-image text box could draw another special image
during its deltaframes. It's really rare and you have to have your
deltaframe luck juuuuuust right (or you could use libTAS, probably), but
it helps to be in 40% slowmode and have a high refresh rate (which, if
it isn't a multiple of 30, you should disable VSync, too, in order to
not have a low framerate).
So instead, special images will only be drawn if the text box has fully
faded in completely. That solves the issue completely.
This commit fixes a bug that's existed since MMMMMM was added (so, 2.2),
where if you quicksaved in a custom level while no music was playing,
then quit and loaded that quicksave, and you were using PPPPPP while
having MMMMMM available, it would play MMMMMM track 15, even though the
game intends for the music to simply be silent.
This is due to the same bug that lets you play MMMMMM tracks if you're
on PPPPPP - musicclass::play() does a modulo, but C++ modulo is not
guaranteed to be positive given negative inputs, so the 16-track offset
is added to a negative number, resulting in targeting the MMMMMM
soundtrack instead of PPPPPP.
That exploit doesn't harm anyone and shouldn't be fixed, EXCEPT it
causes a problem in this specific case. But this bug can be fixed
without removing that exploit.
Note that I made the check do not-equal to -1 instead of greater-than
-1, so levels that intend on using track -2, -3, -4, etc. upon loading a
quicksave will still work as their creator intended. It's just that
specifically -1 is patched out, just to fix this issue.
What this simply does is make it so that in the event that
game.hascontrol is somehow set to false when there isn't a cutscene
running (i.e. when game.state is 0 and script.running is false), it gets
set back to true again.
There's many ways to interrupt a gamestate and/or a running script, most
notably telejumping and doing a screen transition in the middle of the
animation, interrupting it.
This implements the first part of my idea in this comment:
https://github.com/TerryCavanagh/VVVVVV/issues/391#issuecomment-659757071
Achievements could be unlocked in custom levels/make and play, so this adds the wrapper function `game.unlockAchievement` which calls `NETWORK_unlockAchievement` if `map.custommode` is false.
Also, this function and `Game::unlocknum` have both been `ifdef`ed to be empty if MAKEANDPLAY is defined.
This fixes the bug where Viridian would appear to "zip" when they would
be teleported to the position of the teleporter before being flung out
of it.
As discussed in #393, I've also set the oldxp/oldyp when Viridian is
temporarily positioned in the center of the room, even though at this
point they should already be invisible. This is just to be safe.
Fixes#393.
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.
Now if your cursor was at the bottom of the screen when you entered
playtesting but then was moved to the top during playtesting, when you
exit, the roomname won't instantly disappear and then sheepishly rise
back up again.
And if your cursor was at the bottom of the screen when you entered
playtesting, and exited still being that way, the roomname will rise
back down again and won't instantly disappear.
Both of these behaviors make the roomname movement much more continuous
than it was previously, and it feels smoother and less janky.
Also, use inspecial() instead of writing out each part of the
conditional separately. This just basically adds the insecretlab
conditional to the if-statement, which shouldn't be a big deal.
graphics.textbox.clear() should be used instead of
graphics.textboxremove() or graphics.textboxremovefast(), because even
with graphics.textboxremovefast(), you'll still have to process at least
one frame of GAMEMODE logic before the text boxes are actually properly
removed, and this caused a 1-frame glitch when exiting playtesting with
text boxes on-screen and then re-entering playtesting.
Technically I could've only fixed it in Game::returntoeditor(), but I
wanted to be safe, so I also fixed it in scriptclass::hardreset(), too.
This fixes a bug where the settings menu would immediately be brought up
if you used Esc to exit playtesting, unless you were an incredible ninja
and only pressed it for exactly one frame.
This fixes an annoying bug where if you use Up or Down to press ACTION
on the "All crewmates rescued!" dialogue whenever you rescue the last
crewmate during playtesting, it'll move you to the room above or below
you. This is because ed.keydelay isn't set to 6 (which is the standard
value that it gets set to whenever you press most keys in the editor),
but now it is.
The shouldreturntoeditor variable is supposed to be used because it
fixes the warp background not being reset if you exit into a
horizontally/vertically warping room with a different background. It
also properly resets other variables, which is good, too.
There's a bug where playtesting from Ved doesn't properly play the music
of the level, due to no fault with Ved.
This was because the music was being faded out by
scriptclass::startgamemode() case 23 after main() called music.play().
To fix this, just call music.play() when all the other variables are
being set in Game::customloadquick().
So you get a trophy and achievement for completing the game in Flip
Mode. Which begs the question, how does the game know that you've played
through the game in Flip Mode the entire way, and haven't switched it
off at any point? It looks like if you play normally all the way up
until the checkpoint in V, and then turn on Flip Mode, the game won't
give you the trophy. What gives?
Well, actually, what happens is that every time you press Enter on a
teleporter, the game will set flag 73 to true if you're NOT in Flip
Mode. Then when Game Complete runs, the game will check if flag 73 is
off, and then give you the achievement and trophy accordingly.
However, what this means is that you could just save your game before
pressing Enter on a teleporter, then quit and go into options, turn on
Flip Mode, use the teleporter, then save your game (it's automatically
saved since you just used a teleporter), quit and go into options, and
turn it off. Then you'd get the Flip Mode trophy even though you haven't
actually played the entire game in Flip Mode.
Furthermore, in 2.3 you can bring up the pause menu to toggle Flip Mode,
so you don't even have to quit to circumvent this detection.
To fix both of these exploits, I moved the turning on of flag 73 to
starting a new game, loading a quicksave, and loading a telesave (cases
0, 1, and 2 respectively in scriptclass::startgamemode()). I also added
a Flip Mode check to the routine that runs whenever you exit an options
menu back to the pause menu, so you can't circumvent the detection that
way, either.
Gamestates 300..336 are used to start scripts in custom levels. However,
it looks like instead of having the cases have common code, each
individual case was copy-pasted numerous times, which is pretty
wasteful.
std::string is one of those special types that has a constructor that
just initializes itself to a blank state automatically. This means all
`std::string`s are by default already `""`, so there's no need to set
them. And in fact, cppcheck throws out warnings about performance due to
initializing `std::string`s this way.
Instead, the string in MenuOption is just a buffer of 161 chars, which
is 40 chars (160 bytes if each were the largest possible UTF-8 character
size) plus a null terminator. This is because the maximum length of a
menu option that can be fit on the screen without going past is 40
chars.
There's no need to use a template here. Just manually call SDL_tolower()
or SDL_toupper() as needed.
Oh yeah, and use SDL_tolower() and SDL_toupper() instead of libc
tolower() and toupper().
So, originally, I wanted to keep them on Game, but it turns out that if
I initialize it in Game.cpp, the compiler will complain that other files
won't know what's actually inside the array. To do that, I'd have to
initialize it in Game.h. But I don't want to initialize it in Game.h
because that'd mean recompiling a lot of unnecessary files whenever
someone gets added to the credits.
So, I moved all the patrons, superpatrons, and GitHub contributors to a
new file, Credits.h, which only contains the list (and the credits max
position calculation). That way, whenever someone gets added, only the
minimal amount of files need to be recompiled.
They're always fixed-size anyways, there's no need for them to be
vectors.
Also used the new INBOUNDS_ARR() macro for the map.explored bounds
checks in Script.cpp, and made map.explored a proper bool array instead
of an int array.
Since they're always fixed-size, they don't need to be dynamically-sized
vectors.
entityclass::customcrewmoods is now a proper bool instead of an int now,
and I replaced the hardcoded constant 6 with a static const int Game
attribute to make it easier to change.
These are the besttimes, besttrinkets, bestlives, and bestrank
attributes of Game. bestframes was already a plain array.
As these are always fixed-sized, there's no reason for them to be
vectors. Also, I put their size in a static const int so it's easy to
change how many of them there are.
They're always fixed-size, so there's no need to them to be a dynamic
vector.
I changed their type to `bool` too because they don't need to be `int`s.
Also, I replaced the hardcoded 25 constant with at least a name, in case
people want to change it in the future.
In summary, if you got to gamestate 1002 or 1012 without an advancetext,
and you had completestop on, you were basically softlocked. So just add
those gamestates there and advance the gamestate if advancetext is off.
This was caused by the fact that not all unlocks were done through the
Game::unlocknum() function. Some just set the unlock number directly.
But it's fixed now.
The problem we're running into is entirely contained in the Screen - we need to
either decouple graphics context init from Screen::init or we need to take out
the screenbuffer interaction from loadstats (which I'm more in favor of since we
can just pull the config values and pass them to Screen::init later).
This is basically just bolting on the "frames" part of a time trial
score. There's not enough space to properly show it on the time trial
select screen, maybe we can figure something out later. But I at least
want to implement the functionality now.
This change was half-backported from the localization branch, except I
just came up with "scaling mode" as a better term than the more generic
"graphics mode". It doesn't make sense to still have the option be
called "toggle letterbox" because a third option (integer mode) was
added at some point.
The options for fullscreen and scaling mode were at the top, then there
were various other graphical options, and then the option to resize to
the nearest window size that is of an integer multiple was all the way
below that. Now that last option is moved to be right below the other
options related to window sizing.
VVVVVV's menus are kind of packed to the brim, so I thought it was time
to recategorize the menus a little bit. There's now a new "advanced
options" menu which holds the following options which were moved out of
graphic options, game options and especially accessibility options:
- toggle mouse
- unfocus pause
- fake load screen
- room name background
- glitchrunner mode
I also made the positioning of the titles and descriptions more
consistent, and made some options which were moved to the new menu not
so abbreviated ("load screen" and "room name bg")
Flip Mode will now be in the game options menu if either:
(1) You're playing the M&P version.
(2) You have it unlocked and you came here from the in-game pause
screen.
This is because if you're playing M&P, you'd have to close the game,
edit unlock.vvv, and re-launch the game to toggle Flip Mode, since
there's no other way to do so. And if you're playing the full version,
you'd have to save and exit your session in order to toggle Flip Mode.
If you want your game window to simply be exactly 320x240, or 640x480,
or 960x720 etc. then it's really annoying that there's no easy way to do
this (to clarify, this is different from integer mode, which controls
the size of the game INSIDE the window). The easiest way would be having
to close the game, go into unlock.vvv, and edit the window size
manually. VCE has a 1x/2x/3x/4x graphics option to solve this, although
it does not account for actual monitor size (those 1x/2x/3x/4x modes are
all you get, whether or not you have a monitor too small for some of
them or too big for any of them to be what you want).
I discussed this with flibit, and he said that VCE's approach (if it
accounted for monitor size) wouldn't work on high-retina displays or
high DPIs, because getting the actual multiplier to account for those
monitors is kind of a pain. So the next best thing would be to add an
option that resizes to the nearest perfect multiple of 320x240. That way
you could simply resize the window and let the game correct any
imperfect dimensions automatically.
It's sometimes unwanted by people, and it's unwanted enough that there
exist instructions to hexedit the binary to remove it (
https://distractionware.com/forum/index.php?topic=3247.0 ).
Fun fact, the unfocus pause didn't exist in 2.0.
There were a few problems with the old way of doing things:
(1) Level stats were an ad-hoc object. Basically, it's an object whose
attributes are stored in separate arrays, instead of being an actual
object with its attributes stored in one array.
(2) Level filenames with pipes in them could cause trouble. This is
because the filename attribute array was stored in the XML by being
separated by pipes.
(3) There was an arbitrary limit of only having 200 level stats, for
whatever reason.
To remedy this issue, I've made a new struct named CustomLevelStat that
is a proper object. The separate attribute arrays have been replaced
with a proper vector, which also doesn't have a size limit.
For compatibility with versions 2.2 and below, I've kept being able to
read the old format. This only happens if the new format doesn't exist.
However, I also WRITE the old format as well, in case you want to go
back to version 2.2 or below for whatever reason. It's slightly
wasteful to have both, but that way there's no risk of breaking
compatibility.
Centiseconds won't be saved to any save file or anything. This is just
to make speedrunning a bit more competitive, being able to know the
precise time of a time trial or full game run.
The time trial par time on the result screen always has ".99" after it.
This is basically due to the game comparing the number of seconds to the
par number of seconds using less-than-or-equal-to instead of simply
less-than.
This patch is very kludge-y, but at least it fixes a semi-noticeable
visual issue in custom levels that use internal scripts to spawn
entities when loading a room.
Basically, the problem here is that when the game checks for script
boxes and sets newscript, newscript has already been processed for that
frame, and when the game does load a script, script.run() has already
been processed for that frame.
That issue can be fixed, but it turns out that due to my over-30-FPS
game loop changes, there's now ANOTHER visible frame of delay between
room load and entity creation, because the render function gets called
in between the script being loaded at the end of gamelogic() and the
script actually getting run.
So... I have to temporary move script.run() to the end of gamelogic()
(in map.twoframedelayfix()), and make sure it doesn't get run next
frame, because double-evaluations are bad. To do that, I have to
introduce the kludge variable script.dontrunnextframe, which does
exactly as it says.
And with all that work, the two-frame (now three-frame) delay is fixed.
Ironically enough, resetting more variables in script.hardreset() makes
the glitchy fadeout system even more glitchy. Resetting map.towermode,
for example, makes it so that if you're in towers when you quit to the
menu, script.hardreset() makes it so that the game thinks you're no
longer inbounds (because it no longer thinks you're in a tower and thus
considers coordinates in the space of 40x30 tiles to be inbounds instead
of 40x700 or 40x100 tiles to be inbounds), calls map.gotoroom(), which
resets the gamestate to 0. So if we're using the old system, it's better
to reset only as much as needed.
And furthermore, we shouldn't be relying on script.hardreset() to
initialize variables for us. That should be done at the class
constructor level. So I've gone ahead and initialized the variables in
class constructors, too.
Glitchrunner mode is intended to re-enable glitches that existed in
older versions of VVVVVV. These glitches were removed because they could
legitimately affect a casual player's experience. Glitches like various
R-pressing screwery, Space Station 1 skip, telejumping, Gravitron
out-of-bounds, etc. will not be patched.
It should've checked the final spacing and not the intermediate maximum
value. I had changed some things around, and now the minimum spacing
was 5 instead of 0 by mistake.
All menus had a hardcoded X position (offset to an arbitrary starting
point of 110) and a hardcoded horizontal spacing for the "staircasing"
(mostly 30 pixels, but for some specific menus hardcoded to 15, 20 or
something else). Not all menus were centered, and seem to have been
manually made narrower (with lower horizontal spacing) whenever text
ran offscreen during development.
This system may already be hard to work with in an English-only menu
system, since you may need to adjust horizontal spacing or positioning
when adding an option. The main reason I made this change is that it's
even less optimal when menu options have to be translated, since
maximum string lengths are hard to determine, and it's easy to have
menu options running offscreen, especially when not all menus are
checked for all languages and when options could be added in the middle
of a menu after translations of that menu are already checked.
Now, menus are automatically centered based on their options, and they
are automatically made narrower if they won't fit with the default
horizontal spacing of 30 pixels (with some padding). The game.menuxoff
variable for the menu X position is now also offset to 0 instead of 110
The _default_ horizontal spacing can be changed on a per-menu basis,
and most menus (not all) which already had a narrower spacing set,
retain that as a maximum spacing, simply because they looked odd with
30 pixels of spacing (especially the main menu). They will be made even
narrower automatically if needed. In the most extreme case, the spacing
can go down to 0 and options will be displayed right below each other.
This isn't in the usual style of the game, but at least we did the best
we could to prevent options running offscreen.
The only exception to automatic menu centering and narrowing is the
list of player levels, because it's a special case and existing
behavior would be better than automatic centering there.
To fix this annoying flicker (which, btw, took me WAY too long to do), I
had to introduce yet another kludge variable to signal that the
horizontal/vertical warp background should be re-initialized on the
pause screen.
I think I could technically keep the 'graphics.backgrounddrawn = false;'
in maplogic() and remove the 'graphics.backgrounddrawn = false;' in
Game::returntopausemenu(), but I'm keeping that other one around because
it doesn't hurt and just as a general precaution and safety measure.
This makes it so the tower background doesn't persist and scroll upwards
if you exit the menu in a warp zone horizontal or vertical room.
Ugh, and while we're on the subject of separating the in-game tower
background and the menu tower background, could we PLEASE separate the
horizontal / vertical warp backgrounds from the tower backgrounds, too?!
Another thing that's annoyed me a lot is being unable to simply press
Esc to close the pause menu. You'd have to hover over the "return to
game" or "keep playing" option. This would be even more annoying with
more options on the menu, so allowing to press Esc is a nice
quality-of-life thing.
Also set map.tdrawback to true when leaving the menu.
This is to fix the interpolated color of the tower background
persisting, as well as making sure the menu background doesn't persist
when exiting.
This is to pre-emptively prevent piling up stack frames for what I'll be
adding next, which is pressing Esc in the options menu in-game
automatically moving you back to MAPMODE.
There is now an option in "graphic options" named "toggle fps", which
toggles whether the game visually runs at 1000/34 FPS or over 1000/34
FPS. It is off by default.
I've had to put the entire game loop in yet another set of braces, I'll
indent it next commit.
These special images are the crewmates, Level Complete, and Game
Complete images. They flashed depending on if you were lucky and
happened to got your delta-timesteps just right when text boxes were
fading in and out.
Honestly, I'm surprised text box fading in/out hasn't ran into this
issue before. It's insane luck that this issue hasn't occurred before or
anything.
Well, anyways, to fix this, there's now an attribute `allowspecial` on
text boxes, and an optional parameter of the same name for
Graphics::createtextbox(). This attribute is the only thing that will
let these special text box images render. And any createtextbox()es that
utilize these special images have been updated accordingly.
To make it real smooth, just in case it was noticeable that it only
updated at 1000/34 FPS before (well, except in slowmode, it's really
noticeable THERE).
Also this removes the re-typing out of (game.act_fade/10.0f) for every
single R, G, and B in gamerender().
This prevents being able to "roll over" the amount of minutes to 0 (by
simply waiting for the timer to tick past one hour) and being able to
get a result of 00:13 when your result is really 01:00:13.
By looking only at the minutes, the game would read 01:00:13 as 00:13
instead. So simply add the amount of hours to the time trial result.
If you exited to the menu normally (i.e. got on a code path that went
through Game::quittomenu()), the menu music wouldn't play. This is
because FILESYSTEM_unmountassets() was put after music.play(6). So the
game would play the custom level's track 6, and then unmount it, which
meant it could no longer play track 6, but there's nothing telling the
game to play track 6 again. So I just changed the frame ordering around.
I also added a comment to make sure anyone reading the code is aware of
the frame order dependency.
The only class that actually needs its i/j/k kept is scriptclass,
because some custom levels rely on it for creating custom activity
zones. So I haven't touched that.
Other than that, there's no chance that anything important relies on
i/j/k in any other class. For that to be the case, it would have to use
i/j/k without initializing it beforehand, and that can simply be
detected by removing the attribute from the header file and seeing where
the compiler complains. And the compiler complains only about cases
where it's initialized first. (Note that due to this check, I *haven't*
removed Graphics's `m` as it precisely does exactly this, using it
without initializing it first.)
Interestingly enough, otherlevelclass and towerclass have unused i/k
variables for whatever reason.
This prevents the game from being saved if you manage to trigger a
savetele() during a "special" gamemode (like if you use the Gravitron
out-of-bounds glitch when replaying Intermission 2, then go to Game
Complete that way).
And this the function with the least amount of cases where its sentinel
value is used unchecked. Thankfully. obj.getplayer() was a bit of a slug
to get through.
obj.getplayer() can return -1, which can cause out-of-bounds indexing of
obj.entities, which is really bad. This was by far the most changes, as
obj.getplayer() is the most used entity-getting function that returns
-1, as well as the most-used function whose sentinel value goes
unchecked.
To deal with the usage of obj.getplayer() in mapclass::warpto(), I just
added general bounds checks inside that function instead of changing all
the callers.
A few months ago, I added ghosts to the VVVVVV: Community Edition editor. I was told recently I should think
about upstreaming it, and with Terry saying go ahead I finally ported them into VVVVVV. There's one slight
difference however--you can choose whether you have them or not in the editor's settings menu. They're off by
default, and this is saved to the save file.
Anyway, when you're playtesting, the game saves the players position, color, room coordinates and sprite every 3
frames. The max is 100, where if it tries to add more, the oldest one gets removed.
When you exit playtesting, the saved positions appear one at a time, and you can use the Z key to speed it up.
[Here's a video of them in action.](https://o.lol-sa.me/4H21zCv.mp4)
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.
Ok, so it was a bit of a struggle at first figuring out the new API, but
honestly it wasn't so bad in the end.
I made a copy of my old unlock.vvv before testing this, and checking
with `diff` the only difference is the new `encoding="UTF-8"` in the XML
declaration, which isn't a bad thing.
Surprisingly, I only had to change some names and stuff around at the
top of the function. The rest of the function could be left untouched
and it worked fine.
This resulted in two bugs:
1. Custom assets would not be unmounted when quitting to the menu.
2. Custom assets would be unmounted when playtesting a level.
The solution is to unmount assets in Game::quittomenu() instead.
It's a bit bad for the compiler if you have lots of function calls with
hardcoded strings in them, because every time the compiler encounters
one, it has to go out of its way to find a dedicated storage location
for the string, which is really inefficient. And it does this
inefficient thing every single time.
There's not much of an impact compiling these lists, but I at least want
to encourage this sort of code style, instead of the push_back(string)
style, in case we ever need a hardcoded array of things later.
This fixes being able to trigger Undefined Behavior by pressing R when
not in-bounds in the Outside Dimension VVVVVV map, usually when you're
falling upwards towards Game Complete.
I also put bounds checks on normal roomdeaths for good measure. You'll
never know when you need it.
If you started a new game while having had a save (meaning you selectedd
"new game" while it wasn't in the same position as "continue"), then
saved and quit, your cursor will now end up at "continue" instead of
"new game". (If you didn't save, then your cursor would be out-of-bounds
and end up at position 0 anyway.)
For some reason (probably a copy-paste error), this XML tag gets atoi()
called on it before being assigned to Game::hardestroom. And only when
loading a quicksave, at that.
This would result in Game::hardestroom being set to an empty string,
which if you kept until Game Complete, would end up rendering as a
single null byte (if you even have a font face for said null byte).
I'm not sure how this error compiles in the first place, but whatever.
This fixes horizontal and vertical warp backgrounds not resetting, and
also a bunch of other 1-frame glitches, most noticeably cutscene bars
and fadeouts.
This adds a new variable shouldreturntoeditor to Game to signal whether
or not it should return to editor at the end of the frame.
Flag 67 seems to be a general-purpose Game Complete flag. It's used to
replace the CREW option with the SHIP option on the pause screen.
Unfortunately, it gets turned on too early during Game Complete. Right
when Viridian starts to teleport, you can bring up the pause screen and
select the SHIP option.
It will teleport you to the ship coordinates, but still keep you in
finalmode, and since the ship coordinates are at 102,111 (finalmode is
only around 46,54), you'll still be stuck in Outside Dimension VVVVVV,
and you've interrupted the Game Complete gamestate by switching to the
teleporting gamestate. Oh, and your checkpoint is set, too, so you can't
even press R. Oh and since there's no teleporter, and checkpoint setting
doesn't check the sentinel value, this results in Undefined Behavior,
too.
So this results in an in-game softlock. The only option you can do is
quit the game at this point.
To fix this issue, just move turning on flag 67 before the savetele() in
gamecompletelogic2().
Having to rely on a script means the fade out wouldn't be self-contained
in MAPMODE, which could cause a small issue where you could die during
the return to the lab. But that issue is now fixed. There's no need to
use the script, and anyway the endcutscene() and untilbars() in said
script don't do anything because there are no cutscene bars in the first
place, so no need to worry about those.
Again, what I've done here is removed the over-reliance on Terry's State
Machine to return to the lab, and just moved it into separate variables
instead. This means that returning to the lab is ALMOST entirely
self-contained in MAPMODE, except there's a quick jaunt over to GAMEMODE
to run a script because you can only run scripts in GAMEMODE.
Alright, so what I've done here is made exiting to the menu entirely
separate from Terry's State Machine, and thus it can now take place
entirely within MAPMODE instead of having to go back to GAMEMODE. Also,
it's faster by 15 frames since we don't need to wait for the map screen
to go back down.
This cleans up a whole lot of kludge variables, because this aggressive
hardreset() right as ACTION is pressed doesn't do anyone any favors.
This aggressive hardreset() was probably here because of the whole fact
that exiting to the menu uses Terry's State Machine, to minimize the
chances of interruption, but it actually causes more issues and allows
towers to interrupt the fadeout. And we should fix the root cause (the
usage of the state machine) instead of patching together some kludge.
I don't want the quit code to only be in the state machine, but I'll
keep the gamestate around for compatibility reasons (there are custom
levels that directly use gamestate 80 to quit to the menu).
When you complete the game, you're now redirected to the play menu. This
is because your quicksave will have been deleted so you can't go back to
the summary menu.
When you complete a custom level, you'll go back to the levels list, in
case you started the level from a quicksave.
I want exiting No Death Mode to go back to the "play modes" menu, not to
the "start game" menu, because it's too far back. Also do the same if
you either die or complete No Death Mode.
Also I initialized Game::wasinintermission, probably a good thing to
initialize variables.
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.
This commit fixes a slightly frustrating thing where if you start a new
game, and then exit before saving, "start game" will always take you to
a new game, even though you have unlocked things like the Secret Lab or
Time Trials.
Now, if you select "new game" (only possible if you have something
unlocked), then quit before saving, "start game" will still take you to
the play menu, but "continue" is replaced with "start" and "new game" is
gone.
This will be a useful shorthand to ask "do we have the Secret Lab, or
any Time Trial, or Intermission replays, or No Death Mode, or Flip Mode
unlocked?"
This stabilizes the code that handles the menu that you land on if you
press Esc and quit to the menu.
Instead of using Game::returnmenu(), we now use the new function
Game::returntomenu() to clearly express intent that we want to return to
a specific menu. So I've added another kludge variable
Game::wasinintermission for the was-in-intermission case.
Also, I made it so that if you didn't have a main game telesave or
quicksave, you just get brought back to the main menu. Because you
shouldn't be able to go to the play menu without a quicksave or
telesave.
When exiting from a game-gamestate which may have been entered through a
varying amount of menus, the solution is to not use Game::returnmenu(),
and to instead have a way to go back to a certain given menu.
It looks like this variable was originally intended to keep track of th
volume of the game, but then it was used as a boolean in main.cpp to
make sure the game didn't call Mix_Volume() and Mix_VolumeMusic() every
frame.
However, it is now a problem, because I put the music mute handling code
in the very branch that game.globalsound protects against, but since
game.globalsound is here, if I mute the music, then mute the whole game,
then unmute the music, and then unmute the whole game, sound effects
will no longer be muted but the music will still be muted, until I mute
and unmute the whole game again. This is annoying and inconsistent, so
I'm removing this check from the 'if (!game.muted)' branch.
Plus, given that the Mix_VolumeMusic() and Mix_Volume() calls happen
every frame if the game is muted anyways, it doesn't seem to be a
problem to call these every frame.
These do basically nothing. The only time they're used is
getGlobalSound() in an if-statement in main.cpp, but all that
if-conditional does is call setGlobalSound() anyway, which is something
that doesn't really have any side effects. So I'm removing these vars to
simplify the code.
This is for people who want to use their own soundtrack while playing
the game, but who don't want to mute the sound effects as well.
This feature was added to VCE, but it was added in the strangest way. It
was made an option in "game options" instead of being a keybind, and I
don't know why.
The problem was, if you were in a time trial and quit, it wouldn't go
back to selecting your current time trial. But also if you were in a
custom level and quit, you would still be on the playerworlds menu.
The problem was twofold: first, I simply wasn't doing the custommode
check. But secondly, I couldn't use map.custommode directly, because
whenever you quit the game aggressively hardreset()s everything
immediately when you press ACTION.
There's probably a good reason for that aggressive hardreset(), so I
won't touch that hardreset() in any way. Instead, I had to introduce two
kludge variables wasintimetrial and wasincustommode to Game, and use
those to do the check proper.
This makes it more convenient if you have a large levels directory, as
some people in the VVVVVV custom levels community do.
On the first page, this option will change to be "last page" instead.
Since the addition of another menu option pushes up the list of levels
too close to the selected level data itself, I've had to move the list
of levels down by 4 pixels (but "next page"/"previous page"/"return to
menu" are still in their same position).
This feature was already added to VCE but hasn't been upstreamed until
now.
This also replaces some createmenu()s with returnmenu()s as needed even
when said createmenu()s already didn't go to the main menu.
Now when you exit the level editor, you'll be selecting the "level
editor" option in "play levels", and if you exit from a level you'll
still be selecting that level in the levels list.
Furthermore, regardless of what you're exiting, your cursor position
will be remembered.
You were able to mismatch the color of the quicksave/telesave summary
and the text/background by pressing Esc when in the "continue" menu,
then pressing ACTION on "no, return".
This commit fixes that bug by putting the map.settowercolour(3) inside
the Menu::continuemenu creation code itself. However, since the
Menu::youwannaquit code does map.nexttowercolour() right after it does
the game.createmenu(), we also need to put the map.nexttowercolour()
before the game.createmenu() beforehand so it doesn't mess up the cyan
color that Menu::continuemenu sets.
Additionally, I removed the map.settowercolour() from the input handling
of Menu::play, as it's superfluous.
Unless it's the main menu, or unless it's not the same menu. Whether or
not the menu is the same is left up to the caller, because some menus
could be the same but use different names, so we can't simply
automatically check that the names are different and assume that they
aren't the same menu.
This temp variable isn't used anywhere else, and even if it was it's set
to something every time it's used, so there's no risk of this commit
breaking any backwards compatibility.
Looks like it was a remnant from the Flash days, and the "delete your
saves if you want to use slowdown" was a bit too mean so it stopped
being a thing in the C++ version.
Much more stylistic, you don't need to repeat "game.currentmenuname" for
each case, and you don't need to deal with the dangling first "if" that
doesn't have an "else".
I presume it was meant to have the text of the currently-selected menu
option inside it, before the code switched over to using the indice of
the currently-selected menu instead? Would've been more error-prone to
use the text name directly.
Stringly-typed things are bad, because if you make a typo when typing
out a string, it's not caught at compile-time. And in the case of this
menu system, you'd have to do an excessive amount of testing to uncover
any bugs caused by a typo. Why do that when you can just use an enum and
catch compile-time errors instead?
Also, you can't use switch-case statements on stringly-typed variables.
So every menu name is now in the enum Menu::MenuName, but you can simply
refer to a menu name by just prefixing it with Menu::.
Unfortunately, I've had to change the "continue" menu name to be
"continuemenu", because "continue" is a keyword in C and C++. Also, it
looks like "timetrialcomplete4" is an unused menu name, even though it
was referenced in Render.cpp.
If you have invincibility mode or slowdown enabled, the game will not
let you select the Secret Lab, Time Trials, or No Death Mode. To make
this clearer, this commit grays out said options if they are disabled
for that reason.
Previously, the code looked something like:
else { if (...) {...} else { if (...) {...} else { etc. } }
And kept indenting every time there was an else-if.
This commit puts all else-ifs on the same indentation level, so it
doesn't slowly push the code to the right.
If we need it (which I don't think we will be anytime soon) we can
always just get it back through source control. Otherwise, it simply
gets in the way.
Firstly, menu options are no longer ad-hoc objects, and are added by
using Game::option() (this is the biggest change). This removes the
vector Game::menuoptionsactive, and Game::menuoptions is now a vector of
MenuOption instead of std::string.
Secondly, the manual tracker variable of the amount of menu options,
Game::nummenuoptions, has been removed, in favor of using vectors
properly and using Game::menuoptions::size().
As a result, a lot of copy-pasted code has been removed from
Game::createmenu(), mostly due to having to have different versions of
menus depending on whether or not we have certain defines, or having an
mmmmmm.vvv file inside the VVVVVV directory. In the old days, you
couldn't just add or remove a menu option conveniently, you had to
shuffle around the position of every other menu option too, which
resulted in lots of copy-pasted code. But now this copy-pasted code has
been de-duplicated, at least in Game::createmenu().
It's treated like a bool anyway, so might as well make it one.
This also necessitates updating every single instance where it or an
element inside it is used, too.
Looks like it was here from that arg passing stuff from earlier, as a
workaround to not pass args around. Well, there's no need to create an
extra UtilityClass now either, just use the one in the global namespace.
Same principle as removing the separate variable to track number of
collected trinkets. This means it's less error-prone as we're no longer
tracking number of trinkets separately.
In the function that counts the number of trinkets, I would've liked to
have used std::count_if(). However, the most optimal way would require
using a lambda, and lambdas are too new for the C++ standard we're
using. So I just bit the bullet and counted them manually.
game.trinkets is supposed to be correlated with obj.collect, however why
not just count obj.collect directly?
This turns game.trinkets into a function, game.trinkets(), which will
directly count the number of collected trinkets and return it. This will
fix a few corner cases where the number of trinkets can desync with the
actual collection statuses of trinkets.
In order to keep save compatibility with previous versions of VVVVVV,
the game will still write the <trinkets> variable. However, it will not
read the <trinkets> variable from a save file.
This removes the variables obj.nentity and obj.nlinecrosskludge, as well
as removing the 'active' attribute from the entity class object. Now
every entity you access is guaranteed to be real and you don't have to
check the 'active' variable.
The biggest part of this is changing createentity() to modify a
newly-created entity object and push it back instead of already
modifying an indice in obj.entities.
As well, removing an entity now uses the new obj.removeentity() function
and removeentity_iter() macro.
Most of the code was already commented out, and those comments were
removed in earlier commits, but this removes all recording variables
from Game and simplifies the game-gamestate handling in main.cpp a
little bit.
Most of this is telecookie/quickcookie stuff, which was used in the
Flash version, but there's no longer any such thing as a save cookie.
Also one TODO that says to make a function that's now been made.
Unlike, say, the scriptclass i/j/k stuff, these tempstrings are only
purely visual, and there's no known glitches to manipulate them anyway.
Thus I think it's safe to make this cleanup.
It looks like this may have been used earlier in development, judging
from the name, obviously, but right now it seems like it's used as an
error message if a main game level is asked for an invalid room (well,
only two of them - the Lab and Warp Zone). It should probably be
formalized into an error system, if we want to keep teststring, and also
people would never see it anyway because I don't think there's a
reliable and consistent way to trigger loading a non-existent room.
I have seen someone manage to load a non-existent Warp Zone room only
one time, but even then this teststring didn't pop up. So this
teststring doesn't even trigger in the right circumstances.
Also, when it does pop up, as far as I can tell it will stay onscreen,
which is kinda annoying. So I'm just removing this ancient relic from
the code.
This removes all indentation that suddenly switches in the middle of a
function. Most particularly egregious offenses are the ones made by the
person who has 2-wide tabs, but keeps tabbing up to make each
indentation level match up with the 4-wide spaces, so to them (and only
them) it will look just fine, but since by default tabstop is 8-wide,
their lines are pushed off all the way to the right.
