Ever since tilesheets got expanded, custom levels could use as many
tiles as they wanted, as long as it fit under the 32-bit signed integer
limit.
Until 6c85fae339 happened and they were
reduced to 32,767 tiles.
So I'm being generous again and changing the type of the contents array
(in mapclass and editorclass) back to int. This won't affect the
existing tilemaps of the main game, they'll still stay short arrays. But
it means level makers can use 2 billion tiles once again.
The main game used a set of copy-pasted code to set the music of each
area. There WAS some redundancy built-in, but only three rooms in each
direction from the entrance of a zone.
Given this, it's completely possible for players to mismatch the music
of the area and level. In fact, it's easy to do it even on accident,
especially since 2.3 now lets you quicksave and quit during cutscenes.
Just play a cutscene that has Pause music, then quicksave, quit, and
reload. Also some other accidental ways that I've forgotten about.
To fix this, I've done what mapclass has and made an areamap. Except for
music. This map is the map of the track number of every single room,
except for three special cases: -1 for do nothing and don't change music
(usually because multiple different tracks can be played in this room),
-2 for Tower music (needs to be track 2 or 9 depending on Flip Mode),
and -3 for the start of Space Station 2 (track 1 in time trials, track 4
otherwise).
I've thoroughly tested this areamap by playing through the game and
entering every single room. Additionally I've also thoroughly tested all
special cases (entering the Ship through the teleporter or main
entrance, using the Ship's jukebox, the Tower in Flip Mode and regular
mode, and the start of Space Station 2 in time trial and in regular
mode).
Closes#449.
Similar to disabling the elephant flashiness, at least one
photosensitive person has told me the flashy color animation makes their
eyes kind of hurt a little bit. Also it screws up the compression really
badly when they record (especially the green noisy tiles!).
The colors will still cycle, but the individual animations within each
color will be completely static.
This can happen if you select an option in a menu that (A) returns to
the previous menu and (B) saves settings. If the settings save fails,
this will create another menu on the same frame that cycles the tower BG
after it's already been cycled for that frame. Examples are the slowdown
and glitchrunner menus.
I could fix this by creating a new function that copy-pastes all of
Game::savestatsandsettings_menu() except for the map.nexttowercolour()
at the end. But that's copy-pasting code.
Instead what I've done is added a variable to signal if the color has
already been cycled this frame, so we don't cycle it again. This also
covers cases of possible double-cycling in the future as well.
Previously, turning glitchrunner mode on essentially locked you to
emulating 2.0, and turning it off just meant normal 2.3 behavior. But
what if you wanted 2.2 behavior instead? Well, that's what I had to ask
when a TAS of mine would desync in 2.3 because of the two-frame delay
fix (glitchrunner off), but would also desync because of 2.0 warp lines
(glitchrunner on).
What I've done is made it so there are three states to glitchrunner mode
now: 2.0 (previously just the "on" state), 2.2 (previously a state you
couldn't use), and "off". Furthermore, I made it an enum, so in case
future versions of the game patch out more glitches, we can add them to
the enum (and the only other thing we have to update is a lookup table
in GlitchrunnerMode.c). Also, 2.2 glitches exist in 2.0, so you'll want
to use GlitchrunnerMode_less_than_or_equal() to check glitchrunner
version.
There's a bit of inconsistency with how long each color lasts for during
final stretch. Initially, each color lasts for 40 frames, but when you
enter either of the minitowers, the color switches to lasting for 15
frames only. This is because a final_colorframe of 1 makes it go for 40
frames, but a final_colorframe of 2 makes it go for 15 frames - and
final_colorframe gets set to 2 whenever you enter a minitower.
This seems like an oversight because (1) final_colorframe doesn't affect
anything inside the minitower, (2) final_colorframe doesn't get saved to
the save file and always gets set to 1 if your save file has
finalstretch set to true, so saving and reloading will set the colors
back to 40 frames each, and (3) final_colorframe doesn't get set back to
1 when leaving the minitowers.
Tower backgrounds have a bypos and bscroll. bypos is just the y-position
of the background, and bscroll is the amount of pixels to scroll the
background by on each frame, which is used to scroll it (if it's not
being redrawn) and for linear interpolation.
For the tower background (and not the title background), bypos is
map.ypos / 2, and bscroll is (map.ypos - map.oldypos) / 2. However,
usually bscroll gets assigned at the same time bypos is incremented or
decremented, so you never see that calculation explicitly - except in
the previous commit, where I worked out the calculation because the
change in y-position isn't a known constant.
Having to do all these calculations every time introduces the
possibility of errors where you forget to do it, or you do it wrongly.
But that's not even the worst; you could cause a linear interpolation
glitch if you decide to overwrite bscroll without taking into account
map.oldypos and map.ypos.
So that's why I'm adding a function that automatically updates the tower
background, using the values of map.oldypos and map.ypos, that is used
every time map.ypos is assigned. That way, we have to write less code,
you can be sure that there's no place where we forget to do the
calculations (or at least it will be glaringly obvious) or we do it
wrongly, and it plays nicely with linear interpolation. This also
replaces every instance where the manual calculations are done with the
new function.
This makes it easier to add bounds checks to all accesses of
map.explored. Also, all manually-written existing bounds checks have
been removed, because they're going to go into the new getters and
setters.
The getter is mapclass::isexplored() and the setter is
mapclass::setexplored().
This replaces all raw ed.level accesses with new setter and getter
funcs, which makes it easier to add bounds checks later. And I've also
removed all the manually-written bounds checks, since they will go into
the new getter and setter.
To get the room properties of a specific room, you use
editorclass::getroomprop(), which returns a pointer to the room
properties - then you just read off of that pointer. To set a room
property, you use editorclass::setroom<PROP>(), where <PROP> is the name
of the property. These are maintained using X macros to avoid
copy-pasting. editorclass::getroompropidx() is a helper function and
shouldn't be used directly.
Previously, with the wrong loop order, this kludge needed to exist so
entities in finalmode didn't have wrong colors for 1 frame when entering
a room. But now the loop order has been fixed, and so this kludge is no
longer needed.
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.
This patch restores some 2.2 behavior, fixing a regression caused by the
refactor of properly using std::vectors.
In 2.2, the game allocated 200 items in obj.entities, but used a system
where each entity had an `active` attribute to signify if the entity
actually existed or not. When dealing with entities, you would have to
check this `active` flag, or else you'd be dealing with an entity that
didn't actually exist. (By the way, what I'm saying applies to blocks
and obj.blocks as well, except for some small differing details like the
game allocating 500 block slots versus obj.entities's 200.)
As a consequence, the game had to use a separate tracking variable,
obj.nentity, because obj.entities.size() would just report 200, instead
of the actual amount of entities. Needless to say, having to check for
`active` and use `obj.nentity` is a bit error-prone, and it's messier
than simply using the std::vector the way it was intended. Also, this
resulted in a hard limit of 200 entities, which custom level makers ran
into surprisingly quite often.
2.3 comes along, and removes the whole system. Now, std::vectors are
properly being used, and obj.entities.size() reports the actual number
of entities in the vector; you no longer have to check for `active` when
dealing with entities of any sort.
But there was one previous behavior of 2.2 that this system kind of
forgets about - namely, the ability to have holes in between entities.
You see, when an entity got disabled in 2.2 (which just meant turning
its `active` off), the indices of all other entities stayed the same;
the indice of the entity that got disabled stays there as a hole in the
array. But when an entity gets removed in 2.3 (previous to this patch),
the indices of every entity afterwards in the array get shifted down by
one. std::vector isn't really meant to be able to contain holes.
Do the indices of entities and blocks matter? Yes; they determine the
order in which entities and blocks get evaluated (the highest indice
gets evaluated first), and I had to fix some block evaluation order
stuff in previous PRs.
And in the case of entities, they matter hugely when using the
recently-discovered Arbitrary Entity Manipulation glitch (where crewmate
script commands are used on arbitrary entities by setting the `i`
attribute of `scriptclass` and passing invalid crewmate identifiers to
the commands). If you use Arbitrary Entity Manipulation after destroying
some entities, there is a chance that your script won't work between 2.2
and 2.3.
The indices also still determine the rendering order of entities
(highest indice gets drawn first, which means lowest indice gets drawn
in front of other entities). As an example: let's say we have the player
at 0, a gravity line at 1, and a checkpoint at 2; then we destroy the
gravity line and create a crewmate (let's do Violet).
If we're able to have holes, then after removing the gravity line, none
of the other indices shift. Then Violet will be created at indice 1, and
will be drawn in front of the checkpoint.
But if we can't have holes, then removing the gravity line results in
the indice of the checkpoint shifting down to indice 1. Then Violet is
created at indice 2, and gets drawn behind the checkpoint! This is a
clear illustration of changing the behavior that existed in 2.2.
However, I also don't want to go back to the `active` system of having
to check an attribute before operating on an entity. So... what do we
do to restore the holes?
Well, we don't need to have an `active` attribute, or modify any
existing code that operates on entities. Instead, we can just set the
attributes of the entities so that they naturally get ignored by
everything that comes into contact with it. For entities, we set their
invis to true, and their size, type, and rule to -1 (the game never uses
a size, type, or rule of -1 anywhere); for blocks, we set their type to
-1, and their width and height to 0.
obj.entities.size() will no longer necessarily equal the amount of
entities in the room; rather, it will be the amount of entity SLOTS that
have been allocated. But nothing that uses obj.entities.size() needs to
actually know the amount of entities; it's mostly used for iterating
over every entity in the vector.
Excess entity slots get cleaned up upon every call of
mapclass::gotoroom(), which will now deallocate entity slots starting
from the end until it hits a player, at which point it will switch to
disabling entity slots instead of removing them entirely.
The entclass::clear() and blockclass::clear() functions have been
restored because we need to call their initialization functions when
reusing a block/entity slot; it's possible to create an entity with an
invalid type number (it creates a glitchy Viridian), and without calling
the initialization function again, it would simply not create anything.
After this patch is applied, entity and block indices will be restored
to how they behaved in 2.2.
MSVC complains about these, doesn't seem like GCC does. These can be
safely removed because they're unreachable, and they always follow a
case-switch or similar that has a default case which this code is a
duplicate of anyway. (Unless it isn't, in which case all the better to
remove it, becausee otherwise it looks misleading or confusing to casual
glances at the code.)
This is a re-do of 942217f871 (#509), but
with a more conservative fix that only resets the player's newxp and
newyp when they respawn from a checkpoint or spawn in to the map.
Unlike the previous patch, if the player were to suddenly collide with a
conveyor or horizontally-moving platform during gameplay, their
y-position would revert back to the intended next y-position of the
previous frame. But this is the same behavior as before, I haven't ever
seen such a contrived situation come up, and this behavior is probably
more preferable for gameplay than actually going to the conveyor, so
it's fine.
I also decided to reset newxp here, and not just newyp, because while
resetting newyp seems to be enough, it's safer to also reset newxp (and
so future readers won't question why only newyp is reset but not newxp).
I tested this and it once again fixes the death loop issue from earlier,
while also still allowing for that Trench Warfare trick to be possible
(I tested it with the libTAS movie I mentioned in #606; it syncs fine).
There are no other known regressions resulting from this fix
(hopefully).
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)
These variables basically serve no purpose. map.customx and map.customy
are clearly never used. map.finalx and map.finaly, on the other hand,
are basically always game.roomx and game.roomy respectively if
map.finalmode is on, and if it's off, then they don't matter.
Also, there are some weird and redundant variable assignments going on
with these; most notably in map.gotoroom(), where rx/ry (local
variables) get assigned to finalx/finaly, then finalx/finaly get
assigned to game.roomx/game.roomy, then finalx/finaly get assigned to
rx/ry. If finalx/finaly made a difference, then there'd be no need to
assign finalx/finaly back to rx/ry. So it makes the code clearer to
remove these weird bits of code.
This fixes a bug where if you entered a tower before watching the
credits sequence, the credits sequence would have mismatched text and
background colors.
This bug happened because entering a tower modified the r/g/b attributes
of mapclass, and updated graphics.towerbg, without updating
graphics.titlebg too. Then gamecompleterender() uses the r/g/b
attributes of mapclass.
The solution is to put the r/g/b attributes on TowerBG instead. That
way, entering a tower will only modify the r/g/b attributes used to
render towers, and won't affect the r/g/b attributes used to render the
credits sequence.
Additionally, I also de-duplicated the case-switch that updated the
r/g/b attributes based off of the current colstate, because it got
copy-pasted twice, leading to three instances of one piece of code.
When I did #569, I forgot that taking out the code path that set the
player's invis to false meant that the player would still be invisible
upon loading back in to the game if they exited the game while
invisible. Taking out that code path also meant that if game.lifeseq was
nonzero, it wouldn't be reset properly, either. So this fixes those
things.
When I did #567, I didn't test it. And I should have tested it, because
it made the player invisible. This is because map.resetplayer() also
sets the invis attribute of the player to true as well, and I only undid
it setting game.lifeseq to 10.
So instead, I'll just add a flag to map.resetplayer() that by default
doesn't set game.lifeseq or the player's invis attribute. And I tested
it this time, and it works fine. I tested both respawning after death
and exiting to the menu and loading in the game again.
While I was working on #535, I noticed that all the call sites of
script.run() have the exact same code, namely:
if (script.running)
{
script.run();
}
I figured, why not move the script.running check into the function
itself? That way, we won't have to duplicate the check every single
time, and we don't risk forgetting to add the check and causing a bug
because of that.
The check was already duplicated once since 2.0 (it's used in both
GAMEMODE and TELEPORTERMODE), and with the fix of the two-frame delay in
2.3, it's now duplicated twice, leading to THREE instances of this check
in the code, when there should be only one.
This fixes a deltaframe glitch where the "- Press ENTER to Teleport -"
prompt would show up for a split second if you exited the game while the
prompt was fully faded in, and then re-entered it.
Now that tower, title, and horizontal/veritcal warp backgrounds all use
separate buffers, there's no longer any need to temporarily store
variables as a workaround for the buffers stepping on each other.
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.
Yet another set of temporary variables is on a global class when they
shouldn't be. These two are only used in tower background functions and
are never used anywhere else, so they're clearly temporaries.
This function was only used in assigments to mapclass::towercol. But
that variable is unused, and has been removed, so after removing that
variable, this one is unused, too.
The intent of #504 was to make it so oldxp/oldyp would never be messed
with for over-30-FPS stuff, but I forgot that I changed these
assignments in my over-30-FPS patch when I was doing #504. So these
assignments have been restored to the way they were in 2.2, and is
fixed now.
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 is a refactor that simply moves all temporary variables off of
entityclass, and makes it so they are no longer global variables. This
makes the resulting code easier to understand as it is less entangled
with global state.
These attributes were:
- colpoint1
- colpoint2
- tempx
- tempy
- tempw
- temph
- temp
- temp2
- tpx1
- tpy1
- tpx2
- tpy2
- temprect
- temprect2
- x (actually unused)
- dx
- dy
- dr
- px
- py
- linetemp
- activetrigger
- skipblocks
- skipdirblocks
Most of these attributes were assigned before any of the times they were
used, so it's easy to prove that ungloballing them won't change any
behaviors. However, dx, dy, dr, and skipblocks are a bit more tricky to
analyze. They relate to blocks, with dx, dy, and dr more specifically
relating to one-way tiles. So after some testing with the quirks of
one-way tiles, it seems that the jankiness of one-way tiles haven't
changed at all, either.
Unfortunately, the attribute k is clearly used without being assigned
beforehand, so I can't move it off of entityclass. It's the same story
with the attribute k that Graphics has, too.
The entity getters I'm referring to are entityclass::getscm(),
entityclass::getlineat(), entityclass::getcrewman(), and
entityclass::getcustomcrewman().
Even though the player should always exist, and the player should always
be indice 0, I wouldn't want to make that assumption. I've been wrong
before.
Also, these functions returning 0 lull you into a false sense of
security. If you assume that commands using these functions are fine,
you'll forget about the fact that `i` in those commands could be
potentially anything, given an invalid argument. In fact, it's possible
to index createactivityzone(), flipgravity(), and customposition()
out-of-bounds by setting `i` to anything! Well, WAS possible. I fixed it
so now they can't.
Furthermore, in the game.scmmoveme block in gamelogic(), obj.getplayer()
wasn't even checked, even though it's been checked in all other places.
I only caught it just now because I wanted to bounds-check all usages of
obj.getscm(), too, and that game.scmmove block also used obj.getscm()
without bounds-checking it as well.
It's better to do INBOUNDS_VEC(i, obj.entities) instead of 'i > -1'.
'i > -1' is used in cases like obj.getplayer(), which COULD return a
sentinel value of -1 and so correct code will have to check that value.
However, I am now of the opinion that INBOUNDS_VEC() should be used and
isn't unnecessary.
Consider the case of the face() script command: it's not enough to check
i > -1, you should read the routine carefully. Because if you look
closely, you'll see that it's not guaranteed that 'i' will be initialized
at all in that command. Indeed, if you call face() with invalid
arguments, it won't be. And so, 'i' could be something like 215, and
that would index out-of-bounds, and that wouldn't be good. Therefore,
it's better to have the full bounds check instead of checking only one
bounds. Many commands are like this, after some searching I can also
name position(), changemood(), changetile(), changegravity(), etc.
It also makes the code more explicit. Now you don't have to wonder what
-1 means or why it's being checked, you can just read the 'INBOUNDS' and
go "oh, that checks if it's actually inbounds or not".
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.
This makes it so the fix for crewmates' drawframes being wrong for
1-frame is fixed for all crewmates regardless of when they get created.
Sure, crewmates created in mapclass::loadlevel() have their drawframes
fixed there, but for crewmates that get created from scripting (such as
Violet when gotorooming to the Ship teleporter room after Space Station
1), this fix doesn't apply to them. But now it does, and Violet will no
longer be facing the wrong way for 1 frame when teleporting to the Ship
teleporter room in the Space Station 1 Level Complete cutscene.
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.
There's a lot of places where the INBOUNDS() check is spelled out
instead of using the macro, before the macro was introduced. Also the
macro should probably be renamed INBOUNDS_VEC() to be consistent.
Okay, so basically here's the include layout that this game now
consistently uses:
[The "main" header file, if any (e.g. Graphics.h for Graphics.cpp)]
[blank line]
[All system includes, such as tinyxml2/physfs/utfcpp/SDL]
[blank line]
[All project includes, such as Game.h/Entity.h/etc.]
And if applicable, another blank line, and then some special-case
include screwy stuff (take a look at editor.cpp or FileSystemUtils.cpp,
for example, they have ifdefs and defines with their includes).
Including a header file inside another header file means a bunch of
files are going to be unnecessarily recompiled whenever that inner
header file is changed. So I minimized the amount of header files
included in a header file, and only included the ones that were
necessary (system includes don't count, I'm only talking about includes
from within this project). Then the includes are only in the .cpp files
themselves.
This also minimizes problems such as a NO_CUSTOM_LEVELS build failing
because some file depended on an include that got included in editor.h,
which is another benefit of removing unnecessary includes from header
files.
This removes around megabyte from the binary, so a stripped -Og binary
went from 4.0 megabytes to 2.9 megabytes, and an unstripped -O0 binary
went from 8.1 megabytes to 7.1 megabytes, which means I can now finally
upload an unstripped -O0 binary to Discord without having to give money
to Discord for their dumb Nitro thing or whatever.
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.
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.
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.
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.
This patch is very kludge-y, but at least it fixes a semi-noticeable
visual issue in custom levels that use internal scripts to spawn
entities when loading a room.
Basically, the problem here is that when the game checks for script
boxes and sets newscript, newscript has already been processed for that
frame, and when the game does load a script, script.run() has already
been processed for that frame.
That issue can be fixed, but it turns out that due to my over-30-FPS
game loop changes, there's now ANOTHER visible frame of delay between
room load and entity creation, because the render function gets called
in between the script being loaded at the end of gamelogic() and the
script actually getting run.
So... I have to temporary move script.run() to the end of gamelogic()
(in map.twoframedelayfix()), and make sure it doesn't get run next
frame, because double-evaluations are bad. To do that, I have to
introduce the kludge variable script.dontrunnextframe, which does
exactly as it says.
And with all that work, the two-frame (now three-frame) delay is fixed.
It's annoying for casuals to have to close the game if they manage to
get themselves to turn into VVVVVV-Man, but it's amusing enough to
glitchrunners that they mess about with VVVVVV-Man in the main game,
clipping through walls everywhere (well, they call it Big Viridian, but
still).
Ironically enough, resetting more variables in script.hardreset() makes
the glitchy fadeout system even more glitchy. Resetting map.towermode,
for example, makes it so that if you're in towers when you quit to the
menu, script.hardreset() makes it so that the game thinks you're no
longer inbounds (because it no longer thinks you're in a tower and thus
considers coordinates in the space of 40x30 tiles to be inbounds instead
of 40x700 or 40x100 tiles to be inbounds), calls map.gotoroom(), which
resets the gamestate to 0. So if we're using the old system, it's better
to reset only as much as needed.
And furthermore, we shouldn't be relying on script.hardreset() to
initialize variables for us. That should be done at the class
constructor level. So I've gone ahead and initialized the variables in
class constructors, too.
Looks like mapclass::changefinalcol() is called whenever you enter a
room in Outside Dimension VVVVVV.
mapclass::changefinalcol() changes the tile, but it doesn't update their
drawframe. So after that function is called, update their drawframe.
If you update their drawframe while inside that function, then when the
platform actually cycles color it'll cycle backwards quickly sometimes,
which is not ideal.
When I loaded the room where Vitellary is in Space Station 2, I saw this
1-frame glitch happen despite my previous efforts to prevent it. So now
it's fixed.
By "hidden names", I'm referring to "Dimension VVVVVV" and "The Ship"
popping up on the quit/pause/teleporter screens, even though those rooms
don't have any roomnames.
Apparently my commit to fix roomname re-draw bleed on the
quit/pause/teleporter screens exposed yet another hardreset()-caused
bug. The issue here is that since hardreset() sets game.roomx and
game.roomy to 0, map.area() will no longer work properly, and since the
hidden roomname check is based on map.area(), it will no longer display
"Dimension VVVVVV" or "The Ship" once you press ACTION to quit. It used
to do this due to the re-draw bleed, but now it doesn't.
I saw that roomnames didn't get reset in hardreset(), so the solution
here is to re-factor hidden names to be an actual variable, instead of
being implicit. map.hiddenname is a variable that's set in
mapclass::loadlevel(), and if isn't empty, it will be drawn on the
quit/pause/teleporter screens. That way it will still display "Dimension
VVVVVV" and "The Ship" when you press ACTION to quit to the menu.
EDIT: Since PR #230 got merged, this commit is no longer strictly
necessary, but it's still good to refactor hidden names like this.
This was especially noticeable in slowmode, where after going to an
adjacent room, it would look like they stopped for a split second. This
commit makes it so they smoothly continue their journey after switching
rooms.
The integer cast is to round off any fractional part of the velocity so
that they don't make a difference and result in the oldxp/oldyp being
one pixel off. Especially since the player's y-velocity fluctuates while
standing unflipped on the floor.
Incidentally enough, this seems to only have been a problem with screen
transitions for some reason. No other uses of gotoroom() (such as the
one where gotoroom() is called every other frame, or every frame) seem
to have resulted in this "pausing" behavior, or at least a reversion
back to 30 FPS movement. I don't know why.
Just a small thing, but if you teleported out of a tower with the
top/bottom screen spikes being onscreen (by dying, for example), they
would retract once you went back in to a tower. Small little thing, but
it's a good thing to polish.
By "frame" here I'm referring to the fixed-timestep, not a visual
delta-timestep.
Anyway, the problem is because crewmates' drawframes wait for
entityclass::animateentities() to be called in gamelogic(). In the
old, 30-FPS-only system, this entityclass::animateentities() would be
called in gamerender(), before any actual rendering would take place.
However, I've had to move it out of gamerender() because otherwise
entities would animate too fast. As a result, since gamerender() could
be called in between the entity creation and gamelogic(), a
less-than-1-frame visual glitch could happen.
The solution is to set the entity's drawframe as well when fixing facing
the player in Map.cpp.
Looks like duplicate player entities persisting across rooms is a
semi-useful feature used by some levels. Still, though, it's a bit of a
nuisance to have duplicate player entities persisting across game
sessions. And levels can't rely on this persistence anyway, anyone could
just close the game and re-open it to get rid of the duplicate entities
regardless.
The only class that actually needs its i/j/k kept is scriptclass,
because some custom levels rely on it for creating custom activity
zones. So I haven't touched that.
Other than that, there's no chance that anything important relies on
i/j/k in any other class. For that to be the case, it would have to use
i/j/k without initializing it beforehand, and that can simply be
detected by removing the attribute from the header file and seeing where
the compiler complains. And the compiler complains only about cases
where it's initialized first. (Note that due to this check, I *haven't*
removed Graphics's `m` as it precisely does exactly this, using it
without initializing it first.)
Interestingly enough, otherlevelclass and towerclass have unused i/k
variables for whatever reason.
obj.getplayer() can return -1, which can cause out-of-bounds indexing of
obj.entities, which is really bad. This was by far the most changes, as
obj.getplayer() is the most used entity-getting function that returns
-1, as well as the most-used function whose sentinel value goes
unchecked.
To deal with the usage of obj.getplayer() in mapclass::warpto(), I just
added general bounds checks inside that function instead of changing all
the callers.
When the game loads a room in a custom level, previously it would load
the tilemap of that room into ed.swapmap, and then mapclass::loadlevel()
would manually go through each element in ed.swapmap to set each tile in
`contents`. Why do that, when you can just return the vector from
editorclass::loadlevel() and set it directly? ed.swapmap is really
unnecessary.
Instead of using somewhat-obtuse for-loops to initialize or reset these
vectors, it takes up less lines of code and is clearer if we use
std::vector::resize() and std::vector::clear() instead.
