It's possible that musicReadBlob.getIndex() could return the sentinel
value of -1 in case the header with that name is invalid, in which case
we should simply not do anything. Otherwise it'll lead to segfaults. I
opted to do the full bounds check just to be safe, too.
For further safety, I hardcoded the max number of headers, 128, less, so
128 is copy-pasted less and in the future if it needs to be changed
it'll only have to be changed in one place.
The binary blob shouldn't return an index if it ends up being invalid.
That could cause a whole lot of issues if musicclass ends up parsing the
resulting struct.
With all that said, though, musicclass doesn't check the -1 sentinel
value anyway, even though it should, but that'll be fixed later.
This fixes the bug where in glitchrunner mode, quitting to the menu
would always put you back at the play menu on the first option, instead
of the menu you entered the game from.
The problem is the script.hardreset() that gets called before the game
actually quits to the menu, so when Game::quittomenu() gets called to
quit to the menu, all the variables that keep track of whether you're in
a certain gamemode, such as game.insecretlab and map.custommode, all get
prematurely reset before that function can read them and put you back to
the correct menu.
The solution here is to simply reset only what's needed when quitting to
the menu. Specifically, in order for credits warp to work,
script.running needs to be set to false and all the text boxes need to
be removed. Text boxes need to be gone so the "- Press ACTION to advance
text -" prompt will stay up without a text box, enabling the player to
increment the gamestate at will by pressing ACTION, and the script needs
to stop running so further text boxes don't spawn in.
Fixes#389.
After looking at pull request #446, I got a bit annoyed that we have TWO
variables, key.textentrymode and ed.textentry, that we rolled ourselves
to track the state of something SDL already provides us a function to
easily query: SDL_IsTextInputActive(). We don't need to have either of
these two variables, and we shouldn't.
So that's what I do in this patch. Both variables have been axed in
favor of using this function, and I just made a wrapper out of it, named
key.textentry().
For bonus points, this gets rid of the ugly NO_CUSTOM_LEVELS and
NO_EDITOR ifdef in main.cpp, since text entry is enabled when entering
the script list and disabled when exiting it. This makes the code there
easier to read, too.
Furthermore, apparently key.textentrymode was initialized to *true*
instead of false... for whatever reason. But that's gone now, too.
Now, you'd think there wouldn't be any downside to using
SDL_IsTextInputActive(). After all, it's a function that SDL itself
provides, right?
Wrong. For whatever reason, it seems like text input is active *from the
start of the program*, meaning that what would happen is I would go into
the editor, and find that I can't move around nor place tiles nor
anything else. Then I would press Esc, and then suddenly become able to
do those things I wanted to do before.
I have no idea why the above happens, but all I can do is to just insert
an SDL_StopTextInput() immediately after the SDL_Init() in main.cpp. Of
course, I have to surround it with an SDL_IsTextInputActive() check to
make sure I don't do anything extraneous by stopping input when it's
already stopped.
All that pressing R does in No Death Mode is end your run. As a result,
it'll only be pressed by accident, so it's better to just disable it
instead.
It's not even useful to quick-restart, because it's faster to quit and
go through the menu again than it is to wait through the Game Over
screen.
Additionally, I removed the `game.deathseq<=0` conditional because it's
unnecessary due to the if-statement already being inside a
`game.deathseq == -1` conditional.
strcat()s and strcpy()s have been replaced with SDL_snprintf() where
possible, to clearly convey the intent of just building a string that
looks a certain way, instead of spanning it out over multiple lines.
Where there's not really a good way to avoid strcat()/strcpy() (e.g. in
PLATFORM_getOSDirectory()), they will at least be replaced with
SDL_strlcat() and SDL_strlcpy(), which are safer functions and are less
likely to have issues with null termination.
I decided not to bother with PLATFORM_migrateSaveData(), because it's
going to be axed in 2.4 anyways.
There's no need to call a string function and have function call
overhead if you remember how C strings work: they have a null
terminator. So if the first char in a string is a null terminator, then
the string is completely empty. So you don't need to call that function.
The previous check by mwpenny had a few issues:
(a) It was completely overcomplicated for no good reason, and was
basically a Rube Goldberg machine. The original check was...
(1) Creating an std::string of the last char of 'output'...
(2) ...except instead of using the normal std::string constructor, it
was using the one where you pass in a number and a char to create
a string that's just that char repeated N times... except this
was only used to create a 1-length string.
(3) Converted that std::string to a C string.
(4) Then passed it to strcmp(), despite the string at this point
being only one byte and you could just compare the char values
directly.
The original check could've just been:
output[SDL_strlen(output) - 1] == *pathSep
(b) Use of libc strcmp() and strlen() instead of SDL_strcmp() and
SDL_strlen().
Now, actually, PHYSFS_getDirSeparator() happens to be a char array and
not a single char, so mwpenny was going in the right direction by using
strcmp() after all. Except it doesn't seem like he thought about the
fact that PHYSFS_getDirSeparator() could be multiple bytes instead of
one, and so he ended up making the first argument to strcmp() always be
a one-byte char array.
So there's issue (c), which is that it assumes the path separator is one
byte instead of multiple.
This commit fixes all of these issues with the trailing path separator
check.
If necessary, the caller can provide a fallback to be returned in case
the input given isn't a valid integer, instead of having to duplicate
the is_number() check.
This is simply a wrapper function around SDL_atoi(), because SDL_atoi()
could call libc atoi(), and whether or not invalid input passed into the
libc atoi() is undefined behavior depends on the given libc. So it's
safer to just add a wrapper function that checks that the string given
isn't bogus.
std::isdigit() can be replaced with SDL_isdigit(), but there's no
equivalent for std::isxdigit() in the SDL standard library. So we'll
just use libc isxdigit() instead, it's fine.
When I added the over-30-FPS mode, I kept running into this problem
where the special images of text boxes would render during the
deltaframes of fade-in/fade-out animations, even though they shouldn't
be. So I simply added a flag to the text box that enables drawing these
special images.
However, this doesn't solve the problem fully, and there's still a small
chance that a special-image text box could draw another special image
during its deltaframes. It's really rare and you have to have your
deltaframe luck juuuuuust right (or you could use libTAS, probably), but
it helps to be in 40% slowmode and have a high refresh rate (which, if
it isn't a multiple of 30, you should disable VSync, too, in order to
not have a low framerate).
So instead, special images will only be drawn if the text box has fully
faded in completely. That solves the issue completely.
This commit fixes a bug that's existed since MMMMMM was added (so, 2.2),
where if you quicksaved in a custom level while no music was playing,
then quit and loaded that quicksave, and you were using PPPPPP while
having MMMMMM available, it would play MMMMMM track 15, even though the
game intends for the music to simply be silent.
This is due to the same bug that lets you play MMMMMM tracks if you're
on PPPPPP - musicclass::play() does a modulo, but C++ modulo is not
guaranteed to be positive given negative inputs, so the 16-track offset
is added to a negative number, resulting in targeting the MMMMMM
soundtrack instead of PPPPPP.
That exploit doesn't harm anyone and shouldn't be fixed, EXCEPT it
causes a problem in this specific case. But this bug can be fixed
without removing that exploit.
Note that I made the check do not-equal to -1 instead of greater-than
-1, so levels that intend on using track -2, -3, -4, etc. upon loading a
quicksave will still work as their creator intended. It's just that
specifically -1 is patched out, just to fix this issue.
For some reason, ScaleSurface() was drawing each pixel one pixel up and
to the left from its actual position. I have no idea why.
But this was causing an issue where pixels would just be dropped,
because they would be drawn outside of the temporary SDL_Surface from
which the scaled surface would be drawn onto. Also, the offset just
creates a visual one-pixel offset in the result for no reason. So I'm
just removing it.
Big Viridian also suffered from this one-pixel offset, so now they will
no longer look like they're floating above the ground when standing on
the floor.
ScaleSurfaceSlow() uses DrawPixel() instead of SDL_FillRect() to scale a
given surface, which is slow and inefficient, and makes it less likely
that the game could be moved to SDL_Render.
Unfortunately, it has this weird -1,-1 offset, but that will be fixed in
the next commit.
So, earlier in the development of 2.0, Simon Roth (I presume)
encountered a problem: Oh no, all my backgrounds aren't appearing! And
this is because my foregroundBuffer, which contains all the drawn tiles,
is drawing complete black over it!
So he had a solution that seems ingenius, but is actually really really
hacky and super 100% NOT the proper solution. Just, take the
foregroundBuffer, iterate over each pixel, and DON'T draw any pixel
that's 0xDEADBEEF. 0xDEADBEEF is a special signal meaning "don't draw
this pixel". It is called a 'key'.
Unfortunately, this causes a bug where translucent pixels on tiles
(pixels between 0% and 100% opacity) didn't get drawn correctly. They
would be drawn against this weird blue color.
Now, in #103, I came across this weird constant and decided "hey, this
looks awfully like that weird blue color I came across, maybe if I set
it to 0x00000000, i.e. complete and transparent black, the issue will be
fixed". And it DID appear to be fixed. However, I didn't look too
closely, nor did I test it that much, and all that ended up doing was
drawing the pixels against black, which more subtly disguised the
problem with translucent pixels.
So, after some investigation, I noticed that BlitSurfaceColoured() was
drawing translucent pixels just fine. And I thought at the time that
there was something wrong with BlitSurfaceStandard(), or something.
Further along later I realized that all drawn tiles were passing through
this weird OverlaySurfaceKeyed() function. And removing it in favor of a
straight SDL_BlitSurface() produced the bug I mentioned above: Oh no,
all the backgrounds don't show up, because my foregroundBuffer is
drawing pure black over them!
Well... just... set the proper blend mode for foregroundBuffer. It
should be SDL_BLENDMODE_BLEND instead of SDL_BLENDMODE_NONE.
Then you don't have to worry about your transparency at all. If you did
it right, you won't have to resort this hacky color-keying business.
*sigh*
For some reason, the cursor would be either disabled and re-enabled if
you switched to windowed mode, or it would be always enabled if you
switched to fullscreen mode. This only happened when you toggled
fullscreen using the Alt+Enter, Alt+F, or F11 keybinds, and the
fullscreen option in graphic options doesn't have this problem.
This cursor toggling business seems like an arcane incantation back in
the days of SDL1.2, now since no longer necessary with SDL2. However,
after some testing, it seems like removing these indecipherable runes
don't cause any harm, so I'm going to remove them.
Fixes#371.
This function checked the intersection of rectangles, but it used floats
for some reason when its only caller used ints. There's already an
intersection function (UtilityClass::intersects()), so we should be
using that one instead in order to minimize code duplication.
Graphics::Hitest() is used for per-pixel collision detection, directly
checking the pixels of both entities' sprites. I checked with one of my
TASes and it still syncs, so I'm pretty sure this won't cause any
issues.
If you have the translucent room name option enabled, you'd always be
seeing the spikes at the bottom of the screen hidden behind the room
name. This patch makes it so that the spikes get carefully cropped so
they only appear above the room name when the player gets close to the
bottom of the screen.
The function was not actually checking the number that would end up
being used to index the tiles3 vector, and as a result there could
potentially be out-of-bounds indexing. But this is fixed now.
The half-second delay comes from the fact that the game uses
graphics.resumegamemode to go back to GAMEMODE. This system waits for
graphics.menuoffset to reach a certain threshold before actually going
back (this is the animation of the map screen being brought down). To
speed it up, I'll just set graphics.menuoffset directly.
I could've directly set game.gamestate to GAMEMODE, but I wanted to be
safe and use the existing system instead.
This removes the arrays of zeroes that still take up space in the
binary. It doesn't seem like the compiler optimizes or compresses these
zeroes anyway, so just remove them instead, and make
load()/loadminitower()/loadminitower2() no-op functions. The
minitower/contents arrays are already initialized zeroed-out, anyway, so
there's no need to keep these other arrays around.
This saves exactly 72 kilobytes of memory and binary size.
This moves the teleporter, activity prompt, and trophy text "don't draw"
conditionals to the part where the game checks collision with them,
instead of whenever the game draws them.
This makes it so that the game smoothly does the fade-in/fade-out
animation instead of suddenly stopping rendering them whenever their
"don't draw" conditions apply. Now, the "Press ENTER to activate
terminal" prompt will no longer suddenly disappear whenever you activate
one, and the "- Press ENTER to Teleport -" prompt will smoothly fade
back in after teleporting, instead of suddenly popping in on screen.
When I refactored custom level entity creation logic earlier, I forgot
to account for enemy bounds, so enemies would take on the same bounds as
platforms. But this is fixed now.
When I compile in release mode, GCC complains that these variables MAY
be uninitialized when it comes time to create the moving platform
entity. I can't think of any way that it could be uninitialized and
testing my release build seemed to be fine, but I'm initializing these
variables just to be sure.
M&P contains network code despite M&P not being a Steam/GOG release (as
Steam/GOG releases are full releases of the game, not
custom-levels-only releases).
While unlocking achievements is already ifdef'd out in M&P, let's remove
the network code entirely to make sure people can't do other shenanigans
with M&P builds, and also to have a smaller binary size.
This fixes a possible graphical issue where the "Press ENTER to activate
terminal" prompt gets drawn on top of the "- Press ACTION to advance
text -" prompt, which looks bad. Trophy text gets drawn on top, too, so
there's a check there as well.
I've also made it so the activity prompt doesn't get drawn if the player
doesn't have control. After all, what use is it to say "press ENTER" if
the player isn't allowed to?
The game time is moved a little to the left, and the trinket count a
little to the right. To fix#376 for real, the trinket count is now
positioned automatically based on its length. The trinket icon is now
also displayed at the far right (instead of to the left of the count)
for better symmetry, and so that switching between tele save and quick
save doesn't make the trinket icon move if the trinket counts have
different lengths.
While I'm going to fix#376 anyway to make longer numbers (like Seventy
Eight) fit, I decided to also make the box a little wider in advance of
the game being localized, those extra chars could be a lifesaver, while
you wouldn't really notice the difference if you play the game in
English.
