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 enforces the C++03 standard for people making pull requests who may
not realize their fancy features are too new and shouldn't be used
(cough, cough, @leo60228).
I did some internet searching and this is what I got from this page:
https://crascit.com/2015/03/28/enabling-cxx11-in-cmake/
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.
