There were many different ways I could've fixed it, but one thing that
stood out to me was the fact that touching the teleporter wasn't
guaranteed to set its onentity to 0, even though it should be. So now,
every time Viridian touches the teleporter, the teleporter's onentity
will be set to 0, and thus there's no chance of the teleporter
interrupting its own teleport animation and softlocking the game.
We should still do what I suggested in #391, namely setting
game.hascontrol to true if the game is in gamestate 0 and script.running
is false, and also always allowing Esc/Enter to be pressed regardless of
game.hascontrol. But this softlock is fixed now.
Fixes#391.
Whoops. Forgot to do this earlier when adding the Shift+F3 hotkey.
Otherwise the enemy type would become invalid and just turn into the
default square.
The game would softlock if you brought up the map screen or quit screen
after exiting the Super Gravitron to the Secret Lab. This softlock would
only happen if you were in glitchrunner mode.
This is because glitchrunner mode set game.fadetolabdelay when it
shouldn't have, and also checked game.fadetolabdelay when it shouldn't
have.
So I made it so that the game will only set game.fadetolabdelay when not
in glitchrunner mode (I already had a check for game.fadetomenudelay,
too!) and the game will only check for game.fadetomenudelay and
game.fadetolabdelay when not in glitchrunner mode, as well.
I originally made the game check game.fadetomenudelay and
game.fadetolabdelay to prevent being able to re-press ACTION to re-start
the fadeout if the game was already fading out. And I made sure that
this wasn't broken, both in glitchrunner mode and normal mode.
Whoops.
Noticed this earlier when I was merging upstream back into VCE, and
interestingly enough, it doesn't look like cppcheck warns about
undeffing a non-existent define.
I don't know who wrote this code originally, but it's extremely obvious
that it was a different person than who wrote the rest of the code.
Anyway, I fixed the spacing and braces so everything is smushed together
less.
Also, there's no need to put the entire warpdir checking in an
'if(room.warpdir>0)' statement. If any of the cases is jumped to, then
you already know that's true.
"Threadmills" are now properly called "conveyors". I don't know why they
were called "threadmills" anyway, the proper spelling is "treadmills".
Also, warp line `p1`s of 0 and 3 are now properly labeled, as well as
the trinket edentity.
Just set the map roomname to the roomname of the room. It's completely
redundant to set the roomname to an empty string and check if the
roomname of the room is empty.
I do this because we declare-and-initialize some variables in the case.
This isn't strictly necessary, since there's no cross-initialization
errors since it's the last case in the switch, but I'd just like to be
future-proof.
Instead of repeating 'ed.level[curlevel]' (or even worse, you type in
that giant expression inside the brackets instead of reusing
'curlevel'), why not just type 'room'? As an added bonus, I added bounds
checks so 'room' is always guaranteed to point to an existing object.
There are some levels that index 'ed.level' out of bounds, and I'd like
to make their behavior properly defined instead of being undefined. One
of the things usually true about out-of-bounds rooms is that they're
always tiles2.png (that means an edlevelclass tileset that isn't 0),
because the chance of the memory location being exactly 0 is smaller
than it being nonzero. So the out-of-bounds room has tileset 1.
Again, it used this severely overcomplicated expression for god knows
what reason. I've replaced it with a simpler one. Also it's const just
to indicate intent.
Previously it copy-pasted this god-awful overcomplicated expression
(possibly for cursed ActionScript legacy reasons?) everywhere, instead
of putting it in a variable, or even just using a simpler one like the
one I replaced it with.
Now the obj.createentity() and obj.createblock() calls are much nicer.
Now if your cursor was at the bottom of the screen when you entered
playtesting but then was moved to the top during playtesting, when you
exit, the roomname won't instantly disappear and then sheepishly rise
back up again.
And if your cursor was at the bottom of the screen when you entered
playtesting, and exited still being that way, the roomname will rise
back down again and won't instantly disappear.
Both of these behaviors make the roomname movement much more continuous
than it was previously, and it feels smoother and less janky.
Also, use inspecial() instead of writing out each part of the
conditional separately. This just basically adds the insecretlab
conditional to the if-statement, which shouldn't be a big deal.
There's no reason you shouldn't be allowed to press Enter on teleporters
during playtesting.
Well, except that you can press Esc in the teleporter menu in order to
go to the pause menu, which isn't intended in playtesting. But I can
just add a check there so that pressing Esc closes the teleporter menu
instead, it's fine.
graphics.textbox.clear() should be used instead of
graphics.textboxremove() or graphics.textboxremovefast(), because even
with graphics.textboxremovefast(), you'll still have to process at least
one frame of GAMEMODE logic before the text boxes are actually properly
removed, and this caused a 1-frame glitch when exiting playtesting with
text boxes on-screen and then re-entering playtesting.
Technically I could've only fixed it in Game::returntoeditor(), but I
wanted to be safe, so I also fixed it in scriptclass::hardreset(), too.
This fixes a bug where the settings menu would immediately be brought up
if you used Esc to exit playtesting, unless you were an incredible ninja
and only pressed it for exactly one frame.
This fixes an annoying bug where if you use Up or Down to press ACTION
on the "All crewmates rescued!" dialogue whenever you rescue the last
crewmate during playtesting, it'll move you to the room above or below
you. This is because ed.keydelay isn't set to 6 (which is the standard
value that it gets set to whenever you press most keys in the editor),
but now it is.
The shouldreturntoeditor variable is supposed to be used because it
fixes the warp background not being reset if you exit into a
horizontally/vertically warping room with a different background. It
also properly resets other variables, which is good, too.
Instead of using gamestates, just directly use the 'script' attribute of
a script box if it is non-empty.
This is accomplished by having to return the index of the block that the
player collides with, so callers can inspect the 'script' attribute of
the block themselves, and do their logic accordingly.
game.customscript is an unnecessary middleman, but it will be kept
around for compatibility reasons. However, it's still possible to crash
the game, so I'm adding this bounds check.
To avoid going through gamestates, we'll need to carry the name of the
script on the script box itself. And to do that, we'll need to set the
'script' attribute of script boxes when translating edentities into real
entities in custom levels.
This patch optimizes the loop used to limit the framerate in 30-FPS-only
mode so that it uses SDL_Delay() instead of an accumulator. This means
that the game will take up less CPU power in 30-FPS-only mode. This also
means that the game loop code has been simplified, so there's only two
while-loops, and only two places where game.over30mode is checked, thus
leading to easier-to-understand logic.
Using an accumulator here would essentially mean busywaiting until the
34 millisecond timer was up. (The following is just what leo60228 told
me.) Busywaiting is bad because it's inefficient. The operating system
assumes that if you're busywaiting, you're performing a complex
calculation and handles your process accordingly. And this is why
sleeping was invented, so you could busywait without taking up
unnecessary CPU time.
When I moved duplicate player entity removal to
scriptclass::hardreset(), I also inadvertently made it so all non-player
entities got removed as well, even though this wasn't my intent. And
thus, pressing Enter to restart a time trial removes every entity except
the player, since it calls script.hardreset().
The time trial script.hardreset() is bad for other reasons (see #367),
however it's still a good idea to reset only what's needed in
script.hardreset().
There's a bug where playtesting from Ved doesn't properly play the music
of the level, due to no fault with Ved.
This was because the music was being faded out by
scriptclass::startgamemode() case 23 after main() called music.play().
To fix this, just call music.play() when all the other variables are
being set in Game::customloadquick().
So you get a trophy and achievement for completing the game in Flip
Mode. Which begs the question, how does the game know that you've played
through the game in Flip Mode the entire way, and haven't switched it
off at any point? It looks like if you play normally all the way up
until the checkpoint in V, and then turn on Flip Mode, the game won't
give you the trophy. What gives?
Well, actually, what happens is that every time you press Enter on a
teleporter, the game will set flag 73 to true if you're NOT in Flip
Mode. Then when Game Complete runs, the game will check if flag 73 is
off, and then give you the achievement and trophy accordingly.
However, what this means is that you could just save your game before
pressing Enter on a teleporter, then quit and go into options, turn on
Flip Mode, use the teleporter, then save your game (it's automatically
saved since you just used a teleporter), quit and go into options, and
turn it off. Then you'd get the Flip Mode trophy even though you haven't
actually played the entire game in Flip Mode.
Furthermore, in 2.3 you can bring up the pause menu to toggle Flip Mode,
so you don't even have to quit to circumvent this detection.
To fix both of these exploits, I moved the turning on of flag 73 to
starting a new game, loading a quicksave, and loading a telesave (cases
0, 1, and 2 respectively in scriptclass::startgamemode()). I also added
a Flip Mode check to the routine that runs whenever you exit an options
menu back to the pause menu, so you can't circumvent the detection that
way, either.
The music for the Tower is supposed to be ecroF evitisoP in Flip Mode,
and Positive Force when not in Flip Mode. However, if you go to the
options from the pause menu and toggle Flip Mode, the music isn't
changed.
Fixing this is pretty simple, just check the current area if not in a
custom level and play the correct track accordingly when toggling Flip
Mode from in-game.
Gamestates 300..336 are used to start scripts in custom levels. However,
it looks like instead of having the cases have common code, each
individual case was copy-pasted numerous times, which is pretty
wasteful.
std::string is one of those special types that has a constructor that
just initializes itself to a blank state automatically. This means all
`std::string`s are by default already `""`, so there's no need to set
them. And in fact, cppcheck throws out warnings about performance due to
initializing `std::string`s this way.
I ran the game through cppcheck and it spat out a bunch of member
attributes that weren't being initialized. So I initialized them.
In the previous version of this commit, I added constructors to
GraphicsResources, otherlevelclass, labclass, warpclass, and finalclass,
but flibit says this changes the code flow enough that it's risky to
merge before 2.4, so I got rid of those constructors, too.
Looks like coins were basically a scrapped mechanic, although I'm not
sure what these attributes were for. I guess counting the number of
coins in each room? But why, when you can just make a function to count
them automatically? Whatever.
This is just in case these values happen to be used without being
initialized or anything. I vaguely recall someone reporting an issue
where they didn't have a "Documents" folder on Windows and their level
folder ended up being a garbage path, so it's good to do this.
There's a bug in the cutscene that plays if your companion is Vitellary
in the room "Now Stay Close To Me...". The relevant gamestate is
gamestate 43, which for Vitellary calls the script `int1yellow_4`.
When Vitellary says the text box "That big... C thing! I wonder what it
does?", Terry intended for Vitellary to change his facing direction to
the left, as you can see with the command `changedir(yellow,0)` in the
original scripting. `changedir()` just changes the `dir` attribute of an
entity, and a `dir` of 0 means face left and a `dir` of 1 means face
right.
Then when Vitellary says "Maybe we should take it back to the ship to
study it?", Terry intended for him to face rightwards once again, as
indicated by the `changedir(yellow,1)` command.
Unfortunately, what happens instead is that when Vitellary says the
first text box ("That big... C thing! I wonder what it does?"), he turns
left for precisely one frame, and then afterwards goes back to facing
right. Then the second text box comes around, but he's already facing
right. How come?
Well, the problem here is that Vitellary's AI for "follow Viridian" is
overriding his `dir` attribute. Vitellary's AI says "get close to
Viridian", but Vitellary is already close enough to them that he stays
put. However, he still turns to face them as part of that AI.
To fix that, we need to put him in the AI mode that specifically says to
face left, with the command `changeai(yellow,faceleft)`. That way, he no
longer has the AI mode of following Viridian, and he will actually look
left for the intended duration instead of only looking left for one
frame.
But then we have another problem. When the cutscene ends, Vitellary no
longer follows Viridian. I mean it makes sense - we just placed him in
"only face left" mode, not "follow Viridian" mode! And this is not
merely a visual problem, because Vitellary is a supercrewmate and the
game won't let the player walk off the screen if Vitellary isn't
offscreen yet.
To fix THAT issue, we'll need to put Vitellary back in "follow Viridian"
mode. It turns out that the `changeai()` command was more intended for
scripting crewmates (entity type 12), NOT supercrewmates (entity type
14). As such, the command assumes that you'll want state numbers that
apply to entity type 12, such as 10, 11, 12, 13, and 14, even though the
only one that applies to entity type 14 is state 0, and every other
state number just makes it so that the entity doesn't move an inch. And
specifying faceleft/faceright is just state number 17.
Luckily, we can still pass the raw state number to `changeai()`, we
don't have to use its intended names. So I do a `changeai(yellow,0)` to
set Vitellary's state number back to 0 when it comes time to make him
face right again.
As a bonus, I added comments to the changed lines. This is a semi-obtuse
method of scripting, so it's always good to clarify.
The spikes are removed if the game is in no death or time trial mode,
however the removal is accomplished by modifying a static array. So if
the player switches to no death or time trial mode and switches back to
regular play, the spikes will no longer be present until the game is
restarted.
This simple fix writes the spikes back to the static array if the game
is not in no death or time trial mode. Another option is to maintain 2
static arrays - one with the spikes and one without - but that is
needlessly wasteful and prone to mistakes (one array updated but not the
other).
For some reason, there were just exact duplicates of the talkgreen_2
script and alarmon/alarmoff commands. I have no idea why, but cppcheck
identified them.
Graphics::drawmenu() no longer has copy-pasted code for each individual
case. Instead, the individual cases have their own adding on to common
code, which is far easier to maintain.
Also, the only difference Graphics::drawlevelmenu() does is in some
y-positioning stuff. There's no reason to make it a whole separate
function and duplicate everything AGAIN. So it's been consolidated into
Graphics::drawmenu() as well, and I've added a boolean to draw a menu
this way if it's the level menu.
Instead, the string in MenuOption is just a buffer of 161 chars, which
is 40 chars (160 bytes if each were the largest possible UTF-8 character
size) plus a null terminator. This is because the maximum length of a
menu option that can be fit on the screen without going past is 40
chars.
There's no need to use a template here. Just manually call SDL_tolower()
or SDL_toupper() as needed.
Oh yeah, and use SDL_tolower() and SDL_toupper() instead of libc
tolower() and toupper().
I don't know how no one realized that the for-loop to (poorly)
initialize m_headers was basically unnecessary, and that the memset()
should've just been used instead. Well, except it should also be
replaced with SDL_memset(), but that's besides the point.
Also, I decided to hardcode the 128 thing less, in case people want to
fork the source code and make a build where it's changed.
So, originally, I wanted to keep them on Game, but it turns out that if
I initialize it in Game.cpp, the compiler will complain that other files
won't know what's actually inside the array. To do that, I'd have to
initialize it in Game.h. But I don't want to initialize it in Game.h
because that'd mean recompiling a lot of unnecessary files whenever
someone gets added to the credits.
So, I moved all the patrons, superpatrons, and GitHub contributors to a
new file, Credits.h, which only contains the list (and the credits max
position calculation). That way, whenever someone gets added, only the
minimal amount of files need to be recompiled.
They're always the same size, so there's no need for them to be vectors.
Also made the number of elements in ed.level/kludgewarpdir controllable
by maxwidth/maxheight.
I removed editorclass::saveconvertor() because I didn't want to convert
it to treat ed.contents like an array, because it's unused so I'd have
no way of testing it, plus it's also unused so it doesn't matter. Might
as well get rid of it.
These unused vars are:
- Graphics::bfontmask_rect
- Graphics::backgrounds
- Graphics::bfontmask
- GraphicsResources::im_bfontmask
While it seems that Graphics::backgrounds was indexed in
Graphics::drawbackground(), in reality there was never anything in that
vector and thus actually using it would cause a segfault.
map.contents always has 1200 tiles in it, there's no reason it should be
a vector.
This is a big commit because it requires changing all the level classes
to return a pointer to an array instead of returning a vector. Which
took a while for me to figure out, but eventually I did it. I tested to
make sure and there's no problems.
They're always fixed-size anyways, there's no need for them to be
vectors.
Also used the new INBOUNDS_ARR() macro for the map.explored bounds
checks in Script.cpp, and made map.explored a proper bool array instead
of an int array.
Again, basically no reason for it to exist on the class itself.
The usage of the variable was replaced with temp2 instead of temp
because there was already a temp variable in the function it was used
in.
Since they're always fixed-size, they don't need to be dynamically-sized
vectors.
entityclass::customcrewmoods is now a proper bool instead of an int now,
and I replaced the hardcoded constant 6 with a static const int Game
attribute to make it easier to change.
These are the besttimes, besttrinkets, bestlives, and bestrank
attributes of Game. bestframes was already a plain array.
As these are always fixed-sized, there's no reason for them to be
vectors. Also, I put their size in a static const int so it's easy to
change how many of them there are.
They're always fixed-size, so there's no need to them to be a dynamic
vector.
I changed their type to `bool` too because they don't need to be `int`s.
Also, I replaced the hardcoded 25 constant with at least a name, in case
people want to change it in the future.
It could index the `words` array out-of-bounds if there were more than
40 arguments in a command. Not like that would ever happen, but it's
still good to be sure.
In summary, if you got to gamestate 1002 or 1012 without an advancetext,
and you had completestop on, you were basically softlocked. So just add
those gamestates there and advance the gamestate if advancetext is off.
This infinite loop would occur because once you pressed left or right,
the game keeps searching through all the list of teleporters until it
finds one that is unlocked. But if there's none that are unlocked, then
the game goes into an infinite loop, which brings up the Not Responding
dialog on Windows so you can kill it.
Normally, you're not supposed to have no teleporters unlocked while
being able to access a teleporter, but you can achieve this by going to
Class Dismissed from a custom level (while making sure you don't start
in 0,0, because there's a teleporter there that you would unlock).
The solution is to make sure at least one teleporter is unlocked before
doing any searching.
