This macro is to make it so it won't be error-prone to write the
semi-confusing `(a % b + b) % b` statement, and you can just use an easy
macro instead.
Currently, the only places a positive modulo is needed is when switching
tilesets, enemies, and warp directions in the editor, as well as when
getting a tile in the tower, since towers just repeat themselves
vertically. Towers used this weird while-loop to sort of emulate a
modulo, which isn't half-bad, but is unnecessary, and I don't think any
compiler would recognize it as a modulo. (And if it's not optimized to a
proper modulo... what happens if the number being moduloed is really,
really big?)
Believe it or not, there are still some remnants of the ActionScript
coding standards in the codebase! And one of them sometimes pops up
whenever an integer division happens.
As it so happens, it seems like division in ActionScript automatically
produces a decimal number. So to prevent that, the game sometimes
subtracts off the remainder of the number to be divided before
performing the division on it.
Thus, we get statements that look like
(a - (a % b)) / b
And probably more parentheses surrounding it too, since it would be
copy-pasted into yet another larger expression, because of course it
would.
`(a % b)` here is subtracting the remainder of `a` divided by `b`, using
the modulo operator, before it gets divided by `b`. Thus, the number
will always be divisible by `b`, so dividing it will mathematically not
produce a decimal number.
Needless to say, this is unnecessary, and very unreadable. In fact, when
I saw these for the first time, I thought they were overcomplicated
_modulos_, _not_ integer division! In C and C++, dividing an integer by
an integer will always result in an integer, so there's no need to do
all this runaround just to divide two integers.
To find all of these, I used the command
rg --pcre2 '(.+?).+?-.+?(?=\1).+?%.+?([\d]+?).+?\/.+?(?=\2)'
which basically matches expressions of the form 'a - a % b / b', where
'a' and 'b' are identical and there could be any characters in the
spaces.
There's really no need to put the y-multiplication in a lookup table.
The compiler will optimize the multiplication better than putting it in
a lookup table will.
To improve readability and to hardcode things less, the new
SCREEN_WIDTH_TILES and SCREEN_HEIGHT_TILES constant names are used, as
well as adding a new TILE_IDX macro to calculate the index of a tile in
a concatenated-rows (row-major in formal parlance) array. Also, tile
numbers are stored in a temporary variable to improve readability as
well (no more copy-pasting `contents[i + vmult[j]]` over and over
again).
There's really no reason for this simple multiplication plus division to
be in a lookup table. The compiler will optimize it faster than putting
it in a lookup table will, I'm sure.
This comment was referring to a now-deleted variable named mkdirResult
that was binary-"or"ed with all mkdir() results... except for the saves
directory. That variable was only used for save file migration, which is
now axed, so this comment is referring to nothing now.
I don't really know the answer to Ethan's question, but it doesn't
matter now.
So, it turns out freeing everything in binaryBlob::clear() without
checking for NULL results in an abort() because clear() gets called on
musicWriteBlob after it attempts to write the compiled music. It's just
that no one's using VVV_COMPILEMUSIC, so no one's ran into this.
I'm keeping VVV_COMPILEMUSIC around so in the future people can compile
music directly from the game (and probably half the existing
VVV_COMPILEMUSIC code is going to be thrown out, but oh well).
Since this refers to specific exported file data, let's make sure this
is portable. I'm not sure if we'll ever ship on systems where
sizeof(int) != 4 or sizeof(bool) != 1, but better to be safer and
future-proof than not.
This variable is only used when compiling music. Since it doesn't
actually keep track of the number of headers otherwise, ifdef it behind
VVV_COMPILEMUSIC.
2.3 introduced a regression with destroy(platforms). The problem was
that isplatform wasn't being set to false when the entity got disabled,
so if the platform was moving, it would keep moving until it hit a wall,
instead of stopping immediately.
I have just built a new CentOS container based off of the updated
Dockerfile, which now has SDL 2.0.16 installed. This updates the CI to
point to the new container.
Previously, loading STDIN used std::istreambuf_iterator and std::vector
and whatnot because... I guess it was less typing? But this isn't 1989;
we have the disk space to spare and we don't need to use fancy stuff
just to save on typing. It's not that hard to implement an array that
regrows to the nearest power of two every time.
All system header includes should come before project-specific includes
(includes specific to this game), while coming after the include
specific to the given file (if any; main.cpp doesn't have any).
These are unused.
Ethan originally added them in case Terry wanted achievement
percentages. But he didn't add them, and I don't think the achievements
are changing anytime soon, so it's safe to remove this dead code.
If the string is hardcoded, then use compile-time string literal
concatenation instead.
I don't know if compilers are smart enough to recognize when you're
passing in hardcoded strings and to concatenate them into the string
literal at compile time instead. I also don't know that if compilers are
smart enough to recognize that, that further they recognize all the
logging functions are just wrappers around printf, and so they can
perform the same optimization at those function call sites, too. So it's
better to just do the string concatenation explicitly instead.
Instead of having three separate copies of the function list, use macro
magic to make it so there is only one list that we use in three
different cases.
SDL just got an API to toggle VSync without having to tear down the
renderer ( libsdl-org/SDL#4157 ). We can remove the workaround and use
that instead. For now, we are putting it behind an ifdef until SDL
2.0.18 officially releases in November.
Fixes#831.
Constants.h will house constants like the screen size and others. But
basically only the screen size for now.
Now we don't have to type that "4 bytes per 40 chars (whole screen)"
comment everywhere...
Since those are all downstream recipients of either static storage or
memory that doesn't move for the duration of the custom level, it's okay
to make these be `const char*`s without having to redo any of the RAII
memory management.
mapclass::currentarea() is included in this as well. I also cleaned up
Tower.cpp's headers to fix some transitive includes because I was
removing UtilityClass.h includes from all other level files too.
The "Untitled room" names no longer show any coordinates, because doing
so would require complicated memory management that's completely
unneeded. No one will ever see them, and if they do they already know
they have a problem anyway. The only time they might be able to see them
is if they corrupted the areamap, but this was only possible in 2.2 and
previous by dying outside the room deaths array in Outside Dimension
VVVVVV, which has since been patched out. Besides, sometimes the
"Untitled room" gets overwritten by something else anyway (especially in
Finalclass.cpp), so it really, really doesn't matter.
There's no reason it needs to be an std::string here.
Although, realistically, we should be using an enum instead of
string-typing, but, eh, that can be fixed later.
Companions would not spawn if you didn't load the current room via a
room transition. This meant that companions wouldn't spawn if you loaded
a save file with a companion, at least not until you moved to a
different room and triggered a screen transition. But most importantly,
it meant that the Intermission 1 supercrewmate would never spawn,
because going to Intermission 1 does a straight gotoroom, and does not
do a room transition.
Turns out the roomchange refactor broke things, because of course it
did. The companion logic was implicitly relying on that bool to be set,
because...? Either way, it doesn't make sense. Using roomchange implied
that the code wanted to be ran only when doing a room transition, which
is clearly not the case here. The best thing to do here is to just move
it to a separate function that gets called at the end of
mapclass::gotoroom().
So, I ended up breaking supercrewmate spawning with that roomchange
refactor. However, upon investigating how to fix it, I was running into
a weird interpolation issue due to scmmoveme, as well as the companion
spawning in the ground in "Very Good". And I was wondering why I or no
one else ended up running into them.
Well, as it turns out, scmmoveme ends up doing absolutely nothing. There
are only two instances where scmmoveme is used. The first is if you
respawn in "Very Good", and somehow have your scmprogress set to that
room. But that's impossible, because whenever you respawn, your
scmprogress is always set to the one after the room you respawn in. Even
if you respawned in the room previous to "Very Good" (which is "Don't
Get Ahead of Yourself!"), it still wouldn't work, since the logic always
kicks in when a gotoroom happens, and not only when a supercrewmate is
actually spawned. Since the scmprogress doesn't match, that case never
gets triggered, and we get to the second time scmmoveme is used, which
is in the catch-all case that always executes.
This second instance... also does nothing, because since we just
respawned, and our scmprogress got set to the room ahead of us, there is
no supercrewmate on screen. Then getscm() returns 0, and the player is
always indice 0, so the only thing we end up doing is setting the
player's x-position to their own x-position. Brilliant.
Anyway, this code results in interpolation issues and the supercrewmate
spawning in the ground on "Very Good" if you die, when my fix is
applied, because my fix moves this logic around to a different frame
order, and that actually ends up making scmmoveme no longer dead code.
So to recap: we have dead code, which looks like it does something, but
doesn't. But if you move it around in a certain way, it ends up having
harmful effects. One of the joys of working on this game...
It's also hilarious that it gets saved to the save file. Why? The only
time this variable is true, it is for literally less than a frame,
because it always gets set to false, because you always respawn using a
gotoroom whenever the supercrewmate dies, because you never respawn in
the same room as a supercrewmate, because Intermission 1 was
deliberately designed that way (else you'd keep continually dying since
the supercrewmate wouldn't move out of the way).
These were bfont_rect, bg_rect, foot_rect, and images_rect.
bg_rect was only used once to draw the ghost buffer in the editor, but
that was only because Ally didn't know you could just pass NULL in, cuz
the ghost buffer is the same size as the backbuffer.
RGBflip() does the exact same thing as getRGB(), now that all the
surface masks have been fixed. This axes RGBflip() and changes all
callers to use getRGB() instead. It is more readable that way.
By doing this, there is less copy-pasting. Additionally, it is now
easier to search for RGBf() - which is an ENTIRELY different function
than RGBflip() - now that the name of RGBf is no longer the first four
characters of some different, unrelated function. Previously I would've
had to do `rg 'RGBf[^\w]'` which was stupid and awful and I hated it.
Turns out, the r, g, and b arguments don't actually do anything!
There was a call to RGBf() in the function. RGBf() is just getRGB() but
first adds 128 and then divides by 3 to each of the color channels
beforehand. Unfortunately, RGBf() does not have any side effects, and
the function threw away the return value. Bravo.
This also reveals that the website images drawn in the credits in the
main menu are only recolored because of a stale `ct` set by the previous
graphics.bigprint(), and not because any color values were passed in to
drawimagecol()... What fun surprises the game has in store for me every
day.
This fixes a regression where entering playtesting while a track was
fading out (by exiting out of playtesting with a track playing and then
immediately entering back in with the level start music set) would
result in no music.
The cause is the game doing fades even though nothing is playing, which
puts it in a confusing state.
This wrapper function is for (a) future-proofing (b) proactive
prevention of future copy-pasting (c) to clarify that we never actually
halt music in the SDL_mixer sense, we only pause it, so to check if the
music is halted we actually check if the music is paused instead. This
is important because Mix_PlayingMusic() does not check if the music is
paused and Mix_PausedMusic() does not check if the music is halted.
When you're on the music changing screen in the editor, it plays the
current track. When you return, it stops playing the track. However, if
you press escape, it doesn't stop playing the track. This is because
pressing escape just returns to the previous menu without stopping
playing the track.
To fix this, I just added some kludge in the return menu function. This
is kinda super bad but it works for now and is just something to clean
up later. Maybe like each menu having exit callbacks or something, I
dunno.
This is kinda a regression, kinda sorta not. In 2.2 and previous,
pressing escape would just close the settings menu entirely, which also
bypassed the music fadeout. 2.3 made it so pressing escape doesn't
entirely close the settings menu, and just returns to the previous menu,
which fails in a different way. But the intended way is definitely to
select the return option and having the music fade out.
This function now properly deletes the Super Gravitron record, the Super
Gravitron rank, and the best game deaths. They were not being properly
reset previously, meaning you would have to go into your save file to
properly clean out your save data.
If the map size was less than 20x20, platv values outside the map would
end up being saved as 67372036.
This happens because SDL_memset() operates on the byte level, and not
the multi-byte level. So it takes only the lower 8 bits of 4 and repeats
it for each byte in each integer, creating 67372036.
This was done in 2.2 and previous probably to fix the fact that there
were multiple conflicting audio controls (the player wants to mute the
audio but the game wants to fade in the audio), but is now actively
harmful since 2.3, because muting the game while finishing the
completion prompt means the music will never come back in, even after
unmuting.
I also notice that when collecting a custom crewmate, the game checks
for the level's start music instead of if there's actually a current
song playing right now. I don't know why this was done, because it
would've been better to copy-paste the trinket collection logic here.
It's entirely possible for the audio to just be muted and never come
back if the level has no start music but plays a song by using a script.
Anyways, leaving it alone because it's quite possible that a level might
be intentionally designed around this, I can't really tell the
intentions of every level creator, and it's easy to work around (either
don't use custom crewmates, which every modern level basically does
nowadays, or just set the start music).
For some reason, when completing a custom level and fading to the menu,
the game attempts to fade the music in and also fade the music out at
the same time. This results in nothing happening at all, and in 2.2 and
previous, results in audio fading out from max volume while the game is
frozen on a black screen after the fadeout.
To avoid any potential badness, just remove these.
In the main game, if you press R during the trinket collection prompt
after collecting a trinket, AND you have never entered Comms Relay, and
you respawn in a different room, the trinket collection gamestate will
be interrupted, but you will still be left with the advance text prompt,
cutscene bars, and muted music.
The previous workaround to fix the music would be to mute and then
unmute the game, but due to the new music changes, this workaround
(which in and of itself is a bug) no longer works. Instead, the music
would have to be restarted by going into another zone on the map.
Having an advance text prompt outside of a cutscene results in the
player being unable to flip, but they can still move around left and
right.
Speedrunners previously used the no-Comms-Relay interrupting behavior to
skip certain trinket collection prompts entirely with a frame-perfect R
press, so I can't patch that out. Having an advance text prompt outside
of a cutscene is (ab)used in custom levels to intentionally prevent the
player from flipping, and furthermore, it's also used in credits warp
runs of the main game to increment the gamestate; so I cannot patch that
out. The ability to press R everywhere even during cutscenes was added
for good reason - to make it less likely that a softlock can happen - so
I don't want to revert it.
But I still think this is worth fixing because previously, the
punishment for missing the frame-perfect window late was simply not
skipping the trinket prompt (since the R-press would be ignored), but
now the punishment is basically having to reset because of the advance
text prompt.
I would usually handle this in gamestate 0, but awful custom levels
might want to intentionally interrupt the gamestate to do, I don't know,
something. No level does that so far, but I'd like to do the least
invasive thing.
So what I've done is made it so the effects of interruption are undone
if you press R and the gamestate is interrupted. This is handled in
mapclass::resetplayer().
