This saves one indentation level. I also fixed the comments a bit
(multiline instead of single-line, "gravitron squares" instead of "SWN
enemies", also commented the player exclusion from horizontal wrapping
in vertically-wrapping rooms).
This fixes a bug where using the fullscreen toggle keybind (Alt+Enter,
Alt+F, or F11) wouldn't update the color of the "resize to nearest" menu
option. The color doesn't functionally change anything - the option
still won't work, and will still have the message telling you that you
need to be in windowed mode when you move your menu selection to it -
but it's an easy inconsistency to fix; just move the menu recreation in
to Screen::toggleFullScreen() itself.
The game dereferences graphics.screenbuffer without checking it first...
it's unlikely to happen, but the least we can to do be safe is to add a
check and assert here.
If there were two scripts with the same name, removing one of them would
only remove the other script from the script name list, and not also
remove the contents of said script - leading to a desync in state, which
is probably bad.
Fixing this isn't as simple as removing the break statement - I either
also have to decrement the loop variable when removing the script, or
iterate backwards. I chose to iterate backwards here because it
relocates less memory than iterating forwards.
No need to use it when good ol' loops work just fine.
Iterating backwards is correct here, in case there happen to be more
than one of the item in the vectors, and also to minimize the amount of
memory that needs to be relocated.
This is a simple change - we draw minimap.png, instead of the generated
custom map, if it is a per-level mounted custom asset.
Custom levels have already been able to utilize minimap.png, but it was
limited - they could do gamemode(teleporter) in a script, and that would
show their customized minimap.png, but it's not like the player could
look at it during gameplay.
I would have done this earlier if I had figured out how to check if a
specific asset was mounted or not.
Previously, if the game couldn't set the write dir to the base
directory, or couldn't make the base directory, or couldn't calculate
the base directory, it would probably dereference NULL or read from
uninitialized memory or murder your family or something. But now, I've
eliminated the potential Undefined Behavior from the code dealing with
the base path.
Previously, this function had a bug due to failing to account for array
decay. My solution was to just repeat the MAX_PATH again. But in
hindsight I realize that's bad because it hardcodes it, and introduces
the opportunity for an error where we update the size of the original
path but not the size in the function.
So instead, just pass the size through to the function.
I don't want to add too many asserts, because sometimes it's okay if a
file is missing (mmmmmm.vvv). But currently, the game basically expects
all images and sound effects to be present. That might change in the
future, but for now, these asserts are okay.
FILESYSTEM_loadFileToMemory() dereferenced pointers without checking if
they were valid... I don't know of any cases where they could have been
NULL, but better safe than sorry.
So, the codebase was kind of undecided about who is responsible for
initializing the parameters passed to FILESYSTEM_loadFileToMemory() - is
it the caller? Is it FILESYSTEM_loadFileToMemory()? Sometimes callers
would initialize one variable but not the other, and it was always a
toss-up whether or not FILESYSTEM_loadFileToMemory() would end up
initializing everything in the end.
All of this is to say that the game dereferences an uninitialized
pointer if it can't load a sound effect. Which is bad. Now, I could
either fix that single case, or fix every case. Judging by the title of
this commit, you can infer that I decided to fix every case - fixing
every case means not just all cases that currently exist (which, as far
as I know, is only the sound effect one), but all cases that could exist
in the future.
So, FILESYSTEM_loadFileToMemory() is now guaranteed to initialize its
parameters even if the file fails to be loaded. This is better than
passing the responsibility to the caller anyway, because if the caller
initialized it, then that would be wasted work if the file succeeds
anyway because FILESYSTEM_loadFileToMemory() will overwrite it, and if
the file fails to load, well that's when the variables get initialized
anyway.
My next commit will involve using goto to jump to the end of a function
to initialize the variables to NULL, but that results in a compiler
error if we have initializations in the middle of the function. We might
as well put all declarations at the top of each block anyway, to help
the move to C, so I'm doing this now.
Since the length variable in the STDIN block now overshadows the length
variable in the outer block, I've renamed the length variable in the
block to stdin_length.
These casts are sprinkled all throughout the graphics code when creating
and initializing an SDL_Rect on the same line. Unfortunately, most of
these are unnecessary, and at worst are wasteful because they result in
narrowing a 4-byte integer into a 2-byte one when they don't need to
(SDL_Rects are made up of 4-byte integers).
Now, removing them reveals why they were placed there in the first place
- a warning is raised (-Wnarrowing) that implicit narrowing conversions
are prohibited in initializer lists in C++11. (Notably, if the
conversion wasn't narrowing, or implicit, or done in an initializer
list, it would be fine. This is a really specific prohibition that
doesn't apply if any of its sub-cases are true.)
We don't use C++11, but this warning can be easily vanquished by a
simple explicit cast to int (similar to the error of implicitly
converting void* to any other pointer in C++, which works just fine in
C), and we only need to do it when the warning is raised (not every
single time we make an SDL_Rect), so there we go.
This fixes a bug where after loading in to the level editor, pressing
Esc and then switching your option to something other than the first
option, then pressing Esc again to close the menu, then pressing Esc
once more would not keep your menu option.
This is because the code that checks if Menu::ed_settings is already in
the stack doesn't account for if ed_settings is the current menu - the
current menu doesn't get put in to the stack.
In hindsight, maybe I could have designed the new menu system better so
the current menu IS on the stack, and/or should have used a
statically-allocated linked list for each menu name for the stack frames
(instead of an std::vector) and asserted if a menu that already existed
in the stack was created instead... that'll have to be done later,
though.
Pressing Esc to cancel the confirm quit menu didn't play the squeak, in
contrast to pressing ACTION to cancel it, so now it does; pressing Esc
to close the pause menu or pressing ACTION will also now play the
Viridian squeak too.
vx/vy mean x-velocity and y-velocity... except here, where it seems like
they're used as extra parameters that do different things depending on
the entity. But it seems like at one point they were actually meant to
be the speed of the entity (this is the case for the unused decorative
particle entities), and then just never got renamed when they weren't.
The custom levels community named these two parameters meta1 and meta2
in the reference list of entities for the createentity() script command,
so that's what I'm naming them here. This will avoid confusion (I know
that some people reading this function have genuinely mistaken the vx/vy
for actually meaning x-velocity and y-velocity, simply because they were
named that way).
I have spelled out each overloaded version instead, and only the
overloads that are actually used - which just happens to be everything
except the 8-argument one. I don't want to deal with callers right now
(there are too many of them), so I'm not going to change the names that
the callers use, nor do I want to change the amount of arguments any
existing callers use right now - but we will have to deal with them in
one way or another when we move to C.
The script command createentity() is always an int. But not only that,
every time createentity() is used, its arguments are always treated like
ints. Always. I knew that vx/vy were floats because of the int casts in
the function, but I didn't even realize that xp/yp were floats, too,
until I checked just now! That's how much they're treated like ints.
All int casts in createentity() have also been removed, due to being
unnecessary (either because of us suppressing MSVC implicit conversion
warnings, or because there are now no longer any conversions happening).
This boolean is assigned, and it is checked... but it's never assigned
to true, thus making it useless. I also checked 2.2 source and the same
thing happens there; to prevent any confusion, I'm removing this.
So... I did see that map.ypos was a float when I added over-30-FPS mode,
because map.oldypos wasn't there before... I'm guessing that I kind of
just ignored it at the time. But, c'mon, map.ypos and map.oldypos are
always treated as ints, so there's literally no reason for them to be
actually floats in reality. I didn't even know they were anything other
than ints until I checked Map.h.
This is quite simple - whenever the user uses their keyboard or
controller, we hide the mouse cursor. Whenever they move the mouse, we
show it again. This makes it so the cursor gets out of the way when they
play the game, but reappears when they need it.
There is also a timeout, to prevent strobing if the user decides to use
the keyboard/controller and mouse at the same time. There is no timeout
from hiding the mouse cursor, but there is a timeout from showing the
mouse cursor - this because it's okay if the mouse lingers for a few
frames when you start using the keyboard, but really annoying if the
mouse doesn't instantly appear when you move it.
The config option has been removed. I'm going to implement something
that automatically shows and hides the mouse cursor whenever
appropriate, which is better than a config option.
So there were actually only two instances of C99-style end-of-line
comments in C files - and technically one of them was just a C file
including MakeAndPlay.h.
If a track was restarted after it faded out, then it wouldn't play. This
is because currentsong wasn't set to -1 after fading out, and that is
because the fade out calls pause() instead of haltdasmusik() when it
finishes.
Unlike f196fcd896, this fixes the time
trial music while keeping it to the same behavior as 2.2, and fixes
every single possible case that this music bug could have happened.
This reverts only a part of f196fcd896 -
as the original commit author did not do their changes atomically, they
also squashed in a de-duplication within the same commit. So I'm only
reverting the part of the commit that wasn't the de-duplication, which
is simply the changes to the music.fadeout() calls.
This is being (partially) reverted for several reasons:
1. It's not the correct behavior. What this does instead is persist the
track through after you restart the time trial, instead of fading it
out, then restarting it again. This is in contrast to behavior in
2.2, and I see no reason to not keep the same behavior.
2. It's a single-case patch. The time trials are not the only time in
the game a music track could fade out and then be restarted with the
same track - custom levels could do the same thing too. Instead of
fixing only one case, we should strive to fix EVERY case.
The original commit author (trelbutate) also didn't write anything in
the commit description of f196fcd896. What
you should write in the commit description is things like rationale,
analysis, and other good information that would be useful to anyone
looking at your commit to understand why you did what you did. Having no
commit description leaves readers in the dark as to why you did what you
did.
Thus, I don't know why trelbutate went with this solution, or if they
knew that it was only a single-case patching, or if they knew that it
wasn't 2.2 behavior.
By not writing the commit description, they miss a chance for
reflection; speaking from personal experience, I myself have gone back
and improved my commits countless times because I wrote commit
descriptions for every single one of them, and sometimes whenever I
write them, I think to myself "hang on a minute, that doesn't sound
quite right" and end up finding improvements.
If trelbutate wrote a commit description, they might have realized that
it wasn't 2.2 behavior, and gone back and fixed up their commit to be
correct. As it stands, though, they didn't have to think about it in the
first place because they never bothered to write a commit description.
edteleportent is a global variable that gets assigned whenever the
player collides with a warp token, and gets read from later down the
line in gamelogic(). While I don't know of any way to cause anything bad
with this (and I did try), storing a temporary indexing variable like
this is only bound to be a liability in the future - so we might as well
prevent badness now by adding a bounds check here.
This fixes a bug where quitting to the menu from command-line
playtesting with -playassets specified would always use those assets
when loading back in to any custom level. This also fixes loading in to
a custom level quicksave always using the command-line playtesting
arguments instead of using the actual quicksave.
In a vertically-warping room, the 'height' of the room becomes 232
pixels, regardless of if you have a room name or not. So the remaining 8
rows of pixels at the bottom of the screen corresponds with the first 8
rows of pixels at the top of the screen, and entities in the bottom 8
rows of pixels get teleported to the top of the screen.
The screen wrapping drawing code doesn't draw entities in the top 8 rows
of pixels at the bottom, leading to a discontinuous effect where it
looks like vertically-warping entities don't neatly change from the
bottom to the top or vice versa - this is especially noticeable with
enemies. To fix this, just increase the threshold for drawing top
entities at the bottom of the screen by 8 pixels.
When an entity vertically warps, it teleports upwards or downwards by
232 pixels. However, the graphics code draws them with an offset of 230
pixels. This is off by 2 pixels, but it's enough to make a
downwards-moving enemy look like it suddenly collides with the bottom of
the screen (in a room without a room name) before it warps, especially
if you go frame-by-frame.
It seems like for whatever reason that the frames portion of save files
is never read from, and always zeroed. Well, technically they get parsed
but the result is immediately discarded afterwards.
I see no reason to do this, so I'm removing these zeroes.
This fixes being able to make music fully fade in (or out) by unfocusing
the game, or making the fade bars fully fade in (or out) by unfocusing
the game, or racking up the timer while the game is unfocused.
In 2.2 and previous, the game would call resetgameclock() every frame
for the last 30 frames of the time trial countdown in order to make sure
it gets reset. This was in a render function, and didn't get brought out
in 2.3, so 2.3 resets the game clock *while rendering*, which is kinda
bad and is an oversight on my part for not noticing.
Instead of doing that, just add a conditional to the timer so that it
won't tick during the time trial countdown. This fixes#699 even further
by making it so the time trial par can't even be lost during the
countdown, because the timer won't tick up - so you can never get a sad
squeak to play by pausing the game or unfocus-pausing it during the
countdown.
For some reason, resetgameclock() is only ever used in gamerender(), and
everywhere else just zeroes the clock manually. This is weird to me, so
I've made it so everywhere that zeroes the clock uses the
resetgameclock() function to do so.
Otherwise, if the timer ticked up past the par (via using the unfocus
pause or pause menu), it would result in the sad squeak being played
every frame because the game would constantly be setting
timetrialparlost, then moving to the code block below, assuming that
since timetrialparlost that we haven't lost the par already, and playing
the squeak.
timetrialparlost gets reset in hardreset() and startgamemode() anyways,
so there's no need to be constantly resetting this variable.
Fixes#699.
It turns out this entire chunk of code is simply unneeded (and is
actively harmful) since when we're done with the time trial,
quittomenu() gets called, and that removes the previous stack frame
anyway.
I'm guessing that I added this code, then added quittomenu(), then
didn't consider how this code and quittomenu() would mix. But anyways,
this bug is fixed.
Fixes#714.
This seems to be a comment left by Ethan that he never got around to. So
I did it for him.
What I've done is made it so FileSystemUtils.cpp knows what a binary
blob is, and moved the binary blob loading code directly to
FileSystemUtils.cpp. To do this, I removed the private access modifier
from binaryBlob - I don't think we'll need it, and anyways when we move
to C we can't use it.
Along the way, I also cleaned up the style of the function a bit - the
null termination offset is no longer hardcoded, and the function no
longer mixes code and declarations together in the same block.
I also noticed that when printing all the filenames at the end, a single
invalid header would stop the whole loop instead of just being skipped
over... this seems to be a bug to me, so I've made it so invalid headers
just get skipped over instead of stopping the whole loop.
In FileSystemUtils.h, I used a forward declaration. In hindsight,
incomplete forward declarations should basically always be done in
header files if possible, otherwise this introduces the possibility of
transitive includes - if a file includes this header and it does a full
include, the file is silently able to use the full header, whereas if
it's a forward declaration, then the moment the file tries to use the
full header it fails, and then it's forced to include the full header
for itself. But uh, that's a code cleanup for later.
While fixing all the other music bugs, I discovered that starting
playtesting in the editor wouldn't play the level music.
The problem is that the editor playtesting start code calls
music.fadeout() before calling music.play(). This queues up the track
from the music.play() call. After that, what should happen is that
processmusic() processes the fade, the fade is then finished, and then
after that it sees that the music is halted so it can play the queued
track.
Instead what happens is that the function first attempts to play the
music before the fade is processed and finished, so play() will re-queue
the music again, but the queue gets cleared right after that (this is a
subtle bit of behavior - it means if the game fails to play a queued
track due to it fading, it's not going to re-queue it again and end up
in some sort of infinite loop).
This is a frame ordering issue - the function is tripping over itself
when it shouldn't be. To fix it, just put the queue processing code
after the fade processing code.
