ClearSurface() is less verbose than doing it the old way, and also
conveys intent clearer. Plus, some of these FillRect()s had hardcoded
width and height values, whereas ClearSurface() doesn't - meaning this
change also has better future-proofing, in case the widths and heights
of the surfaces involved change in the future.
This is pretty old commented-out code from earlier versions of the game;
they are no longer useful, and are just distracting. If we need them, we
can always refer back to this commit (but I sincerely doubt that we'll
need them).
Apparently in C, if you have `void test();`, it's completely okay to do
`test(2);`. The function will take in the argument, but just discard it
and throw it away. It's like a trash can, and a rude one at that. If you
declare it like `void test(void);`, this is prevented.
This is not a problem in C++ - doing `void test();` and `test(2);` is
guaranteed to result in a compile error (this also means that right now,
at least in all `.cpp` files, nobody is ever calling a void parameter
function with arguments and having their arguments be thrown away).
However, we may not be using C++ in the future, so I just want to lay
down the precedent that if a function takes in no arguments, you must
explicitly declare it as such.
I would've added `-Wstrict-prototypes`, but it produces an annoying
warning message saying it doesn't work in C++ mode if you're compiling
in C++ mode. So it can be added later.
Wow, there are a lot of these. All of these exit paths now use
VVV_exit() instead, which attempts to save unlock.vvv and settings.vvv,
and also frees all resources so Valgrind is happy. This is a good thing,
because previously unlock.vvv/settings.vvv wouldn't be written to if we
decided to bail for a given reason.
This isn't a memory leak (not even Valgrind complains), because it gets
properly cleaned up in GraphicsResources::destroy(). Still, it's memory
that is just laying around not being used, and in the name of
deallocating things as soon as you no longer need them, we should
deallocate the base tilesheet images after we split all of them into
tiles.
This reduces the memory cost of all tilesheet images by half, since we
were essentially keeping around duplicates for nothing; this doesn't
really have much of an impact with conventional tilesheet sizes, since
they're usually small enough, but since 2.3 allowed for tilesheet images
of any size, this is a pretty big deal for really big tilesheet images.
It's okay to do this, even though they also get freed in
GraphicsResources::destroy(), because SDL_FreeSurface() does a NULL
check on the pointer passed to it, and we set the pointer to NULL after
freeing the surfaces.
This removes the music cleanup code from musicclass::init(), and
requires that we also call destroy() in Graphics::reloadresources().
This is because we'll need to re-use the musicclass cleanup code
elsewhere, and we don't want to copy-paste the cleanup code. Or at
least, I don't (but I'm not a game dev, game devs copy-paste all the
friggin' time).
It doesn't feel quite write leaving all the buffer creation code in
main(), even though it's perfectly okay to do so and it doesn't result
in any memory mismanagement that Valgrind can report; so I'm factoring
all of it out to a separate function, Graphics::create_buffers().
As a bonus, we no longer have to keep qualifying with `graphics.` in the
buffer creation code, which is nice.
These destroy all the buffers that are created on the Graphics class.
Since these buffers can't be created at the same time as the rest of
Graphics is (due to the fact that they require knowing the pixel format
of the game screen), they can't be destroyed at the same as the rest of
Graphics is, either.
This is a very complicated way of zeroing out grphx (instead of using
SDL_zero()), which itself is completely unnecessary because grphx.init()
gets called immediately afterwards anyway.
It should be next-line brace, not same-line brace. Even in a codebase
that uses same-line braces everywhere, I still prefer having next-line
braces inside functions (because they're at the top level, and you can't
next them). But regardless, this should still be next-line brace like
(most of) the rest of the codebase.
While compiling in release mode, GCC warns about these two potentially
being used uninitialized further down. The only way this could happen is
if the case-switches below didn't match up with a case, which would
require the game to be in an invalid state (and have invalid values for
rcol and spcol), but it's better to be safe than sorry.
During 2.3 development, there's been a gradual shift to using SDL stdlib
functions instead of libc functions, but there are still some libc
functions (or the same libc function but from the STL) in the code.
Well, this patch replaces all the rest of them in one fell swoop.
SDL's stdlib can replace most of these, but its SDL_min() and SDL_max()
are inadequate - they aren't really functions, they're more like macros
with a nasty penchant for double-evaluation. So I just made my own
VVV_min() and VVV_max() functions and placed them in Maths.h instead,
then replaced all the previous usages of min(), max(), std::min(),
std::max(), SDL_min(), and SDL_max() with VVV_min() and VVV_max().
Additionally, there's no SDL_isxdigit(), so I just implemented my own
VVV_isxdigit().
SDL has SDL_malloc() and SDL_free(), but they have some refcounting
built in to them, so in order to use them with LodePNG, I have to
replace the malloc() and free() that LodePNG uses. Which isn't too hard,
I did it in a new file called ThirdPartyDeps.c, and LodePNG is now
compiled with the LODEPNG_NO_COMPILE_ALLOCATORS definition.
Lastly, I also refactored the awful strcpy() and strcat() usages in
PLATFORM_migrateSaveData() to use SDL_snprintf() instead. I know save
migration is getting axed in 2.4, but it still bothers me to have
something like that in the codebase otherwise.
Without further ado, here is the full list of functions that the
codebase now uses:
- SDL_strlcpy() instead of strcpy()
- SDL_strlcat() instead of strcat()
- SDL_snprintf() instead of sprintf(), strcpy(), or strcat() (see above)
- VVV_min() instead of min(), std::min(), or SDL_min()
- VVV_max() instead of max(), std::max(), or SDL_max()
- VVV_isxdigit() instead of isxdigit()
- SDL_strcmp() instead of strcmp()
- SDL_strcasecmp() instead of strcasecmp() or Win32 strcmpi()
- SDL_strstr() instead of strstr()
- SDL_strlen() instead of strlen()
- SDL_sscanf() instead of sscanf()
- SDL_getenv() instead of getenv()
- SDL_malloc() instead of malloc() (replacing in LodePNG as well)
- SDL_free() instead of free() (replacing in LodePNG as well)
Ever since 2.0, the colors of some of the Time Trial trophies in the
Secret Lab don't correspond to the crewmate of the given level. The
trophy for the Tower uses Victoria's color, and the Lab trophy uses
Vermilion's color. The Space Station 2 trophy uses Viridian's color, and
the Final Level trophy uses Vitellary's color.
This doesn't appear to be intentional, and it would be odd if it was,
since this game matches the colors everywhere else (each zone on the map
is colored with their respective crewmate in mind, for instance). Also,
the Lab trophy has the sad expression, which is Victoria's trait - it
would be weird if this was intended for Vermilion instead.
But the biggest piece of evidence that this was unintentional is the
corresponding comment for each color in Graphics::setcol(). It mislabels
yellow as cyan, cyan as yellow, blue as red, and red as blue.
To fix this, I simply have to set the correct color for each trophy in
case 25 of entityclass::createentity(). I could fix it in
Graphics::setcol() itself, but custom levels might depend on those
certain colors being the way they are, so it's a safer bet to just fix
it in the trophy creation case itself.
The diff of this might look weird. Even though all I'm doing is changing
some value assignments around, it looks like the "patience" algorithm
thinks I'm moving a whole case of the trophy switch-case around.
In C++, when you have two variables in different scopes with the same
name, the inner scope wins. Except you have to be really careful because
sometimes they're not (#507). So it's better to just always have unique
variable names and make sure to never clash a name with a variable in an
outer scope - after all, the C++ compiler and standard might be fine
with it, but that doesn't mean humans can't make mistakes reading or
writing it.
Usually I just renamed the inner variables, but for tx/ty in editor.cpp,
I just got rid of the ridiculous overcomplicated modulo calculations and
replaced them with actual simple modulo calculations, because the
existing ones were just ridiculous. Actually, somebody ought to find
every instance of the overcomplicated modulos and replace them with the
actual good ones, because it's really stupid, quite frankly...
This commit fixes a bug that also sometimes occurred in 2.2, where the
teleporter sprite would randomly turn into a solid color and just be a
solid circle with no detail.
Why did this happen? The short answer is an incorrect lower bound when
clamping the teleporter sprite index in `Graphics::drawtele()`. The long
answer is bad RNG with the teleporter animation code. This commit fixes
the short answer, because I do not want to mess with the long answer.
So, here is what would happen: the teleporter's `tile` would be 6. The
teleporter code decrements its `framedelay` each frame. Then when it
reached a `framedelay` of 0, it would call `fRandom()` and essentially
ask for a random number between 0 and 6. If the RNG ended up being
greater than or equal to 4, then it would set its `walkingframe` to -5.
At the end of the routine, the teleporter's `drawframe` ends up being
its `tile` plus its `walkingframe`. So having a `walkingframe` of -5
here is fine, because its `tile` is 6.
Until it isn't. When its `tile` becomes 2, it still keeps its
`walkingframe` around. The code that runs when its `tile` is 2 does have
the possibility of completely resetting its `walkingframe` to be in
bounds (in bounds after its `tile` is added), but that only runs when
its `framedelay` is 0, and in the meantime it'll just use the previous
`walkingframe`.
So you could have a `walkingframe` of -5, plus a `tile` of 2, which
produces a `drawframe` of -3. Then `Graphics::drawtele()` will clamp
that to 0, which just means it'll draw the teleporter backing, and the
teleporter backing is a simple solid color, so the teleporter will end
up being completely and fully solid.
To fix this, I just made `Graphics::drawtele()` clamp to 1 on the lower
bound, instead of 0. So if it ever gets passed a negative teleporter
index, it'll just draw the normal teleporter sprite instead, which is
better.
This fixes the draw order by drawing all other entities first, before
then drawing all humanoids[1] after, including the player afterwards.
This is actually a regression fix from #191. When I was testing this, I
was thinking about where get a crewmate in front of another entity in
the main game, other than the checkpoints in Intermission 1. And then I
thought about the teleporters, because I remember the pre-Deep Space
cutscene in Dimension Open looking funny because Vita ended up being
behind the teleporter. (Actually, a lot of the cutscenes of Dimension
Open look funny because of crewmates standing behind terminals.)
So then I tried to get crewmates in front of teleporters. It actually
turns out that you can't do it for most of them... except for Verdigris.
And then that's what I realized why there was an oddity in WarpClass.cpp
when I was removing the `active` system from the game - for some reason,
the game put a hole in `obj.entities` between the teleporter and the
player when loading the room Murdering Twinmaker. In a violation of
Chesterton's Fence (the principle that you should understand something
before removing it), I shrugged it off and decided "there's no way to
support having holes with my new system, and having holes is probably
bad anyway, so I'm going to remove this and move on". The fact that
there wasn't any comments clarifying the mysterious code didn't help
(but, this *was* 2.2 code after all; have you *seen* 2.2 code?!).
And it turns out that this maneuver was done so Verdigris would fill
that hole when he got created, and Verdigris being first before the
teleporter would mean he would be drawn in front of the teleporter,
instead of being behind it. So ever since
b1b1474b7b got merged, there has actually
been a regression from 2.2 where Verdigris got drawn behind the
teleporter in Murdering Twinmaker, instead of properly being in front of
it like in 2.2 and previous.
This patch fixes that regression, but it actually properly fixes it
instead of hacking around with the `active` system.
Closes#426.
[1]: I'm going to go on a rant here, so hear me out. It's not explicitly
stated that the characters in VVVVVV are human. So, given this
information, what do we call them? Well, the VVVVVV community (at least
the custom levels one, I don't think the speedrunning community does
this or is preoccupied with lore in the first place) decided to call
them "villis", because of the roomname "The Villi People" - which is
only one blunder in a series of awful headcanons based off of the
assumption that the intent of Bennett Foddy (who named the roomnames)
was to decree some sort of lore to the game. Another one being
"Verdigris can't flip" because of "Green Dudes Can't Flip". Then an OC
(original character) got named based off of "The Voon Show" too. And so
on and so forth.
I'm going to refactor drawing an entity out to a separate function, and
since I'm going to do that, I might as well make some things const to
clarify intent first and foremost and possibly improve performance or
compiler optimization.
Instead of using the same tower buffer that gets used for towers, use a
separate buffer instead so there's no risk of stepping on the tower
buffer's toes at the wrong point in time.
This commit combined with the previous one fixes#369.
With the previous commit in place, we can now simply move some usages of
the previous towerbg to use a separate object instead. That way, we
don't have to mess with a monolithic state, or some better way to phrase
what I just said, and we instead have two separate objects that can
coexist side-by-side.
Previously, the tower background was controlled by a disparate set of
attributes on Graphics and mapclass, and wasn't really encapsulated. (If
that's what that word means, I don't particularly care about
object-oriented lingo.) But now, all relevant things that a tower
background has has been put into a TowerBG struct, so it will be easy to
make multiple copies without having to duplicate the code that handles
it.
For some reason, this `tl` is a `point`? But the only other time the
name `tl` is used elsewhere in the code is a float on a `textboxclass`.
Regardless, this is unused.
On the deltaframes of the tower background, there would be "pixel bleed"
if the tower background would be scrolling from the top. This is because
there wouldn't be any more pixels from above the screen to grab, because
the background rendering functions didn't draw any pixels above the
screen. But they couldn't draw any pixels above the screen, because that
was simply the end of the buffer. But now that the buffer is expanded,
we can now draw above the screen, and fix this glitchy interpolation
rendering.
In order to fix the weird title screen pixels at the top on deltaframes,
we'll need to have a bit more space at the top. Also to the left, in
case we need a background to scroll from the left in the future.
The window icon is simply another asset that can be customized by level
makers. And in fact, one of my levels changes the window icon. It's
simply named VVVVVV.png, but it doesn't sit in the graphics folder,
rather it sits in the root VVVVVV directory.
I noticed that this asset was missed when per-level assets loading was
added, so I decided to add it in.
There's a NULL check on screenbuffer because reloadresources() gets
called before screenbuffer's init() does.
I was investigating a desync in my Nova TAS, and it turns out that
the gravity line collision functions check for the `oldxp` and `oldyp`
of the player, i.e. their position on the previous frame, along with
their position on the current frame. So, if the player either collided
with the gravity line last frame or this frame, then the player collided
with the gravity line this frame.
Except, that's not actually true. It turns out that `oldxp` and `oldyp`
don't necessarily always correspond to the `xp` and `yp` of the player
on the previous frame. It turns out that your `oldyp` will be updated if
you stand on a vertically moving platform, before the gravity line
collision function gets ran. So, if you were colliding with a gravity
line on the previous frame, but you got moved out of there by a
vertically moving platform, then you just don't collide with the gravity
line at all.
However, this behavior changed in 2.3 after my over-30-FPS patch got
merged (#220). That patch took advantage of the existing `oldxp` and
`oldyp` entity attributes, and uses them to interpolate their positions
during rendering to make everything look real smooth.
Previously, `oldxp` and `oldyp` would both be updated in
`entityclass::updateentitylogic()`. However, I moved it in that patch to
update right before `gameinput()` in `main.cpp`.
As a result, `oldyp` no longer gets updated whenever the player stands
on a vertically moving platform. This ends up desyncing my TAS.
As expected, updating `oldyp` in `entityclass::movingplatformfix()` (the
function responsible for moving the player whenever they stand on a
vertically moving platform) makes it so that my TAS syncs, but the
visuals are glitchy when standing on a vertically moving platform. And
as much as I'd like to get rid of gravity lines checking for whether
you've collided with them on the previous frame, doing that desyncs my
TAS, too.
In the end, it seems like I should just leave `oldxp` and `oldyp` alone,
and switch to using dedicated variables that are never used in the
physics of the game. So I'm introducing `lerpoldxp` and `lerpoldyp`, and
replacing all instances of using `oldxp` and `oldyp` that my over-30-FPS
patch added, with `lerpoldxp` and `lerpoldyp` instead.
After doing this, and applying #503 as well, my Nova TAS syncs after
some minor but acceptable fixes with Viridian's walkingframe.
By "unnecessary qualifiers to self", I mean something like using the
'game.' qualifier for a variable on the Game class when you're inside a
function on the Game class itself. This patch is to enforce consistency
as most of the code doesn't have these unnecessary qualifiers.
To prevent further unnecessary qualifiers to self, I made it so the
extern in each header file can be omitted by using a define. That way,
if someone writes something referring to 'game.' on a Game function,
there will be a compile error.
However, if you really need to have a reference to the global name, and
you're within the same .cpp file as the implementation of that object,
you can just do the extern at the function-level. A good example of this
is editorinput()/editorrender()/editorlogic() in editor.cpp. In my
opinion, they should probably be split off into their own separate file
because editor.cpp is getting way too big, but this will do for now.
This fixes a bug where font_positions wouldn't get cleared after exiting
a custom level that had a font.txt if it didn't exist in the default
graphics, leading to messed-up-looking font rendering.
This bug was originally discovered by Ally.
When this is done, there is potential for a mistake to occur when
writing out the bounds check, which is eliminated when using the macro
instead. Luckily, this doesn't seem to have happened, but what's even
worse is I hardcoded 400 instead of using SDL_arraysize(ed.level), so if
the size of ed.level the bounds checks would all be wrong, which
wouldn't be good. But that's fixed now, too.
Since there's an INBOUNDS_ARR() macro, it's much better to specify the
macro for the vector is a macro for the vector, to avoid confusion.
All usages of this macro have been renamed accordingly.
Due to #464, standing inside a gravity line during a gotoroom that
occurs every frame will end up with the gravity line being gray instead
of being white. To temporarily fix this (until #464 is properly fixed),
I decided to add some kludge that colors it white if its onentity is 1.
I tested this patch with gravity lines in both constant-gotoroom and
normal environments, and it seems to be fine for both.
This patch fixes a regression caused by commit
6b1a7ebce6.
When you spawn a crewmate with an invalid color, by doing something like
`createentity(100,100,18,-1,0)` (here the color is -1, which is
invalid), a white crewmate with the color of solid white (255, 255, 255)
would appear.
That is, until AllyTally came along and committed commit
6b1a7ebce6 (Make "[Press ENTER to
return to editor]" fade out after a bit) (PR #158). Then after that
commit, it would seem like the crewmate didn't appear, but in reality
they were just invisible, because they had an invisible color.
As part of Ally's changes, to properly support drawing text with a
certain amount of alpha, she made BlitSurfaceColoured() account for the
alpha of the color given instead of only caring about the RGB of the
color, discarding the alpha, and using the alpha of the surface it was
drawing instead. This effectively made it so the alpha of whatever it
was drawing would be 255 all the time, except for if you had custom
textures and your custom textures had translucent pixels.
However, the default color set by Graphics::setcol() if you didn't
provide a valid color index was 0xFFFFFF. Which is only (255, 255, 255)
but ends up having an alpha value of 0 (because it's actually
0x00FFFFFF). And all colors drawn with alpha 0 end up being drawn with
alpha 0 after 6b1a7ebce6. So
invalid-colored entities will end up being invisible.
To fix this, I just decided to add alpha to the default value instead.
In addition, I used getRGB() to be consistent with all the other colors
in the function.
For some reason, there's some color-clamping code in this function
directly after already-existing color-clamping code. This code dates
back to 2.2. And also, there's a smaller lower-bound of -1 for the red
channel, despite the fact that this smaller value doesn't matter because
the red would get clamped to 0 by the first code anyway.
So even if this was put here for some strange reason, it doesn't matter
because it doesn't do anything anyway.
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.
