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.
If you exited from the editor, custom assets would not be unmounted. But
I made sure to put the FILESYSTEM_unmountassets() before the
music.play(6) because otherwise the menu music wouldn't play.
You could also exit to the menu from a custom level using the
rollcredits() command, so I made sure to put a
FILESYSTEM_unmountassets() when returning to the menu from the credits
as well. I also made sure to put it before the music.playef(18) so
there's no risk of the sound effect not playing properly, or not playing
the non-level-specific one.
I added a comment to both FILESYSTEM_unmountasset()s to make sure anyone
reading the code is aware of the frame order dependency.
This is really awful, but there's not much we can do.
TinyXML-2 no matter what will never stop on newlines, so without
changing the XML parser, this is the best we can do - just remove the
"\n " (that's a linefeed plus exactly 12 spaces) if it
appears at the end of the contents of an edentity tag.
Also a giant comment for good measure.
Looks like duplicate player entities persisting across rooms is a
semi-useful feature used by some levels. Still, though, it's a bit of a
nuisance to have duplicate player entities persisting across game
sessions. And levels can't rely on this persistence anyway, anyone could
just close the game and re-open it to get rid of the duplicate entities
regardless.
If a trinket or crewmate ID is out-of-bounds, it will not be created.
This is not only because the `collect`/`customcollect` check in
entityclass::createentity() would then be out-of-bounds, but also
touching it would also be out-of-bounds, too.
Display trinkets will always be created if the ID is out-of-bounds.
Apparently some people (@AllyTally) have been creating display trinkets
with IDs of -1 in order to get a display trinket that always shows up,
which is rather horrifying. But it makes sense, there's a lot more
nonzero int values than there are the amount of int values that are
zero, so it's fairly likely that the `collect` check will always come up
to be true (nonzero). Also, it's more useful to be able to have a
display trinket that always shows up without having to collect a trinket
beforehand, than it is to have it not be created (because technically by
default, you're already in the world where you don't have it created).
Display trinkets still have their `para` set to their ID, though, and if
they managed to gain an `onentity` of 1, bad things could happen... So
just to be sure, I added INBOUNDS checks to crewmates and trinkets in
entityclass::updateentities() so no UB will happen if you collect a
crewmate/trinket with an out-of-bounds ID. Also, I de-duplicated the
`collect`/`customcollect` setting, too.
The flashy color of the elephant can be hard on people's eyes,
especially if they're the type who want screen effects disabled because
they might have epilepsy. The elephant takes up a good 3/4ths of the
screen, you know. If screen effects are disabled, the elephant will use
color 22, which is a neutral gray.
I'm only adding this because the VVVVVV speedrun mods (@tzann, @mohoc)
invalidate all runs that have the elephant texture removed, even though
many people would be looking at a potentially epilepsy-inducing image
many times a day grinding 100% speedruns. (Imo, their justification for
this is flimsy at best.)
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).
If you don't have a font.txt, it could happen that a font index is
requested that's out-of-bounds. And that would result in a segfault. So
to fix that I'm adding INBOUNDS checks to all functions that index the
fontmap.
This fixes indexing out-of-bounds in the functions that draw all the
special images such as the elephant and teleporters. Let's make sure the
game doesn't segfault.
I tracked down all the functions that took in an entity's drawframe and
made sure that no matter what value an entity's drawframe was, the game
would never segfault.
To deal with using a different image file for Flip Mode, it looks like
copy-paste was used. This isn't exactly maintainable code. So I'm
replacing it with a reference that changes depending on if the game is
in Flip Mode or not, instead.
Removing the player entity has all sorts of nasty effects, such as
softlocking the game because many inputs require there to be a player
present, such as opening the quit menu.
The most infamous glitch to remove the player entity is the Gravitron
Fling, where the game doesn't see a gravity line at a specific
y-position in the current room, and when it moves the bottom gravity
line it moves the player instead. When the gravity line gets outside the
room, it gets destroyed, so if the player gets dragged outside the room,
they get destroyed, too. (Don't misinterpret this as saying anytime the
player gets dragged outside the room, they get destroyed - it's only the
Gravitron logic that destroys them.)
Also, there are many places in the code that use entity-getting
functions that have a fallback value of 0. If it was possible to remove
the player, then it's possible for this fallback value of 0 to index
obj.entities out-of-bounds, which is not good.
To fix this, entityclass::removeentity() is now a bool that signifies if
the entity was successfully removed or not. If the entity given is the
player (meaning it first checks if it's rule 0, just so in 99% of cases
it'll short-circuit and won't do the next check, which is if
entityclass::getplayer() says the indice to be removed is the player),
then it'll refuse to remove the entity, and return false.
This is a change in behavior where callers might expect
entityclass::removeentity() to always succeed, so I changed the
removeentity_iter() macro to only decrement if removing the entity
succeeded. I also changed entityclass::updateentities() from
'removeentity(i); return true;' to 'return removeentity(i);'.
Apparently it results in Undefined Behavior if the argument given isn't
representable as an unsigned char. This means that (potentially) plain
char and signed chars could be unsafe to use as well.
It's rare that this could happen in practice, though. std::isdigit() is
only used by is_positive_num() which is only used by find_tag(), so
someone would have to deliberately put something crazy after an `&#` in
a custom level file in order for this to happen. Still, better to be
safe than sorry and all.
This fixes a compile error that could happen where the compiler doesn't
know what std::isdigit() is, but I'm puzzled as to why this wasn't
happening earlier, 'cause I've only been reported that it happens by
only one person.
Continuing from #280, another potential source of out-of-bounds indexing
(and thus, Undefined Behavior badness) comes from script commands. A
majority of them don't do any input validation at all, which means the
potential for out-of-bounds indexing and segfaulting in custom levels.
So it's always good to add bounds checks to them.
Interesting note, the only existing command that has bounds checks is
the flag() command. That means you can't turn out-of-bounds flags on or
off. But there's no bounds checks for ifflag(), or customifflag(), which
means you CAN index out-of-bounds with those commands! That's a bit bad
to do, so.
Also, I decided to add the bounds checks for playef() at the
musicclass::playef() level, instead of just the level of the playef()
command. I don't know of any other cases outside of the command where
musicclass::playef() will index out of bounds, but musicclass is the one
containing the indexed vector anyway, I wanted to cover more cases, and
it's better to be safe than sorry.
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)
2.2 and earlier had this god-awful thing where it put the closing tag of
an edentity onto the next line, and then kept the indentation the same.
This requires parsing the XML in an extremely specific way (i.e.
ignoring the whitespace) so the newline and indentation isn't taken as
part of the actual contents of the tag.
2.3 removed this awful whitespace entirely to make it easier on parsers.
When I tested #270, I tested against a 2.3 re-save of Dimension Open and
diffed the two, because I thought testing against the original version
of the level would result in a bunch of noise I didn't want due to the
whitespace change. Well, I did exactly what I intended, and ended up
ignoring the whitespace change so much that levels saved in this stupid
format ended up getting broken.
Luckily, we can just tell TinyXML-2 to parse a document exactly like how
TinyXML-1 would've parsed it, by supplying the COLLAPSE_WHITESPACE enum
to it (by default it's on PRESERVE_WHITESPACE).
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.
Seems a bit wasteful to do the whole "parse the XML document thing"
instead of a simple file check. It doesn't even fail if the XML document
is invalid, but whatever.
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.
Some of the file was indented with two spaces and the rest indented with
tabs. It feels like two different people worked on the file, one more
than the other. Since most of it uses two spaces, I'll just replace the
tabs with two spaces.
This is to respect the fact that the top half of the file is indented
with spaces, while the bottom half is indented with tabs.
Graphics::reloadresources() is on the bottom half.
The previous way manually concatenated the first 7 characters of the
string together (and had an std::min() calculation). The new way instead
does std::string::substr(), which is much more snappy.
The actual unindent is done in a separate commit to minimize noise,
because diffs are terrible at clearly conveying unindents (it should put
all the minus lines together and all the plus lines together, too).
The entirety of the rest of scriptclass::loadcustom() is encased in a
block that first checks if the script with the name even exists. Instead
of indenting the rest of the function, just invert the check and reduce
indentation level.
This commit refactors custom level scripts to no longer be stored in one
giant vector containing not only every single script name, but every
single script's contents as well. More specifically,
scriptclass::customscript has been converted to an std::vector<Script>
scriptclass::customscripts (note the extra S), and a Script is just a
struct with an std::string name and std::vector<std::string> contents.
This is an improvement in both performance and maintainability. The game
no longer has to look through script contents in case they're actually
script names, and then manually extract the script contents from there.
Instead, all it has to do is look for script names only. And the
contents are provided for free. This results in a performance gain.
Also, the old system resulted in lots of boilerplate everywhere anytime
scripts had to be handled or parsed. Now, the boilerplate is only done
when saving or loading a custom level. This makes code quality much,
much better.
To be sure I didn't actually change anything, I tested by first saving
Dimension Open in current 2.3 (because current 2.3 gets rid of the
awful edentity whitespace), and then resaved it on this patch. There is
absolutely no difference between the current-2.3-resave and
this-patch-resave.
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.
Main game would retain custom level assets, now fixed. Also, custom fonts load properly. Finally, levels can be stored as a zip and placed in the levels folder, with the .vvvvvv file at the root of the zip and custom asset folders (graphics, sounds etc) also at the root.
Previously, the editor would always say it saved or loaded a level,
even if it was not successful. For example, because a file to load does
not exist, a file to save has illegal characters in its name or the
name is too long to be stored. Now failure is reported. Also, when
quitting the editor and saving before quitting is unsuccessful, the
editor will abort quitting.
I think it's a bit silly to always include the Steam and GOG APIs
whenever we build VVVVVV, since the only time they'll ever be used is in
a live build and not a dev build.
So now Steam and GOG are disabled by default. If you want them, you'll
need to add -DSTEAM=ON or -DGOG=ON respectively at CMake time. They're
also both automatically enabled for release builds.
This commit adds bounds checks to those commands in case say()/reply()
asks for more lines than there are left in the all-script-lines buffer
(not just the current script, so in order for it to segfault your script
has to be last in the all-script-lines vector), and in case text() asks
for more lines than there are in the rest of the rest of the parsed
internal script.
For some reason, scriptclass::loadcustom() sometimes refers to
`customscript` as `script.customscript` instead of `customscript`. The
`script.` is unnecessary.
Although it's not an issue, this should minimize the stack footprint of
calling scriptclass::load(), especially if it goes down to calling
scriptclass::loadcustom() or scriptclass::loadother().
Otherwise, it would end up indexing a vector out-of-bounds, which is UB
and causes a segfault, too. This is used in some constructs like
say(-1)
<internal command>
where the text contents don't matter, only that a text box shows up.
The `speak` command is the exact same as the `speak_active` command,
except without one line of code. So instead of copy-pasting the entire
thing, it's better to just combine them into the same chunk of code.
Instead each line is now held in a const char* array, like it should be.
This results in less work for the compiler, especially with
optimization, since every time the compiler encounters a constant
argument in a function, it has to go off and locate a place to put it.
But if we're upfront and say, hey, here's all the strings we ever need
conveniently packaged into one place, it'll be much more cooperative.
I have the feeling that none of the devs understood what extern did, and
they kind of just sprinkled it everywhere until things started working.
But like all other classes, it should just be one line in the class's
respective header file, and shouldn't be so messy.
When the game loads a room in a custom level, previously it would load
the tilemap of that room into ed.swapmap, and then mapclass::loadlevel()
would manually go through each element in ed.swapmap to set each tile in
`contents`. Why do that, when you can just return the vector from
editorclass::loadlevel() and set it directly? ed.swapmap is really
unnecessary.
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.
Resetting game.activeactivity fixes triggering Undefined Behavior if you
quit to the menu while standing inside an activity zone, and then
re-entered the game. Resetting game.act_fade also fixes the activity
zone prompt fadeout that happens once the above segfault is fixed.
Don't worry, other activity-zone like variables such as teleporter
prompts and trophy text are already covered. obj.trophytext is reset in
scriptclass::hardreset(), too, and game.readytotele is reset every time
mapclass::gotoroom() is called, and mapclass::gotoroom() is called every
time a gamemode is started.
For some reason, the only way to get a cyan crewmate is by cycling
through an already-existing crewmate by keeping left-clicking on it.
This is because when you cycle through crewmate colors, the allowed
colors are 0-5, but when you place down a crewmate, it picks a random
color from 1-5, which seems to be a bit consistent.
So placing and cycling a crewmate now use the same color ranges.
Blackout mode doesn't work properly, because the game doesn't actually
black out the screen, it merely stops drawing things. Oh and only some
things at that, some other things are still drawn. This results in a
freeze-frame effect, which is apparently fixed in the Switch version.
Custom levels utilize this freeze-frame effect, so it's a bit annoying
that the "Game paused" screen draws on top of it and makes it so that
the freeze-frame freezes on "Game paused" until blackout is turned off.
So I'm making it so that "Game paused" doesn't get drawn in blackout
mode. However it will still do graphics.render() because otherwise it'll
just result in a black screen.
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.
Instead of using somewhat-obtuse for-loops to initialize or reset these
vectors, it takes up less lines of code and is clearer if we use
std::vector::resize() and std::vector::clear() instead.
Since translucent roomname backgrounds were introduced in
TerryCavanagh/VVVVVV#122, it exposes one glaring flaw with the game that
until now has been kept hidden: in rooms with room names, the game
cheapens out with the tile data and doesn't have a 30th row, because the
room name would hide the missing row. As a result, rooms with room names
have 29 rows instead of 30 to fill up the entire screen. And it looks
really weird when there's nothing but empty space behind the translucent
room name background.
To remedy this, I added one row to each room with a room name in the level.
First, I had to filter out all the rooms with no room names. However, that's
actually all contained in Otherlevel.cpp, the Overworld, which contains 221
rooms (8 of which are the Secret Lab, 6 more of which are the Ship, so 207 are
the actual Overworld, right? Wrong, 2 of those Overworld no-roomname rooms are
in the Lab, so there are actually 205 Overworld rooms). The remaining level
data files all contain rooms with room names.
But the process wasn't that easy. I noticed a while ago that each room
contains 29 `tmap.push_back()`s, one for each row of the room, and each row is
simply a string containing the 40 tiles for that row, concatenated with
commas.
However, I decided to actually check my intuition by doing a grep on each
level file and counting the number of results, for example `grep 'push_back'
Labclass.cpp | wc -l`. Whatever number comes out should be divisible by 29.
That particular grep on Labclass.cpp returns 1306, which divided by 29 is 45
with a remainder of 1.
So what does that mean? Does that mean there's 45 rooms each, and 1 leftover
row? Well, not exactly. The extra row comes from the fact that Outer Space has
30 rows instead of 29. Outer Space is the room that comes up when the game
finds a room is non-existent, which shouldn't happen with a properly-working
game, except in Outside Dimension VVVVVV. In fact, each level file has their
own Outer Space, and every single Outer Space also has 30 rooms. So really,
this means there are 44 rooms in the Lab and one Outer Space room. (Well, in
reality, there are 46 rooms in the Lab, because 2 of them use the Outside
tileset but have no room names, so they're stored in Otherlevel.cpp instead.)
We find the same result for the Warp Zone. `grep 'push_back' WarpClass.cpp |
wc -l` returns 697, which is 24 remainder 1, meaning 23 rooms of 29 rows and 1
room of 30 rows, which corresponds with 23 rooms in the Warp Zone and one
Outer Space room.
However, Outside Dimension VVVVVV + Tower Hallways and Space Station 1 and 2
are both odd curiosities. Finalclass.cpp contains Outside Dimension VVVVVV,
(which is Intermission 1 and 2 and the Final Level), but also the Tower
Hallway rooms, i.e. the auxiliary Tower rooms that are not a part of the main
tower. Spacestation2.cpp contains both Space Station 1 and 2, so don't be
deceived by the name.
`grep 'push_back' Finalclass.cpp | wc -l` returns 1597, which is actually 55
remainder 2. So... are there two rooms with 30 rows? Yes, in fact, The
Gravitron and Outer Space both contain 30 rows. So there are actually 55 rooms
stored in Finalclass.cpp (not including the minitowers Panic Room and The
Final Challenge), 54 rooms of actual level data and one Outer Space room, and
breaking down the 54 rooms even further, 51 of them are actually in Outside
Dimension VVVVVV and 3 of them are Tower Hallways. Of the 51 Outside Dimension
VVVVVV rooms, 14 of those are Intermission 1, 4 of them are Intermission 2,
and the rest of the 33 rooms are the Final Level (again, not including the
minitowers).
`grep 'push_back' Spacestation2.cpp | wc -l` returns 2148, which is 74
remainder 2. Are there two rooms with 30 rows again? No; one of those counted
2148 rows is a false-positive, because there's an if-else in Prize for the
Reckless that replaces the row with spikes with a row without spikes if you
are in a time trial or in No Death Mode. So there's 73 rooms in Space Station
1 and 2, and one Outer Space room.
With all this in mind, I decided to duplicate the current last row of each
room, the 29th row, to add a 30th row. However, I wasn't going to do this
automatically! But neither was I going to write some kludge-y code to parse
each nightmare of a level file and duplicate the rows that way.
Enter: Vim macros! (Er, well, actually, I use Neovim.) I first did
`/push_back`, so that pressing `n` would keep going to the next `push_back` in
the file. Then I went to the 29th row of the first room in the file, did a
`Yp`, and then started my macro with `qq`. The macro went like this: `30nYp`,
which is simply going to the 29th row of the next room over and duplicating
it. And that's all there was to it. However, I had to make sure that (1) my
cursor was before the `push_back` on the line of the 29th row of the room, and
(2) that I didn't skip rooms, both of which were problems I encountered when
pressing Ctrl+Z a given invocation of the macro (the Ctrl+Z is just a
metaphor, you actually undo by typing `u` in Vim). And also I had to make sure
to be careful around the extra lines of `push_back`s in Prize for the Reckless
and The Gravitron, and make sure I didn't run past the end of the file and
loop back around. Thankfully, all Outer Space rooms are at the end of each
file.
But first, I had to increase the number of rows drawn in Graphics.cpp by 1 in
order to compensate for this, and do the same when reading the tile data in
Map.cpp. I had to change fillcontent(), drawmap(), drawfinalmap(),
drawtowermap(), and drawtowermap_nobackground(). Funnily enough, the tower
functions already used 30 rows, but I guess it's an off-by-one due to the
camera scrolling, so they now draw 31 rows each.
Then, I went in-game to make sure that the row behind each room name looked
fine. I checked EVERY single room with a room name. I turned on invincibility
mode and added a temporary line to hardreset() that always turned on
game.nocutscenes for a smoother playtesting experience. And to make sure that
rooms which have entirely empty bottom rows actually still have 30 rows,
instead of having 29 and the game assuming that the 30th row was empty
(because that sounds like it could lead to Undefined Behavior), I added this
temporary debugging line to the start of mapclass::fillcontent():
printf("(%i,%i) has %i rows\n", game.roomx, game.roomy, (int) tmap.size());
Everywhere I checked - and I made sure to check all rooms - every room had 30
rows and not 29 rows.
Unfortunately, some rooms simply couldn't be left alone with their 29th row
duplicated and had to be manually edited. This was because the 29th row would
contain some edge tiles because the player would be able to walk somewhere on
the 28th, 27th, and 26th rows, and if you duplicated said edge tiles behind
the room name, it would look bad.
Here's a list of rooms whose 30th rows I had to manually edit:
- Comms Relay
- The Yes Men
- Stop and Reflect
- They Call Him Flipper
- Double-slit Experiment
- Square Root
- Brought to you by the letter G
- The Bernoulli Principle
- Purest Unobtainium
- I Smell Ozone
- Conveying a New Idea
- Upstream Downstream
- Give Me A V
- $eeing Dollar $ign$
- Doing Things The Hard Way
- Very Good
- Must I Do Everything For You?
- Now Stay Close To Me...
- ...But Not Too Close
- ...Not as I Do
- Do Try To Keep Up
- Whee Sports
- As you like it
This is actually a strange case where it looked bad because of the 29th
row, instead of the 30th row, and I had to change the 29th row instead of
the 30th row to fix it.
- Maze With No Entrance
- Ascending and Descending
- Mind The Gap
Same strange case as "As you like it" (it's the 29th row I had to change
that was the problem, not the 30th).
- 1950 Silverstone Grand V
- The Villi People
I found that Panic Room and The Final Challenge also looked strange behind the
roomname background, but I can't do much about either because towers' tile
data wrap around at the top and bottom, and if I added another row to either
it would be visible above the room name.
I've considered updating the development editors with these new level tiles,
but I decided against it as the development editors are already pretty
outdated anyway.
This didn't cause any issues on the old push_back(string) system, but it
causes a huge alignment issue now. Don't know why these 7 zeroes are
here, but I'm fixing them.
It's a bit inconsistent how every time you toggle flip mode, it does
this flashing and shaking (and different SFX), regardless of whether or
not you're turning it on or off. To be more consistent with what happens
when you toggle screen effects, only turning on Flip Mode should do the
flashing/shaking/game-saved sound, and turning it off should just play
the Viridian squeak.
It's always personally irked me that the only time you get a Viridian
squeak when pressing ACTION to start fading out and going in-game is
when you start playing a custom level. And that's only if you don't have
a quicksave.
To make things more consistent (and to add more polish to the game), I
made sure there was a music.playef(11) every time game.mainmenu gets
set. I also made sure that this doesn't result in double-squeaking, i.e.
music.playef(11) being called twice, which can be very loud and
ear-hurting.
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.
Whenever I compile with -O2, GCC gives me a warning that the return
value of fread() is being ignored. Which is a fair point, it means that
we're not doing proper error handling if something goes wrong. But I'm
also going to check the return value of fwrite() for good measure.
I believe that checking that this number is not equal to length is the
way to catch all errors and output an error message accordingly. I
didn't use ferror() or feof() mostly because I think it takes up too
much code. Also an error from fwrite() only says "Warning" because I
don't think there's much we can do if we don't fully write all bytes to
the intended file.
Previously, it was used to parse 30 strings whenever loading a room. But
now it's no longer used since we just assign the tilemap to the vector
directly.
They are now stored in const int arrays instead. Except for the Prize
for the Reckless room, which I made sure had its spikes removed in No
Death Mode and the Time Trial.
Instead, they're all stored in a constant int array.
I made sure The Gravitron still has 30 rows just like Outer Space,
though I don't think it matters.
Previously:
- Linux: xdg-open
- Everything else: open
Now:
- macOS and Haiku: open
- Everything else: xdg-open
This is all according to a comment by leo60228 in PR #203.
This change makes it so that in in-editor playtesting, the code to
handle pressing ENTER on activity zones is the same code to handle
pressing ENTER on activity zones in custommodeforreal.
This removes the need to copy-paste code and adapt it to in-editor
playtesting. And thus, this fixes an editor artifact where you can press
ENTER on activity zones while doing the death animation, even though you
can't do that when you're playing custom levels normally.
Since this is at the start of maprender(), the text boxes drawn by this
function will get hidden behind everything else being drawn on top of
it. Thus, it'll only result in a glitchy rendering where the text boxes
at the very top of the screen will be rendered in the roomname space of
the map screen.
To fix this rendering glitch, just remove the drawgui(). It's useless
anyway since it's being drawn behind everything else, so no need to have
it here.
We need to replace an "or" with an "and".
My best guess for this oversight happening was because of the weird
ordering. The code originally did "temp < 30" first and "temp > -30"
second instead of the other way around. With the weird ordering, it
becomes more natural to insert an "or" instead of an "and". So I swapped
around the ordering just for good measure.
This is also fixed in the mobile version.
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.
It's a bit annoying how vvvvvvman status is preserved between in-game
sessions, and the only thing reset is the color. This is annoying
because it means you have to close the game to reset vvvvvvman.
But now it'll be reset properly. I chose to put this reset code in
mapclass::resetplayer() instead of scriptclass::hardreset() because it
seemed like the more appropriate place. It's where all other properties
of the player are reset, after all.
This is to be extra safe and ensure that your hard-earned record isn't
lost at all.
In 2.2, the game didn't save your Super Gravitron record at all. It only
saved it if you closed the game by quitting to the title screen and
pressing ACTION on "quit game". You couldn't press Alt+F4, and you
couldn't press X, you had to do it that way, otherwise your record would
be lost.
In 2.3 right now, the game WILL save your unlock.vvv when you close the
game gracefully by any means, but this still means that if it doesn't
otherwise close gracefully (like, say, a crash), it won't save your
record. It feels like we shouldn't rely on this catch-all saving to save
Super Gravitron records.
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().
This makes it so if you bring up the quit screen during a faded-out
screen, you will at least see the screen and won't see black. And also
the fade-in and fade-out animations will still work on the quit screen.
And more importantly, I tested and there's no 1-frame flicker or
anything.
This was used by the old system, which also had an over-reliance on
Terry's State Machine. And due to the fact that it relied on fademode,
it also meant that bringing up the pause screen while faded-out would
result in the player getting sent back to the menu, so one accidental
Esc press during a cutscene could mean countless hours of progress lost
(especially in custom levels).
This would cause the editor to think that it itself is in the middle of
fading to the menu, and so then will fade to the menu, thus losing any
unsaved work without warning.
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).
Due to the previous commit, these will no longer be regularly taking in
out-of-bounds entity indices. Or at least they shouldn't, so I'm putting
in these print statements here on the off-chance that they do.
Otherwise, this would result in the game updating an entity twice, which
isn't good. This is most noticeable in the Gravitron, where many
Gravitron squares are created and destroyed at a time, and it's
especially noticeable during the part near the end of the Gravitron
where the pattern is two Gravitron squares, one at the top and bottom,
and then two Gravitron squares in the middle afterwards. The timing is
just right such that the top one of the two middle ones would be
misaligned with the bottom one of the two when a Gravitron square gets
outside the screen.
To do this, I changed entityclass::updateentities() into a bool, and
made every single caller check its return value. I only needed to do
this for the ones preceding updateentitylogic() and
entitymapcollision(), but I wanted to play it safe and be defensive, so
I did it for the disappearing platform kludge, as well as the
updateentities() within the updateentities() function.
Apparently, the game just leaves minitowermode on, which can cause
issues if you're in a horizontal warping room (and not any other room
type, due to how frame ordering works). This would manifest itself as a
wrong warp to (20,20) because the game is trying to apply the hardcoded
minitower screen transitions when it shouldn't be.
The main ones to beware of here are entityclass::updateentities(),
entityclass::updateentitylogic(), and entityclass::entitymapcollision().
They would index out-of-bounds and thus commit Undefined Behavior if the
entity was removed in entityclass::updateentities(). And it would've
been fine enough if I only added bounds checks to those functions.
However, I decided to be a bit more defensive and play it safe, and
added bounds checks to ALL functions taking in not only an entity
indice, but also blocks and linecrosskludge indices.
Otherwise, if you died after entering a room with a horizontal or
vertical warp background (but not the all-sides warp background), the
warp background would be the first thing you see when going to the Game
Over screen, and would then start scrolling downwards with the proper
tower background coming in from the top.
This oversight seems to have always been in the game.
Was No Death Mode actually tested? Like, did anyone ever play through
the entire game without dying in the Warp Zone, or even AFTER completing
the Warp Zone, like, ever?
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.
Looks like there wasn't a custommode check for the spawning of crewmates
based on which crewmates were rescued, but now there is.
This has gone undiscovered for a long time, mostly because people don't
use the rescued() internal command.
While I was working on my over-30-FPS patch, I found out that the tower
background in the credits scroll was being completely re-drawn every
single frame, which was a bit wasteful and expensive. It's also harder
to interpolate for my over-30-FPS patch. I'm guessing this constant
re-draw was done because the math to get the surface scroll properly
working is a bit subtle, but I've figured the precise math out!
The first changes of this patch is just removing the unconditional
`map.tdrawback = true;`, and having to set `map.scrolldir` everywhere to
get the credits scrolling in the right direction but make sure the title
screen doesn't start scrolling like a descending tower, too.
After that, the first problem is that it looks like the ACTION press to
speed up the credits scrolling doesn't speed up the background, too. No
problem, just shove a `!game.press_action` check in
`gamecompletelogic()`.
However, this introduces a mini-problem, which is that NOW when you hold
down ACTION, the background appears to be slowly getting out of sync
with the credits text by a one-pixel-per-second difference. This is
actually due to the fact that, as a result of me adding the conditional,
`map.bscroll` is no longer always unconditionally getting set to 1,
while `game.creditposition` IS always unconditionally getting
decremented by 1. And when you hold down ACTION, `game.creditposition`
gets decremented by 6.
Thus, I need to set `map.bscroll` when holding down ACTION to be 7,
which is 6 plus 1.
Then we have another problem, which is that the incoming textures desync
when you press ACTION, and when you release ACTION. They desync by
precisely 6 pixels, which should be a familiar number. I (eventually)
tracked this down to `map.bypos` being updated at the same time
`map.bscroll` is, even though `map.bypos` should be updated a frame
later AFTER updating `map.bscroll`.
So I had to change the `map.bypos` update in `gamecompleteinput()` and
`gamecompletelogic()` to be `map.bypos += map.bscroll;` and then place
it before any `map.bscroll` update, thus ensuring that `map.bscroll`
updates exactly one frame before `map.ypos` does. I had to move the
`map.bypos += map.bscroll;` to be in `gamecompleteinput()`, because
`gamecompleteinput()` comes first before `gamecompletelogic()` in the
`main.cpp` game loop, otherwise the `map.bypos` update won't be delayed
by one frame for when you press ACTION to make it go faster, and thus
cause a desync when you press ACTION.
Oh and then after that, I had to make the descending tower background
draw a THIRD row of incoming tiles, otherwise you could see some black
flickering at the bottom of the screen when you held down ACTION.
All of this took me way too long to figure out, but now the credits
scroll works perfectly while being more optimized.
I had forgotten that the game flashed and did screen-shaking when you
pressed ACTION to quicksave.
While testing for my over-30-FPS patch I stumbled across this.
It's less code being copied and pasted, especially since for my
over-30-FPS patch I would have to make a separate function for each if
both of them were still there, but if they're unified into one then I
will only have to make one more function.
And since map.scrolldir is now used outside of GAMEMODE, we'll need to
reset it in hardreset() and when exiting playtesting.
Due to the previous commit, the descending tower background now has to
account for map.bscroll, or else it will be off by one pixel from the
incoming textures. But ascending tower backgrounds work fine, so no need
to do anything with those.
Looks like this was done as a quick fix instead of taking the time to
figure out the math needed to actually draw the incoming textures, which
is fair enough - it only makes one room, Panic Room, slightly laggier.
While I was working on my over-30-FPS patch, though, I came across the
fact that this background kept getting entirely redrawn every frame, and
it seems like it would be easier to interpolate descending tower
backgrounds if we scrolled what was already there instead.
Here, we have to draw two rows of incoming textures, otherwise the
scrolling surface will produce black lines.
Previously, if you had backgrounds disabled in accessibility options,
and went to the editor and opened up the editor menu, it would be drawn
straight on top of what was already there in the editor instead of being
drawn on top of black. So now it's drawn on top of black.
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.
During testing, I made a cursed level that set the flash timer to
precisely 1,000,000 frames. It turns out that if I activated the timer
in playtesting, exited playtesting, and exited the editor without ever
re-entering playtesting, the timer still kept going. So to prevent being
able to do that, we should hardreset() when exiting the editor.
In-game because that's where screen effects are used the most. But on
the title screen, screen effects are used when you press ACTION to start
the game, and when you enable screen effects, too.
Otherwise, we don't need screen effects for any other game-gamestate.
This de-duplicates the screen effects rendering code by putting it
inside a function, Graphics::renderwithscreeneffects(), and using that
instead of copy-pasted code.
The code to decrement the timers for flashing and shaking is now handled
outside the game-gamestate case-switch, instead of having to be
duplicated inside each render function.
As a bonus, I made it so the timer decrements even if screen effects are
disabled. This is to prevent any theoretical situation where the timer
can "pile up" due to disabled screen effects not letting it tick down.
This removes a lot of duplicate code, which towerrender() mostly
consisted of, even though the only difference is that it draws a
separate map and screen edge spikes are drawn.
This removes lots of duplicated code that drawtowerentities() did,
because all that really changed was accounting for map.ypos (which can
be done conditionally) and where and when the room wrapped (which can
also be done conditionally).
This fixes an oddity that's only visual, which could only happen in
custom levels by using the createentity() internal command.
For the same reason that the second through fourth tiles of moving
platforms on the top and left was buggily rendered, SDL_BlitSurface()
strikes again to mutate the SDL_Rect we pass it and render the next
SDL_BlitSurface() call inbounds, even though we don't need it to.
Previously, the game could end up rendering a warping sprite twice due
to the fact that it could run "if entity is on the right side of the
screen" right after "if entity is on the left side of the screen" (but
not the other way around). This is most noticeable if you have a custom
player sprite with translucent pixels and stand on the left side of a
warping screen, but the code suggests it happens when warping through
the top of the screen, too.
Instead of doing
if (!obj.entities[i].invis)
{
...
}
It's better to do
if (obj.entities[i].invis)
{
continue;
}
...
It reduces the indentation by one level, which is always a good thing.
In the last commit, I removed having the flip mode conditional directly
inside the sprite-drawing code for each size type, which would reduce
the indentation one level. However, I opted to hold off un-indenting
until this commit, otherwise it would've produced too much noise.
The game uses flipsprites.png instead of sprites.png when in flip mode,
mostly to add exceptions for sprites that SHOULDN'T be flipped in flip
mode.
Looks like to achieve this, the routines for sprite drawing got
copy-and-pasted every single time flipsprites.png needed to be
conditionally used, resulting in large amounts of copy-pasted code. And
this copy-pasted code resulted in copy-paste errors, with relating to
VVVVVV-Man, because apparently due to two copy-pasting errors, the
combined giant crewmate in the epilogue uses flipsprites.png even if you
aren't in flip mode, and it also uses the width instead of the height of
sprites_rect when in flip mode (although, this doesn't end up mattering,
but still).
The solution here is to simply change the referenced sprites vector to a
pointer that can conditionally change based on the game being in flip
mode or not.
Looks like I forgot to test that my music silencing patch didn't break
the music being silent during the "You have found a shiny trinket" and
"You have found a shiny crewmate" text boxes. So I've added a check for
game.completestop in the music handling in main.cpp.
Found this bug while I was testing my towerlogic/gamelogic merge patch.
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 fixes being able to rack up a large amount of stack frames by
pressing Esc repeatedly in the editor, which would be a problem if you
were to then return to the main menu afterwards.
Instead, if Menu::ed_settings is already in the stack, the game will
simply return to that menu instead of creating it. Else, it will just
create the menu.
Also, as extra attention to detail, I made sure that the menu create or
return only happens if Esc opens the settings menu, and not when Esc is
closes it.
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.
This commit fixes a bug where the second, third, and fourth tiles of
moving platforms would render offset from the first tile if the moving
platform hit the top or left edge of the screen.
This is due to the fact that SDL_BlitSurface() will end up changing the
coordinates of the rectangle we pass to it to be 0 if they're negative,
but only after it's already been drawn. Previously, we kept re-using the
same rectangle each time we drew each segment of the moving platform,
but since it only changes the draw rectangle after it's already been
drawn, the first tile shows up fine, but not the rest of the tiles,
hence resulting in an offset.
To fix this, we do the same thing as we did for drawing the "really big
sprite" (size-type 9): just reset the rectangle we use every time we
draw a segment of the moving platform.
They're literally the exact same thing, except one is 4 and the other
has an 8. No need to have all that copy-pasted code.
Actually, the only other difference was that size-type 8 set the
drawRect to sprites_rect instead of tiles_rect for some reason? Even
though it doesn't matter, anyway, because SDL_BlitSurface() only cares
about the X and Y you pass it, not the width and height, which is the
only difference between tiles_rect and sprites_rect.
To make sure that people wouldn't wonder where size-type 8 went if they
saw a blank space between size-type 7 and size-type 9, I kept the
size-type 8 conditional, but inside it is just a comment telling you to
go to size-type 2.
Instead of copy-pasting all the BlitSurfaceStandard()s all over again,
just make the referenced vector a pointer that changes depending on
map.custommode.
There's a noticeable seam in the horizontal and warp backgrounds
whenever you enter a new room. Entering a new room triggers the game to
re-draw the entire warp background instead of simply scrolling what it
already has. This seam is the result of the initial background draw
being misaligned with the rest of the scrolling.
If you get out your measuring tools, you'll see that it's misaligned by
exactly 3 pixels (this applies to both horizontal and vertical warping).
If you look at the part of the code where the game draws fresh warping
textures after scrolling the existing ones offscreen, you'll see it
starts with an offset of 317, which is exactly 320 minus 3. And for
vertical, it uses 237, which is exactly 240 minus 3.
This is where the misalignment comes from. Since the incoming textures
are drawn 3 pixels to the left, but the initial draw isn't, this results
in a misalignment and causes the seam.
To fix this, draw the initial draw of the horizontal and vertical warp
backgrounds 3 pixels to the left.
It looks like one bracket got out-of-place for whatever reason. This
doesn't affect the case-switch at all, due to how case-switches work,
but it's still weird to look at.
Indentation has been updated accordingly.
Some custom levels have their own custom music and sync that music to
scripted cutscenes, which is actually pretty impressive. However,
they've always run into a small thorn, which is that you can easily
desync the music by unfocusing the game, because the audio will keep
playing when the game is unfocused.
This should remove that thorn by pausing the audio on unfocus, and
resuming when focused, so that the music can no longer desync, but you
can still pause the game by unfocusing it.
This is yet another feature in VCE that hasn't been upstreamed until
now.
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 environment variable SteamTenfoot corresponds with the game running
in Steam Big Picture mode or SteamOS if it is defined. There's a
certification process for both full controller support and Big Picture
mode, and being able to launch a file window in Big Picture mode is an
instant cert failure.
Have to add some includes and put these behind some ifdefs, of course.
I'm pretty sure FreeBSD and OpenBSD and Haiku are POSIX enough that the
"open" command will work on them, too.
I would've loved to make FILESYSTEM_openDirectoryEnabled a simple bool
instead of a function, but I ran into issues with putting it in the
FileSystemUtils header file, so I'll just make it a function and call it
a day.
This fixes a bug where levels in the levels list duplicate if there's an
invalid file (such as a folder) in the levels directory.
It looks like it happens because we don't free the memory if
PHYSFS_readBytes() encounters an error, even though we should. Then we
get into Undefined Behavior territory and end up reusing memory, and
here it just happens that previously, parsing the entire XML document
for each level file was enough to make the loaded file pointer point to
garbage that would fail the metadata check, but if we optimize it so we
don't parse the entire XML document, it starts reusing memory instead.
To find each individual tag quickly, to optimize levels list loading.
I opted to not read the tags <Created>, <Modified>, and <Modifiers> as
they're actually pretty useless.
Also I've added a tag finder for <MetaData> but it's not meant to be
used directly, it's only used to check that the tag exists.
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.
This is to not reset your cursor position every time you return on
something. It's also to automatically keep track of which menu was the
previous menu instead of manually hardcoding said previous menu.
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.
This marks pressing ACTION on "next page" in the levels list, credits,
pressing ACTION on "continue" in "You have unlocked" menus, and pressing
ACTION on an unlock option in the unlock menu and time trial unlock menu
as being the same menu.
This is to prevent creating unnecessary stack frames when using said
menu options in those menus.
These aren't necessary, the menu will update regardless. There isn't
even such a call for the mouse cursor toggle option, that's how
unnecessary it is.
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.
I assume it was so a dev could mark the spot where they needed to put in
the analogue toggle, and they found a unique yet easy to remember
sequence of characters to Ctrl+F as a marker.
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.
I think it's about time that this number be updated, yeah? This isn't to
say that 2.3 is finished or almost finished or anything, this is just to
clearly differentiate that this isn't 2.2.
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.
The game won't let you select the Secret Lab if you're in invincibility
mode, probably so you can't set illegitimate Super Gravitron records
just by standing there and doing nothing.
However, for some reason, it'll still let you select the Secret Lab even
if you've slowed down the game. For consistency, let's prevent selecting
the Secret Lab if the game isn't running at fullspeed, too.
I've converted every "else if"-chain in menu render/input code to be a
case-switch, except for the levels list, the "game options" menu
(because it has the MMMMMM menu option which isn't a compile-time
constant), and the "play" menu (because it has the Secret Lab menu
option which also isn't a compile-time option).
I also did NOT convert some case-switches relating to unlocks in
Input.cpp, mostly because they use a system where the "if we have this
unlocked" conditional is a part of the "if this is the current menu
option" conditional, and they use the 'else' branch to play a sad sound
if that "if we have this unlocked" conditional fails.
I've also converted the game.gameframerate and game.crewrescued() "else
if"-chains to be case-switches instead.
There was one level that was indented with 2 spaces instead of 4, which
made everything else look weird. Then some lines were randomly indented
further for no reason.
Doing this before the next commit is done so as to not make the next
commit noisier.
This removes duplicate code that came about as a result of various
possible permutations of menu options, depending on being M&P, having no
custom level support, having no editor support, and having MMMMMM.
The menus with such permutations are the following:
- main menu
- "start game" is gone in MAKEANDPLAY
- "player levels" is gone in NO_CUSTOM_LEVELS
- "view credits" is gone in MAKEANDPLAY
- "game options"
- "unlock play data" is gone in MAKEANDPLAY
- "soundtrack" is gone if you don't have an mmmmmm.vvv file
- "player levels"
- "level editor" is gone in NO_EDITOR
I achieve this de-duplication by clever use of calculating offsets,
which I feel is the best way to de-duplicate the code with the least
amount of work, if a little brittle.
The other options are to (1) put function pointers on each MenuOption
object, which is pretty verbose and would inflate Game::createmenu() by
a lot, (2) switch all game.currentmenuoption checks to instead check for
the text of the currently-selected menu option, which is very
error-prone because if you make a typo it won't be caught at
compile-time, (3) add a unique ID to each MenuOption object that
represents a text but will error at compile-time if you make a typo,
however this just duplicates all the menu option text, which is more
code than was duplicated previously.
So I just went with this one.
I don't know why you would have to have both defined simultaneously, but
now you can.
The compile fail was caused by the fact that if both were defined, then
there would be an expression like this in Map.cpp:
switch (t)
{
case 0:
}
which is an invalid expression.
The solution is to put 'case 0:' into the MAKEANDPLAY ifdef-guard as
well.
Just like I moved the menu ACTION press handler, I'm doing this as well.
It only removes one level of indentation, but it makes titlerender()
easier to understand.
Just like before, I have to put the separate function first, else
titlerender() won't know what it is.
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.
This takes out 3 indentation levels from the ACTION press handling,
making titleinput() easier to read as a whole.
Unfortunately, we have to put menuactionpress() first, even though I'd
want it the other way around, otherwise titleinput() won't know what it
is.
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().
This removes map.numshinytrinkets in favor of using
map.shinytrinkets.size(). Having automatic length tracking is much less
error-prone and less tedious.
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.
Previously, it existed solely to count the number of trinkets and
crewmates when loading a level, because we were keeping track of the
amount of them manually, incrementing and decrementing every time a
trinket or crewmate was added or removed, but loading a new level
represented a case that could potentially not be an increment or
decrement.
However, since the amount tracking is now handled automatically, this
function now does nothing, and can be safely removed.
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.
It's a bit rude to put the user back at the main menu after toggling
something. Maybe they also wanted to do something else in the menu while
they're toggling MMMMMM, there's no reason to immediately put them back
there.
Whenever you collect a trinket, game.stat_trinkets gets updated (if
applicable) to signify the greatest amount of trinkets you have ever
collected in the game. However, custom levels shouldn't be able to
affect this, as their trinkets are not the same trinkets as the main
game.
It turns out that when the game warps moving platforms, it won't remove
the block from the position before they warped. Eventually, these blocks
will pile up and will never be removed, causing a memory leak.
I noticed that the code for going to the adjacent room when offscreen
and for warping instead if the room is warping was a bit
copy-and-pasted. To clean up the code a bit, there's now 5 separate
checks in gamelogic():
if (map.warpx)
if (map.warpy)
if (map.warpy && !map.warpx)
if (!map.warpy)
if (!map.warpx)
I made sure to preserve the previous weird horizontal warping behavior
that happens with vertical warping (thus the middle one), and to
preserve the frame ordering just in case there's something dependent on
the frame ordering.
The frame ordering is that first it will warp you horizontally, if
applicable, then it will warp you vertically, if applicable. Then if you
have vertical warping only, that weird horizontal warp. Then it will
screen transition you vertically, if applicable. Then it will screen
transition you horizontally, if applicable.
To explain the weird horizontal warp with the vertical warp: apparently
if an entity is far offscreen enough, and if that entity is not the
player, it will be warped horizontally during a vertical warp. The
points at which it will warp is 30 pixels farther out than normal
horizontal warping.
I think someone ran into this before, but my memory is fuzzy. The best I
can recall is that they were probably createentity()ing a high-speed
horizontally-moving enemy in a vertically warping room, only to discover
that said enemy kept warping horizontally.
As a result of the previous commit, textboxclass::clear() is now unused.
textboxclass::firstcreate() was already useless. So remove both those
functions and initialize the values in the textboxclass constructor.
This removes the variables graphics.ntextbox, as well as removing
'active' from each text box object. Thus, all text boxes are really
real, and you don't have to check its 'active' variable.
Something that's slightly annoying is that in order to make the vector
of text boxes be properly used, the text box cannot remove itself.
Because the text box does not know it's in a vector. So move the removal
of the text box to drawgui() instead.
Just like earlier, these are of the form
if (cond1) { if (cond2) { if (cond3) { thing; } } }
and are really annoying to read.
Also this removes the remnants of the 'active' system that have been
replaced with 'if (true)' conditionals in order to not add noise to the
diff.
Previously, it was used in order to clear a block and deactivate it, and
the constructor function simply called clear() in order to not duplicate
code. However, clear() is no longer necessary (just remove the block
from the blocks vector), and so we can put initialization right back in
the constructor function.
It turns out the game engaged in pseudo-UB when removing activity zones,
which got turned into actual UB due to the previous commit.
There were three places where this could happen:
- Pressing ENTER on an activity zone in normal gameplay
- Pressing ENTER on an activity zone in in-editor playtesting (because
the code is duped here)
- Pressing ESC and quitting to menu while standing inside an activity
zone
In all cases, game.activeactivity would still be pointing to a
non-existent activity zone. This activity zone in the previous system
would simply be a block with a false 'active', and in the system where
C++ vectors are used properly, would index past the blocks array.
In fact, it is a bug that when you press ENTER on an activity zone, the
activity zone prompt suddenly turns to black, then immediately
disappears. It was pointing to a block that had its clear() method
called, which is why it was all black, and it was an inactive block!
This commit makes it so pressing ENTER on an activity zone smoothly
fades out the activity zone prompt instead of being sudden black.
This removes the variables obj.nblocks, as well as removing the 'active'
attribute from the block object. Now every block is guaranteed to be
real without having to check the 'active' variable.
Removing a block while iterating now uses the removeblock_iter() macro.
Previously there was an entclass::clear(), and initialization of an
entclass was done by calling clear() in order to not duplicate code. But
now there's no need for an entclass::clear(), and it is in fact unused
(just call entityclass::removeentity() instead), so I'm removing this
function.
I guess these were here earlier when there were 'active' conditionals,
but then I removed those, so now they look weird next to the 'i != j'
conditionals, so I'm removing them.
These would be of the form
if (cond1) { if (cond2) { if (cond3) { thing; } } }
which is really annoying to read and could've been written as
if (cond1 && cond2 && cond3) { thing; }
so that's what I'm fixing here.
There will be another commit later that fixes this but in places related
to blocks.
Not sure why this function is here. It makes sense if you know that the
game will only do certain moving platform things if you already have a
moving platform in the room, however apparently this function has
absolutely nothing to do with it.
This function's sole purpose was to make sure obj.nentity was in sync,
and that obj.nentity-1 pointed to the last 'active' entity in
obj.entities. But now that obj.nentity is removed and we use
obj.entities.size() instead, it is no longer necessary.
In the previous commit, if an if-statement consisted solely of checking
the active attribute of an entity, I temporarily changed it to 'true'
and put a comment to remove it later, because it would add too much
noise to unindent everything in the same commit.
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.
Also when we switch everything to not use 'active', we'll need this
macro to remove entities while iterating forwards through the vector one
at a time.
Ok, once we switch everything over to not use the 'active' system, it's
easier to read removeentity(t) than it is to read
entities.erase(entities.begin() + t).
This moves the setenemyroom() function onto the entity object itself, so
I can more easily change all 'entities[k].' to 'entity.' in
entityclass::createentity() later.
Additionally, I've had to move the rn() macro from Entity.h to Ent.h, or
else entclass::setenemyroom() won't know what it is.
This moves the setenemy() function onto the entity object itself,
instead of having to give the indice of the entity in obj.entities. This
makes the code more object-oriented so later I can simply change all
'entities[k]' to 'entity.' in entityclass::createentity().
It is an exact duplicate of musicclass::haltdasmusik(), so use that
function instead and update callers. Looks like
musicclass::haltdasmusik() came first, anyway (musicclass::stopmusic()
was only used in editor.cpp).
Looks like musicfade is an unused variable, anyway. I might remove it,
but I have some plans in the future that involve repairing what it was
intended for, so I'll hold off on removing it (and some other unused
variables in Music.cpp) for now.
As discussed earlier, some custom levels have taken advantage of the
fact that songs 0 and 7 loop and also fade in when using PPPPPP while
having an mmmmmm.vvv file present.
The problem is that it would index out-of-bounds if you did this, but
this UB hasn't caused an exception until my change to refactor
script-related vectors by removing their separate length-trackers.
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.
Just a miscellaneous code cleanup.
There's no glitches that take advantage of the previous situation,
namely that 'temp' was a global variable in Logic.cpp and editor.cpp.
Even if there were, it seems like it would easily lead to some undefined
behavior. So it's good to clean this up.
This is the "Behavioral logic", "Basic Physics", and "Collisions with
walls" trio.
They were originally aligned but then I removed global args, thus
misaligning them. So now I'm re-aligning them back again.
Surprisingly not that many. It looks like at one point in development,
damage blocks were created for every single spike in the Tower, before
it was too laggy so they switched to directly checking collision with
the tile instead.
This removes a bunch of commented-out code that was clearly kept from
the Flash version, even though the Flash graphics API is much different
than SDL's. Also removes a bunch of TODOs that either say nothing, or
say something whose meaning has been totally lost to time due to being
completely vague, or something that's already been done and someone
forgot to remove the TODO.
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.
To be fair, it was more on the level of entire functions using different
indentation than the surrounding code, but it's not consistent enough
for me to justify leaving it alone.
Looks like this function was created because editorclass::load() takes
in a string by reference, not by value, and thus mutates it afterwards,
so if you passed a string in when you didn't want it to be mutated, bad
things would happen.
However, a better workaround for the above issue would simply to
duplicate the string and pass that string instead, thus the original
string wouldn't be affected.
To reiterate, I just want to remove the mixed indentation that just
randomly pops up in the middle of a file, because it gets annoying.
Thus, the indentation of a particular piece of code should simply match
the surrounding code. And I consider it completely fine that this file
switches from indenting 4-wide spaces to tabs starting from
Graphics::setcol() onwards. I don't think it's worth it to untabify
Graphics::setcol() and below.
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.
This changes something like UtilityClass::String to help.String,
basically. It takes less typing this way, and is a neat effect of having
global args actually be global variables.
Now that it does nothing due to some earlier changes, it's a useless
function that does nothing. (Well, it was already a useless function,
but those earlier changes made it clearer just exactly how useless it
is.) So remove that function and remove all its callsites.
'swfStage' gets set to 'stage' in updategraphicsmode() but... that does
absolutely nothing, because they both contain exactly the same thing.
And these variables aren't referenced anywhere else. So I'm removing
both of these variables.
Although it keeps getting set to true and false in various places, it
never once gets checked, essentially deeming it a variable that's used
but does nothing.
The 'r', 'g', and 'b' arguments do absolutely nothing. Except unlike
Graphics::drawtile(), there's only one version of Graphics::drawtile2(),
so just remove those args and update callers.
The 'r', 'g', and 'b' arguments do absolutely nothing, even though
they're used in the version of Graphics::drawtile() that's more used. So
delete the other version without those extra arguments, and then remove
the extra arguments from the remaining version. And then update callers.
