Since mainmenu is only ever used in Input.cpp, I might as well make it
clearer by moving it into a static global variable in Input.cpp. (The
same applies to fadetolab/fadetomenu, but I didn't think much about
those at the time... that'll be a refactor for later.)
While I've decoupled fademode from gamemode starting, being faded out on
the title screen results in a black screen and you being unable to make
any input. So we'll need to store the current fademode in a temporary
variable when going to in-game options, then put it back when we return
to the pause menu. Yes, you can turn on glitchrunner mode during the
in-game options, and then immediately return to the pause menu to
instantly go back to the title screen; this is intended.
Due to frame ordering, putting the fademode back needs to be deferred to
the end of the frame to prevent a 1-frame flicker.
It's actually sufficient enough to do this temporary fademode storage to
fix the whole thing, but I also decided to decouple fademode and
gamemode starting just to be sure.
Assuming glitchrunner mode is off, if you open the pause menu while
fully faded-out and then go to Graphic Options or Game Options, then the
'mode' that you selected previously will kick in again and you'll be
suddenly warped back.
So if you previously started a new game in the main game (mode 0, also
the selected mode if you do this from command-line playtesting), and
then open the pause menu and go to in-game options, then you'll suddenly
go back to starting a new game again. If you had started a custom level,
doing this will warp you back to the start of the level again.
The problem is simple - when the title screen is fully faded out, it
calls startgamemode(). So the solution is simple as well - just decouple
the fademode from calling startgamemode(), and use a different variable
to know when to actually call startgamemode().
Custom levels can have warp lines. If you have a warp line and a warping
background in the same room, the warp line takes precedence over the
warp background.
However, whenever you enter a room with a warp line and warp background,
any entities on the warping edges will be drawn with screenwrapping for
one frame, even though they never wrapped at all.
This is due to frame ordering: when the warp line gets created,
obj.customwarpmode gets set to true. Then when the screen edges and
warping logic gets ran, the very first thing that gets checked is this
exact variable, and map.warpx/map.warpy get set appropriately - so
there's no way the entity could legitimately screenwrap.
However, that happens in gamelogic(). gamelogic() is also the one
responsible for creating entities upon room load, but that happens after
the obj.customwarpmode check - so when the game gets around to rendering
in gamerender(), it sees that map.warpx or map.warpy is on, and draws
the screenwrapping, even though map.warpx/map.warpy aren't really on at
all. Only when gamelogic() is called in the frame later do map.warpx and
map.warpy finally get set to false.
To fix this, just set map.warpx and map.warpy to false when creating
warp lines.
I just spotted this one - if vy isn't bounds-checked, this causes bogus
input from the createentity() script command to commit Undefined
Behavior. Should've spotted this one when I was adding bounds checks to
the rest of createentity() earlier, but at least it's fixed now.
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().
It is no longer possible to cause Undefined Behavior via accessing
out-of-bounds room properties.
What happens instead is - if you attempt to fetch an out-of-bounds room
property, you get a "blank" room property that just has all of the
defaults, plus its tileset is 1 because all tilesets that are nonzero
use tiles2.png, and it closely emulates the previous behavior where it
was some bogus value but definitely not zero. Its Direct Mode is also 1,
because the tiles contained within it are just mishmashed repeats of
existing tiles on the map, and we shouldn't autotile that.
The roomname also gets cleared in case the user attempts to set the room
name of an out-of-bounds room property.
If you attempt to set the property of an out-of-bounds room property,
then nothing happens.
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.
This removes all traces of Undefined Behavior from getting and placing
tiles.
This mimics the previous behavior (2.2 and below) as reasonably as
possible. `vmult` was previously a vector, there was a bunch of unused
space directly after the end of the usable space of the vector, which
was all filled with zeroes. The same goes for `contents`, having
previously been a vector, and so having a bunch of zeroes immediately
following the end of the in-bounds space. That's why both are 0 if you
index them out of bounds.
This makes it easier to add bounds checks to all accesses of
ed.contents.
To do this, I've added editorclass::gettile(), editorclass::settile(),
and editorclass::getabstile() (with a helper function of
editorclass::gettileidx() that really shouldn't be used directly), and
replaced all raw accesses of ed.contents with those functions
appropriately.
This also makes the code more readable, as a side effect.
The existing bounds checks were correct sometimes but other times were
not.
The bounds check for 2x2 and 2x1 sprites only covered the top-left
sprite drawn; the other sprites could still be out of bounds. But if the
top-left sprite was out of bounds, then none of the other sprites
wouldn't be drawn - although it ought to be that the other sprites still
get attempted to be drawn. So I've updated the bounds checks
accordingly, and now an out of bounds top-left sprite won't prevent the
drawing of the rest of the sprites.
Similarly, if the sprite of a Gravitron square was out of bounds, that
would prevent its indicators from being drawn. But the indicators
weren't being bounds-checked either (2.3 lets you have less than 1200
tiles in a given tilesheet). So the bounds check has been moved to only
cover the drawframe and the indicator indexes accordingly, and an out of
bounds sprite won't prevent attempting to draw the indicators.
It is possible for any of the QueryIntAttribute()s to fail, most
commonly if the attributes don't exist. If that happens, then that part
of the temporary edentity won't be initialized, and we'll end up having
a partially-uninitialized edentity - then doing much of anything with it
will result in undefined behavior.
To fix this, just initialize the temporary edentity.
If an XML tag doesn't contain anything inside, pText will be NULL. If
that happens without being checked, then NULL will be passed to
SDL_strcmp(). SDL_strcmp() will either call libc strcmp() or use its own
implementation; both implementations will still dereference the NULL
without checking it.
This is undefined behavior, so I'm fixing it. The solution is to do what
is done with all other XML parsing functions, and to make sure pText
gets set to a safe empty string (which is just a pointer to a null
terminator) if it happens to be NULL.
PR #279 added game.gametimer solely for the editor ghosts feature. It
seems that whoever originally wrote it (Leo for the now-dead VVVVVV:
Community Edition, I believe) forgot that the game already had its own
timer, that they could use.
The game timer does increment on unfocus pause (whereas this doesn't),
but that's a separate issue, and it ought to not do that.
So #434 didn't end up solving the deltaframe flashing fully, only
reduced the chances that it could happen.
I've had the Level Complete image flash a few times when the Game Saved
text box pops up. This seems to be because the Level Complete image is
based off of the text box being at y-position 12, and the Game Saved
text box is also at y-position 12. Level Complete only gets drawn if the
text box additionally has a red channel value of 165, and the Game Saved
text box has a red channel value of 174. However, there is a check that
the text box be fully opaque first before drawing special images. So
what went wrong?
Well, after thinking about it for a while, I realized that even though
there is indeed an opaqueness check, the alpha of the text box updates
BEFORE it gets drawn. And during the deltaframes immediately after it
gets updated, the text box is considered fully opaque. It's completely
possible for the linear interpolation to end up with a red channel value
of 165 during these deltaframes, while the text box is opaque as well.
As always, it helps if you have a high refresh rate, and run the game
under 40% slowdown.
Anyways, so what's the final fix for this issue? Well, use the text box
'target' RGB values instead - its tr/tg/tb attributes instead of its
r/g/b attributes. They are not subject to interpolation and so are
completely reliable. The opaqueness check should still be kept, though,
because the target values don't account for opaqueness. And this way, we
get no more deltaframe flashes during text box fades.
An even better fix would be to not use magic RGB values to draw special
images... but that'd be something to do later.
Clang warns on this. This doesn't fix anything but it does ensure that
whoever's reading it won't be focused as to whether or not omitting the
second set of braces is legal or not.
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.
In 2.2, at render time, the game rendered screenshakes and flashes if
their timers were above 0, and then decremented them afterwards. The
game would also update the analogue filter right before rendering it,
too.
In 2.3, this was changed so the flash and screenshake timers were
unified, and also done at the end of the frame - right before rendering
happened. This resulted in 1-frame flashes and screenshakes not
rendering at all. The other changes in this patchset don't fix this
either. The analogue filter was also in the wrong order, but that is
less of an issue than flashes and screenshakes.
So, what I've done is made the flash and screenshake timers update right
before the loop switches over to rendering, and only decrements them
when we switch back to fixed functions (after rendering). The analogue
filter is also updated right before rendering as well. This restores
1-frame flashes and screenshakes, as well as restores the correct order
of analogue filter updates.
This reintroduces 2-frame edge-flipping after the 1-frame input delay
got removed. This is because along with processing input and moving
Viridian, logical onground/onroof assignments need to processed in the
same between-render sequence as well - otherwise Viridian only gets 1
frame of edge-flipping due to frame ordering.
I will need to separate these into two different variables because I
will need to move logical onground/onroof assignments to the start of
gamelogic() - if I kept them together, however, that would change the
visuals of onground/onroof, which I want to keep consistent with 2.2.
To do this, GAMEMODE input needs to be processed, and Viridian needs to
be moved, in the same sequence between render frames. So just move
gameinput to after gamerender. Yes, this is not 2.2 order, but gameinput
only handles player input and nothing else - plus a 1-frame input delay
feels really awful to play with in over-30-mode.
In order to re-remove the 1-frame input delay, we will have to poll
input right after rendering a frame - in other words, just before an
input function gets called.
To do this, I've added a new function enum type - Func_input - that is
the same as a fixed function, but before its function gets called,
key.Poll() gets called. And all input functions have been updated to use
this enum accordingly.
This once again fixes the facing directions of crewmates upon room load,
except now it covers more cases.
So, here is the saga so far:
- 2.0 (presumably) to 2.2: crewmate direction fix is special-cased at
the end of mapclass::loadlevel(). Only covers crewmates created during
the room load, does not cover crewmates created from scripts, only
covers state 18 of crewmates.
- 2.3 currently (after #220): crewmate direction fix is moved to
entityclass::createentity(), which covers every avenue of crewmate
creation (including from scripts), but still only covers state 18.
- This commit: crewmate direction fix now covers every possible state of
the crewmate, also does not copy-paste any code.
What I've done instead is to make it so createentity() will immediately
call updateentities() on the pushed-back entity. This is kludge-y, but
is completely okay to do, because unlike other entities, crewmate
entities never change their state or have any side-effects from
double-evaluation, meaning calling updateentities() on them is
idempotent and it's okay to call their updateentities() more than once.
This does have the slight danger that if the states of crewmates were to
change in the future to no longer be idempotent, this would end up
resulting in a somewhat hard-to-track-down double-evaluation bug, but
it's worth taking that risk.
This fix is not applied to entity 14 (the supercrewmate) because it is
possible that calling updateentities() on it will immediately remove the
entity, which is not idempotent (it's changing the state of something
outside the object). Supercrewmates are a bit difficult to work with
outside of the main game anyways, and if you spawn them you could
probably just use the changedir() script command to fix their direction,
so I'm not inclined to fix this for them anyway.
This copy-pasted code only existed because the previous loop order was
incorrect and rendered entities before they would get properly updated
by the fixed render function. Now, the fixed render function is
guaranteed to be called before the render function, so we can rely on
that to update the drawframe and realcol of entities instead of
duplicating the code ourselves in createentity().
The drawframe assignment is still kept to fix the case where dying while
completestop is active (i.e. during a trinket or crewmate rescue
cutscene) and respawning in a different room won't turn everything into
Viridian sprites.
The background would change for 1 frame before sending you back to the
pause menu or editor settings. The map.nexttowercolour() call needs to
be deferred until the end of the frame.
The new loop order introduces a glitch where the menu would display
whichever menu was saved to kludge_ingametemp for 1 frame right as the
user returned to the pause menu. This happened because the
game.returntomenu() happens in titleinput(), which comes before
titlerender(). To fix this, we just need to defer it to the end of the
frame.
game.shouldreturntoeditor was added to fix a frame ordering issue that
was causing a bug where if you started playtesting in a room with a
horizontal/vertical warp background, and exited playtesting in a
different room that also had a horizontal/vertical warp background and
which was different, then the background of the room you exited in would
slowly scroll offscreen, when you re-entered the editor, instead of the
background consisting entirely of the actual background of the room.
Namely, the issue was that the game would render one more frame of
GAMEMODE after graphics.backgrounddrawn got set to false, and re-set it
to true, thus negating the background redraw, so the editor background
would be incorrect.
With defer callbacks, we can now just use a couple lines of code,
instead of having to add an extra kludge variable and putting handling
for it all over the code.
Sometimes, there needs to be code that gets ran at the end of the game
loop, otherwise rendering issues might occur. Currently, we do this by
special-casing each deferred routine (e.g. shouldreturntoeditor), but it
would be better if we could generalize this deference instead.
Deferred callbacks can be added using the DEFER_CALLBACK macro. It takes
in one argument, which is the name of a function, and that function must
be a void function that takes in no arguments. Also, due to annoying C++
quirks, void functions taking no arguments cannot be attributes of
objects (because they have an implicit `this` parameter), so it's
recommended to create each callback separately before using the
DEFER_CALLBACK macro.
Otherwise, the player would appear to "zip" during the deltaframes
between their previous position and their new position. This did not
happen in the previous game loop order and only happens in the new one.
Previously, before the game loop order got fixed, going to the in-game
settings would switch over to the new render function too early, causing
a deltaframe glitch that had to be fixed. But now, the render function
only gets switched when the current gamestate's function list gets
finished executing, so the game won't suddenly switch to titlerender()
in the middle of the ACTION press to the in-game settings screen.
As a consequence, titleupdatetextcol() no longer needs to be exported to
Input.cpp.
The previous location of this loop was placed there because it happened
just after the end of the render function. Now that the loop order is
fixed, the first thing that happens after the render function is the
start of gamelogic(), so this loop should go there now, else entity
positions won't be interpolated.
Also it now preincrements instead of postincrements because I like
preincrements.
Okay, so the reason why all render functions were moved to the end of
the frame in #220 is because it's simpler to call two fixed functions
and then a delta function instead of one fixed function, then a delta
function, and then another fixed function.
This is because fixed functions need special handling inside
deltaloop(), and you can't simply duplicate this handling after calling
a delta function. Oh, and to make matters worse, it's not always
fixed-delta-fixed, sometimes (like in MAPMODE and TELEPORTERMODE) it's
delta-fixed-fixed, so we'd need to handle that somehow too.
The solution here is to generalize the game loop and factor out each
function, instead of hardcoding it. Instead of having hardcoded
case-switches directly in the loop, I made a function that returns an
array of functions for a given gamestate, along with the number of
functions, then the game loop processes it accordingly. In fixedloop(),
it iterates over the array and executes each function until it reaches a
delta function, at which point it stops. And when it reaches the end of
the array, it goes back to the start of the array.
But anyway, if it gets to a delta function, it'll stop the loop and
finish fixedloop(). Then deltaloop() will call the delta function. And
then on the next frame, the function index will be incremented again, so
fixedloop() will call the fixed functions again.
Actually, the previous game loop was actually made up of one big loop,
with a gamestate function loop nested inside it, flanked with code that
ran at the start and end of the "big loop". This would be easy to handle
with one loop (just include the beginning and end functions with the
gamestate functions in the array), except that the gamestate functions
could suddenly be swapped out with unfocused functions (the ones that
run when you unfocus the window) at any time (well, on frame boundaries,
since key.isActive only got checked once, guarding the entire "inner
loop" - and I made sure that changing key.isActive wouldn't immediately
apply, just like the previous game loop order) - so I had to add yet
another layer of indirection, where the gamestate functions could
immediately be swapped out with the unfocused functions (while still
running the beginning and end code, because that was how the previous
loop order worked, after all).
This also fixes a regression that the game loop that #220 introduced
had, where if the fixed functions switched the gamestate, the game would
prematurely start rendering the gamestate function of the new gamestate
in the deltaframes, which was a source of some deltaframe glitches. But
fixing this is likely to just as well cause deltaframe glitches, so it'd
be better to fix this along with fixing the loop order, and only have
one round of QA to do in the end, instead of doing one round after each
change separately.
Fixes #464... but this isn't the end of the patchset. There are bugs
that need to be fixed, and kludges that need to be reverted.
Y-position 180 would be the position of the Level Complete and Game
Complete special text boxes in Flip Mode. However, since the y-position
of flipme text boxes actually no longer change (because we have to
accomodate changing Flip Mode on-the-fly), these text boxes will never
actually be y-position 180 - so we should remove these checks for
clarity.
A-ha! I've spotted an inconsistency! The normal trinket collection text
boxes (gamestate 1000-1003) is aware of Flip Mode, and will position
themselves accordingly to read the correct way in Flip Mode. However,
foundtrinket() doesn't do this.
Well, now it does.
This is why the text box attribute was named flipme, after all.
You may have noticed that the flipme command inverts textflipme instead
of simply setting it to true. Well, that's because it should be the same
as the previous behavior, which was essentially to invert it instead of
setting it to true - i.e. calling flipme twice would keep the original
text box position in Flip Mode, which means it would be upside-down
(this is a lot of flipping to keep track of...) - because flipme added
to texty in-place instead of simply assigning to it. (It did the
calculation incorrectly in 2.2 and previous, but I digress.)
Similarly, textflipme is not reset in hardreset(), because none of the
other script text box variables are reset either.
This ensures that if the player decides to toggle Flip Mode while one of
these text boxes is up, they won't be oriented improperly. Additionally,
it also de-duplicates a bunch of Flip Mode check code, which is also a
win.
createtextboxreal() is the same as createtextbox(), but with a flipme
parameter added to create text boxes that have their flipme attribute
set to true. createtextbox() just calls createtextboxreal() with flipme
set to false, and createtextboxflipme() just calls createtextboxreal()
with flipme set to true; this is because I do not want to use C++
function overloading.
Instead of calculating the y-position of the text box when it's created,
we will store a flag that says whether or not the text box should be
flipped in Flip Mode (and thus stay right-side-up), and when it comes
time to draw the text box, we will check Flip Mode and calculate the
position then.
Instead of duplicating the same variables over and over again,
Graphics::drawgui() can just make its own SDL_Rect. It's not that hard.
As far as I can tell, textrect was always being properly kept up to date
by the time Graphics::drawgui() got around to rendering
(textboxclass::resize() keeps being called a LOT), so this shouldn't be
a noticeable change from the user perspective.
The "Game Saved" text box, along with its associated telesave() call,
exists in both Game.cpp and Script.cpp, so one of them is the copy-paste
of the other. Unfortunately this copy-paste resulted in an inconsistency
where both of them don't check for the same things when deciding whether
or not the telesave should actually happen (this is why you don't
copy-paste, kids... it's scary!).
Either way, de-duplicating this now is less work for me later.
Every Level Complete sequence is the same copy-pasted thing, but with
minor changes. To make my work easier, I'm de-duplicating them so I have
less text boxes to change later, and less grind to grind.
These default arguments are never used anywhere. And if they were used
anywhere, it'd be better to explicitly say 255,255,255 than make readers
have to look at the header file to see what these default to. Also, this
creates four different overloads of createtextbox(), instead of only
two - but we ought to not be using function overloading anyway.
These commented-out code blocks just get in the way of clarity when I'm
refactoring flipped textboxes created in the gamestate system. So I'm
getting rid of them. If we need them back, we always have Git history.
Since the only difference is the y-positions, I've decided to remove the
copy-pasted code. A better solution would be to have a function that
draws multiline text and handles it accordingly in Flip Mode, but that
could be done later.
The only difference between Flip Mode and normal mode is the y-position
and sprite used to draw the crewmates. Everything else is the same, so
I've removed the copy-pasted portion.
The diff might look a bit ugly due to the unindentation.
Since the only difference in Flip Mode is the positiveness/negativeness
of the iterator variable, plus the starting y-offset, I've removed the
copy-pasted code and did this instead.
The diff might look a bit ugly due to the unindentation.
Like cutscene bars, I've added Graphics::setfade(), to ensure that no
deltaframe rendering glitches happen due to oldfadeamount not being
updated properly.
And indeed, this fixes a deltaframe rendering glitch that happens if you
return to the editor from playtesting on a faded-out screen, then fade
out again (by either re-entering playtesting and then cause a fadeout to
happen again, or by quitting from the editor afterwards). The same
glitch also happens outside of in-editor playtesting if you exit to the
menu while the screen is faded out.
To do this, I've added Graphics::setbars(), to make sure
oldcutscenebarspos always gets assigned when cutscenebarspos is. This
fixes potential deltaframe rendering issues if these two mismatch.
While working on #535, I noticed that editormenuactionpress() still
didn't do the explicit void declaration. Then I ran `rg 'void.*\(\)'`
and found three other functions that I somehow missed in #628. Whoops.
Well, now they no longer are missed.
This is a small quality-of-life tweak that makes it so if you're in the
middle of editing a level, you don't have to save the level, exit to the
menu, change whatever setting you wanted, re-enter the editor, and type
in the level name, just to change one setting. This is the same as
adding Graphic Options and Game Options to the in-game pause menu,
except for the editor, too.
To do this, I'm reusing Game::returntopausemenu() (because all of its
callers are the same callers for returning to editor settings) and
renamed it to returntoingame(), then added a variable named
ingame_editormode to Game. When we're in the options menus but still in
the editor, BOTH ingame_titlemode and ingame_editormode will be true.
This is a small quality-of-life thing that makes it so you don't have to
move your menu selection all the way over to the "return" button in
order to return to the previous menu. You can just press Escape instead
to return to the previous menu. The previous behavior of pressing Escape
was to bring up the 'confirm quit' menu, or if you were in an options
menu in-game, return to the pause menu.
If you're on the main menu (and thus don't have any previous menu) and
press Escape, the game will instead bring up the 'confirm quit' menu.
For consistency, the "quit game" option on the main menu will also bring
up the 'confirm quit' menu as well, instead of immediately closing the
game.
Pressing the controller button mapped to Escape will also work as well.
The only menus that don't have return buttons are the 'countdown' menus
- so the game will not let you press Escape if there's a menu countdown
happening.
Now that pressing Escape in the 'continue' menu will just bring you back
to the 'play' menu, there's no need to specifically put
map.nexttowercolour() first when canceling the 'confirm quit' menu.
As part of my work in #535, I've noticed that 2.3 currently with 2.2
loop order doesn't have interpolated cutscene bars. This is because
cutscene bars in 2.3 get updated at the start of the frame, which
interpolates them correctly until the render functions are put in their
proper place.
There is, however, a somewhat bigger issue, outside the scope of #535,
where cutscene bars always get updated regardless of which gamemode you
are in. Previously in 2.2 and previous, cutscene bars only got updated
in GAMEMODE and TELEPORTERMODE; sometime during 2.3, the cutscene bars
timer got pulled out of all the individual game modes and moved to the
very start of the loop. (I was probably the one who did this change;
I've been caught in a trap of my own devising.)
Thus, going to MAPMODE during the cutscene bars animation doesn't keep
their position paused like it would in 2.2. This is also categorically a
more-than-visual change, since the untilbars() script command depends
on the cutscene bars timer. I see no reason for the cutscene bars to
behave differently in this way than 2.2; #535 would also end up doing
the same fix more-or-less anyway.
Since TELEPORTERMODE currently uses the same renderfixed function as
MAPMODE, I've had to add a teleporterrenderfixed() that just calls
maprenderfixed(), but also does the cutscene bars timer.
As a partial fix for #618, adding the SDL2 version number to the README
will clarify that you need a specific version of SDL2 in order to
compile (and run) the current version of the game (2.3 at the time of
writing); in the future, the SDL2 dependency will be upgraded with each
SDL release.
This is to avoid error messages that complain about missing symbols like
SDL_zeroa() (added in SDL 2.0.14) not being present at the time of
compilation.
Closes#626.
This moves the responsibility of toggling fullscreen when any of the
three toggle fullscreen keybinds are pressed (F11, Alt+Enter, Alt+F)
directly into key.Poll() itself, and not its caller (which is main() -
more specifically, fixedloop()). Furthermore, the fullscreen toggle
itself has been moved to a separate function that key.Poll() just calls,
to prevent cluttering key.Poll() with more business logic (the function
is already quite big enough as it is).
As part of my work in re-removing the 1-frame input delay in #535, I'm
moving the callsite of key.Poll() around, and I don't want to have to
lug this block of code around with it. I'd rather refactor it upfront
than touch any more lines than necessary in that PR.
This fixes a bug where the resumemusic() script command would always
play MMMMMM track 15 (or, if you're using PPPPPP, just not work). This
is because musicclass::haltdasmusik() assigns resumesong AFTER calling
Mix_HaltMusic(), but the songend() callback fires before the resumesong
assignment, meaning resumesong gets set to -1 instead of whatever
currentsong was previously.
To fix this, just move the assignment into the callback itself (I don't
know why this wasn't done before). I could have moved it to before the
Mix_HaltMusic() call, but moving it into the callback itself fixes it
for all cases of the music stopping (such as when the music fades out).
This avoids the room name awkwardly moving back up if the cursor is at
the bottom of the screen in a room with a room name, then the user
switches to a room without a room name, then moves the cursor away from
the bottom, then switches to a named room - even though the cursor was
already away from the bottom of the screen.
Conversely, if the user moves their cursor to the bottom of the screen
in an unnamed room, then switches into a named room, the room name will
already have been hidden and they won't need to wait for it to hide.
This fixes the drawer suddenly popping up only to disappear, if the user
leaves a Direct Mode room into a non-Direct Mode room when the drawer
hasn't closed all the way, and then re-enters a Direct Mode room.
Gravity line correction no longer happens on every deltaframe. This
means less CPU time is wasted. Although, there's probably no need to
correct gravity lines on every single frame... hm... well, that's an
optimization for later (there's plenty of other stuff to cache, like
minimap drawing or editor foreground drawing).
Since it only ever gets assigned from FILESYSTEM_getUserSaveDirectory(),
and that function returns a C string, and the variable is only ever read
from again, this doesn't need to be an std::string.
In #553, when Dav999 added error messages to settings menus if the game
was unable to successfully save the changed settings, he seemed to have
forgotten the PPPPPP/MMMMMM toggle option.
However, I can fully blame him for only that miss. The Flip Mode options
were using game.savemystats (which was removed in #591), so if he
searched for all instances of game.savestats()
(game.savestatsandsettings() was only added in #557), he would've missed
the game.savemystats.
Later, when I did #591, I didn't realize that I should've replaced the
ones in the Flip Mode options with game.savestatsandsettings_menu(), so
part of the blame does fall on me.
Anyways, this is fixed now.
If there was absolutely no music playing, and you went to the in-game
options to switch between MMMMMM and PPPPPP, the behavior would be a bit
glitchy.
If you started with PPPPPP, switching once to MMMMMM wouldn't play
anything, but then switching back to PPPPPP would play MMMMMM track 15.
Then switching back to MMMMMM wouldn't do anything, but then switching
back to PPPPPP again would play PPPPPP track 15 - and from there, the
behavior is stable.
If you started with MMMMMM, switching once to PPPPPP would play MMMMMM
track 15. Then switching back to MMMMMM wouldn't do anything, but then
switching back to PPPPPP would play PPPPPP track 15 - and as above, the
behavior is stable after that.
Anyways, the point is, -1 shouldn't be passed to musicclass::play()
unless you want glitchy things. And I'm not patching -1 out of
musicclass::play() itself, because passing negative numbers results in a
useful glitch (that's existed since 2.2) where you can play MMMMMM
tracks while having PPPPPP selected, effectively doubling the amount of
usable music tracks within a custom level; it also seems like the game
does -1 checks elsewhere, so I'm just being consistent with the rest of
the game (although, yes, I am technically single-case patching this).
I ran Include What You Use on the file, and a BUNCH of transitive
includes showed up.
colourTransform is used in the file, so GraphicsUtil.h needs to be
included. libc floor() is used in the file, so math.h needs to be
included (I'm removing this next...). NULL is used, so stddef.h. And
stdlib.h is used because we use rand() directly instead of going through
fRandom(). Speaking of which, we use fRandom(), so Maths.h needs to be
included, too.
So, 2.3 added recoloring one-way tiles to no longer make them be always
yellow. However, custom levels that retexture the one-way tiles might
not want them to be recolored. So, if there are ANY custom assets
mounted, then the one-ways will not be recolored. However, if the XML
has a <onewaycol_override>1</onewaycol_override> tag, then the one-way
will be recolored again anyways.
When I added one-way recoloring, I didn't intend for any custom asset to
disable the recoloring; I only did it because I couldn't find a way to
check if a specific file was customized by the custom level or not.
However, I have figured out how to do so, and so now tiles.png one-way
recolors will only be disabled if there's a custom tiles.png, and
tiles2.png one-way recolors will only be disabled if there's a custom
tiles2.png.
In order to make sure we're not calling PhysFS functions on every single
deltaframe, I've added caching variables, tiles1_mounted and
tiles2_mounted, to Graphics; these get assigned every time
reloadresources() is called.
This function will check if a specific file is a mounted per-level
custom asset, instead of being a variable that's true if ANY file is a
mounted asset.
Now you only have to call one function (and pass it a tile number) to
figure out if you should recolor a one-way tile or not, and you don't
have to copy-paste.
It's only used in FileSystemUtils and never anywhere else, especially
not Graphics. Why is this on Graphics again?
It's now a static variable inside FileSystemUtils. It has also been
renamed to assetDir for consistency with saveDir and levelDir. Also,
it's a C string now, and is no longer an STL string.
There's no need to create an std::string for every single element just
to see if it's a key name.
At least in libstdc++, there's an optimization where std::strings that
are 16 characters or less don't allocate on the heap, and instead use
the internal 16-char buffer directly in the control structure of the
std::string. However, it's not guaranteed that all the element names
we'll get will always be 16 chars or less, and in case the std::string
does end up allocating on the heap, we have no reason for it to allocate
on the heap; so we should just convert these string comparisons to C
strings instead.
This bug is technically NOT a regression - the code responsible for it
has been around since the source release.
However, it hasn't been a problem until Graphic Options and Game Options
were added to the pause screen. Since then, if you opened the pause menu
in Flip Mode, pressing up would move to the menu option below, and
pressing down would move to the menu option above. Notably, left and
right still remain the same.
This is because the map screen input code assumes that the menu options
will be flipped around - however, this has never been the case. What
happens instead is that the menu options get flipped around time when in
Flip Mode - flipping what's already flipped - so it ends up the same
again.
(Incidentally enough, the up/down reversing code is present on the title
screen, and is correct - if you happen to set graphics.flipmode to true
on the title screen, the title screen doesn't negate the flipped menu
options, so pressing up SHOULD be treated like pressing down, and vice
versa. However, in 2.3, it's not really possible to set
graphics.flipmode to true on the title screen without using GDB or
modifying the game. In 2.2 and previous, you can just complete the game
in Flip Mode, and the variable won't be reset; 2.3 cleaned up all exit
paths to the menu to make sure everything got reset.)
This isn't a problem when there's only two options, but since 2.3 adds
two more options to the pause screen, it's pretty noticeable.
Anyway, this is fixed by simply removing the branch of the
graphics.flipmode if-else in mapinput(). The 'else' branch is now the
code that gets executed unconditionally. Don't get confused by the diff;
I decided to unindent in the same commit because it's not that many
lines of code.
This fixes a "root cause" bug (that's existed since 2.2 and below) where
recreated surfaces wouldn't preserve the blend mode of their original
surface.
The surface-level (pun genuinely unintended) bug that this root bug
fixes is the one where there's no background to the room name during the
map menu animation in Flip Mode.
This is because the room name background relies on graphics.backBuffer
being filled with complete black. This is achieved by a call to
ClearSurface() - however, ClearSurface() actually fills it with
transparent black (this is not a regression; in 2.2 and previous, this
was an "inlined" FillRect(backBuffer, 0x00000000)). This would be okay,
and indeed the room name background renders fine in unflipped mode - but
it suddenly breaks in Flip Mode.
Why? Because backBuffer gets fed through FlipSurfaceVerticle(), and
FlipSurfaceVerticle() creates a temporary surface with the same
dimensions and color masks as backBuffer - it, however, does NOT create
it with the same blend mode, and kind of sort of just forgets that the
original was SDL_BLENDMODE_NONE; the new surface is SDL_BLENDMODE_BLEND.
Thus, transparency applies on the new surface, and instead of the room
name being drawn against black, it gets drawn against transparency.
Here I'm using "surface recreation" to mean allocating a new surface
with almost the exact same properties as a given previous. As you can
see, GraphicsUtil likes to recreate surfaces all the time - copying the
masks and flags (unused lol) of an existing surface - and only varies it
by the dimensions of the new surface.
As you can see, this is a lot less wordy and a lot less repetitive than
copy-pasting it a bunch.
In normal mode, the room name is at the bottom of the screen. When you
bring up the map screen, it appears as if the room name is moving up
from the bottom of the screen, and the map screen is "pushing" it up.
The effect is pretty seamless, and when I first played the game (back in
2014), I thought it was pretty cool.
However, in Flip Mode, the room name is at the top of the screen. So one
would expect the menu animation to come from above the screen. Well, no,
it still goes from the bottom of screen; ruining the effect because it
seems like there are two room names on the screen, when there ought to
be only one.
To be fair, I only noticed this while fixing another bug now, but it's
one of those things you can't unsee (I have cursed you with knowledge!);
not to mention that I probably only didn't notice this because I don't
play in Flip Mode that often (and I'd wager almost no one does; Flip
Mode previous to 2.3 seems to have been really untested, like I said
in #165). It feels like a bit of an oversight that the direction of the
animation is the same direction as in unflipped mode. So I'm fixing
this.
If you stood in two activity zones at once, you'll automatically select
the one that got created first. And when you activated it, the activity
zone prompt would switch to fading out the prompt of the OTHER activity
zone, the one you didn't activate.
This wasn't a problem in 2.2 and previous, because the fading animation
was simply bugged and defaulted to being solid black. However, in 2.3,
the fading animation is fixed, so this is possible.
Also, this really only happens in the main game. Since there's only one
type of useful activity zone in custom levels - namely the terminal
activity zone - if two activity zones did happen to overlap, activating
one of them wouldn't result in visibly fading out a different activity
zone (because they both look the same); furthermore custom level makers
are careful to not overlap terminal activity zones, lest this result in
player confusion; furthermore the placed activity zones only cover a
small area, whereas in the main game, crewmates' activity zones are
pretty big.
(Technically, you CAN create main game activity zones in custom levels,
but those are hardcoded to call main game scripts, and basically nobody
uses them.)
So what's the solution? Simply adding game.hascontrol and script.running
checks to the updating of game.activity_last[prompt|r|g|b].
Why not add those checks to the assignment of game.activeactivity, just
above? Because that would introduce a frame ordering issue (that
would NOT be (automatically) fixed by #535) where the eligibility of
pressing Enter on an activity zone now checks if you were standing in an
activity zone LAST frame, and not THIS frame. (I tested this with
libTAS.) Better to fiddle with the rendering code than fiddle with the
actual physics code.
The specific spot I used to test this was standing in Violet's activity
zone and the activity zone of the ship radio terminals (the three
terminals on the ground in her room); the ship radio terminals are
first-placed, so if you're testing this (and you should!), make that the
prompt is of the ship radio activity zone before activation.
This probably should've been moved to RenderFixed a while ago, because
it's unnecessary to run this on every single deltaframe.
The only minor wrinkle here is that this means rendering of activity
zone fades will be delayed for 1 frame, but #535 will fix that.
Since you're now allowed to bring up the map screen during cutscenes,
you've also been able to activate activity zones and teleporter prompts
during cutscenes. This only really affects custom levels; nowhere in the
main game can you overlap with an activity zone while in a cutscene.
To fix this, I've just added a script.running check to Enter keybind
processing.
I was looking through all calls to game.returnmenu(), and I noticed that
the return option in the game pad screen didn't have a
map.nexttowercolour(). I tested it and, yep, returning from there
doesn't update the background color.
So that should be fixed now.
I'm... not sure why this was here? It's absolutely not needed.
I'm guessing maybe at one point during development, there might have
been wanted a special song to be played during the credits, or no song
at all (although the function being niceplay() instead of play() seems
to support the first possibility) - but there's no need for this to be
here.
Now that recreating the same menu keeps currentmenuoption, we can remove
all these superfluous assignments. This means repeating ourselves less;
in case the option numbers change in the future, we won't have to
remember to update these reassignments, too.
When recreating the same menu, there's basically no reason to reset the
currently-selected menu option. (Also, no need to worry about indexing
out of bounds or anything - the number gets checked while iterating over
all menu options; it's never used to actually index anything. At worst
there might be a 1-frame flicker as the bounds code in gameinput() kicks
in, but that shouldn't happen anyways.)
Zip files that have been successfully mounted in editorclass::loadZips()
will now be ignored when the game does its second pass over the levels
directory. Otherwise, this would produce a superfluous error message,
because the game would attempt to parse the zip file as a level file
(when it's not a level file and is in fact a binary file).
This returns if the file given is mounted or not. 2.3 added level zip
support, so whenever the game loads level metadata, it will mount any
zip files in the levels directory; this function can be used to check if
any of those files have been mounted, and ignore them if so.
Otherwise, this would produce a superfluous warning message in the
console. Directories are now ignored and never attempted to be opened;
so now any warning messages printed out are genuine file that something
has genuinely gone wrong with.
Well, there's still a warning message printed if there's a symlink to a
directory; this is rarer, but it's still a false positive.
This function will be used to differentiate files from directories.
Or at least that was the hope. Symlink support was added in 2.3, but it
doesn't seem like PHYSFS_stat() lets you follow the symlink to check if
what it points to is itself a file or directory. And there doesn't seem
to be any function to follow the symlink yourself...
So for now, this function considers symlinks to directories to be files.
PHYSFS_readBytes() returns a PHYSFS_sint64, but we forcefully shove it
into a 32-bit signed integer.
Fixing the type of this doesn't have any immediate consequences, but
it's good for the future in case we want to use the return value for
files bigger than 2 gigabytes; it doesn't harm us in any way, and it's
just better housekeeping.
PHYSFS_fileLength() returns -1 if the file size can't be determined. I'm
going to set it to 0 instead, because it seems like that's more
well-behaved with consumers.
Take lodepng_decode24() or lodepng_decode32(), for example - from a
quick glance at the source, it only takes in a size_t (an unsigned
integer) for the filesize, and one of the first things it does is malloc
with the given filesize. If the -1 turns into SIZE_MAX and LodePNG
attempts to allocate that many bytes... well, I don't know of any
systems that have 18 exabytes of memory. So that seems pretty bad.
The function returns a PHYSFS_sint64, but we forcefully shove it into a
PHYSFS_uint32. This means we throw away all the negative numbers, which
is bad because the function returns -1 if the size of the file can't be
determined; plus, we also throw away 32 bits of information, reducing
our range of supported file sizes from 9 exabytes to 4 gigabytes.
File size support is only as good as the weakeast link, and it looks
like one of the consumers of FILESYSTEM_loadFileToMemory(),
SDL_RWFromConstMem(), only takes in a signed 32-bit integer of size;
however, I would still like to do at least the bare minimum to support
as many file sizes as we can, and changing types around is one of those
bare minimums.
I had misread this line in #629 and thought that it was just clearing
the entire surface, when really it was filling the surface with opaque
black. ClearSurface() would instead make it transparent, which would
mean when it got drawn, it would get drawn against blue, and not black.
Whoops.
These float attributes are assigned to, and then never read again. The
coordinate systems of blocks are a bit of a mess - some use xp/yp, some
use xp/yp and rect.x/rect.y - but I can confidently say that these are
never used, because it compiles fine if I remove the attributes from the
class, plus remove all assignments to it.
Screen.cpp wasn't explicitly including SDL.h, instead relying on
Screen.h to include it.
It was also relying on SDL.h to include stdio.h on Linux, which breaks
because SDL.h doesn't include stdio.h on Windows. So stdio.h is now
explicitly included as well.
stdlib.h is not used in this file.
After reasoning about it for a bit, there's no reason for these checks
to be here. `zip_normal` will either be
/home/infoteddy/.local/share/VVVVVV/levels if the asset directory is a
directory, or levels/levelname.zip if the asset directory is inside the
same zip as the level is. I don't see how they could ever be data.zip.
My guess is because of the VCE bug where it messed up its search path,
and before that bug was fixed, it had to be worked around here by
explicitly blacklisting data.zip here. When the assets mounting stuff
was ported from VCE to vanilla, vanilla didn't have the problem, and so
this data.zip blacklisting stuff was unnecessary.
Either way, I see no reason for this, so I'm going to remove it.
There is no need to use heap-allocated strings here, so I've refactored
them out. I've also cleaned up both of the functions a bit, because the
line spacing of the previous version was completely non-existent, brace
style was same-line instead of next-line, and the variable names were a
bit misleading (in FILESYSTEM_mountassets(), there is a `zippath` AND a
`zip_path`, which are two completely different variables).
Also, FILESYSTEM_mount() now prints an error message and bails if
PHYSFS_getRealDir() returns NULL, whereas it didn't do that before.
The function is literally just an alias for PHYSFS_exists(), which does
not exclusively check for directories. Plus, the function is also used
to check if a non-directory file exists. Why is this function named
"directoryExists"?!
The info message when a .data.zip file is mounted is now differentiated
from the message when an actual directory is mounted (the .data.zip
message specifies ".data.zip").
The error message for an error occurring when loading or mounting a .zip
is now capitalized.
The "Custom asset directory does not exist" now uses puts(), because
there's no need to use printf() here.
I don't know why this is here; it's unused. I don't know why the
compiler doesn't warn about this being unused either - maybe it's
secretly being used? That also means I'm not sure if the compiler is
optimizing this away or not. Anyway, this is getting removed.
The STL here cannot be completely eliminated (because the custom entity
object uses std::string), but at least we can avoid unnecessarily making
std::strings until the very end.
There's not really any reason for this function to use heap-allocated
strings. So I've refactored it to not do that.
I would've used SDL_strrstr(), if it existed. It does not appear to
exist. But that's okay.
PhysFS by default just uses system malloc(), realloc(), and free(); it
provides a way to change them, with a struct named PHYSFS_Allocator and
a function named PHYSFS_setAllocator().
According to PhysFS docs, this function should be called before
PHYSFS_init(), which is why this allocator stuff is handled in
FileSystemUtils.cpp.
Also, I've had to make two "bridge" functions, because PHYSFS_Allocator
wants pointers to functions taking in `PHYSFS_uint64`s, not `size_t`s.
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.
When you pass NULL in for the SDL_Rect* parameter to SDL_FillRect(), SDL
will automatically fill the entire surface with that color. There's no
need for us to create the SDL_Rect ourselves.
This is a function that does what it says - it clears the given surface.
This just means doing a FillRect(), but it's better to use this function
because it conveys intent better.
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.
One of these days, I need to get around to running Include What You Use
on this codebase. Until then, while I was working on #624, I noticed
these; I'm removing them now.
The recently released SDL 2.0.14 adds a native function for opening URIs
from the host system, superseding the OS-specific implementations of
FILESYSTEM_openDirectory.
This fixes a regression where moving platforms had no collision. Because
their width and height would be maintained, but their type would be -1.
(Also because I didn't test enough.)
In #565, I decided to set blocks' types to -1 when disabling them, to be
a bit safer in case there was some code that used block types but not
their width and heights. However, this means that when blocks get
disabled and re-created in the platform update loops, their types get
set to -1, which effectively also disables their collision.
In the end, I'll just have to compromise and remove setting blocks to
type -1. Because in a better world, we shouldn't be destroying and
creating blocks constantly just to move some platforms - however, fixing
such a fundamental problem is beyond the scope of at least 2.3 (there's
also the fact that this problem also results in some bugs that are a
part of compatibility, whether we like it or not). So I'll just remove
the -1.
next_split_s() could potentially commit out-of-bounds indexing if the
amount of source data was bigger than the destination data.
This is because the size of the source data passed in includes the null
terminator, so if 1 byte is not subtracted from it, then after it passes
through the VVV_min(), it will index 1 past the end of the passed buffer
when null-terminating it.
In contrast, the other argument of the VVV_min() does not need 1
subtracted from it, because that length does not include a null
terminator (next_split() returns the length of the substring, after all;
not the length of the substring plus 1).
(The VVV_min() here also shortens the range of values to the size of an
int, but we'll probably make size_t versions anyway; plus who really
cares about supporting massively-sized buffers bigger than 2 billion
bytes in length? That just doesn't make sense.)
If PHYSFS_enumerate() isn't successful, we now print that it wasn't
successful, and print the PhysFS error message. (We should get that
logging thing going sometime...)
Note that level dir listing still uses plenty of STL (including the end
product - the `LevelMetaData` struct - which, for the purposes of 2.3,
is okay enough (2.4 should remove STL usage entirely)); it's just that
the initial act of iterating over the levels directory no longer takes
four or SIX(!!!) heap allocations (not counting reallocations and other
heap allocations this patch does not remove), and no longer does any
data marshalling.
Like text splitting, and binary blob extra indice grabbing, the current
approach that FILESYSTEM_getLevelDirFileNames() uses is a temporary
std::vector of std::strings as a middleman to store all the filenames,
and the game iterates over that std::vector to grab each level metadata.
Except, it's even worse in this case, because PHYSFS_enumerateFiles()
ALREADY does a heap allocation. Oh, and
FILESYSTEM_getLevelDirFileNames() gets called two or three times. Yeah,
let me explain:
1. FILESYSTEM_getLevelDirFileNames() calls PHYSFS_enumerateFiles().
2. PHYSFS_enumerateFiles() allocates an array of pointers to arrays of
chars on the heap. For each filename, it will:
a. Allocate an array of chars for the filename.
b. Reallocate the array of pointers to add the pointer to the above
char array.
(In this step, it also inserts the filename in alphabetically -
without any further allocations, as far as I know - but this is a
COMPLETELY unnecessary step, because we are going to sort the list
of levels by ourselves via the metadata title in the end anyways.)
3. FILESYSTEM_getLevelDirFileNames() iterates over the PhysFS list, and
allocates an std::vector on the heap to shove the list into. Then,
for each filename, it will:
a. Allocate an std::string, initialized to "levels/".
b. Append the filename to the std::string above. This will most
likely require a re-allocation.
c. Duplicate the std::string - which requires allocating more memory
again - to put it into the std::vector.
(Compared to the PhysFS list above, the std::vector does less
reallocations; it however will still end up reallocating a certain
amount of times in the end.)
4. FILESYSTEM_getLevelDirFileNames() will free the PhysFS list.
5. Then to get the std::vector<std::string> back to the caller, we end
up having to reallocate the std::vector again - reallocating every
single std::string inside it, too - to give it back to the caller.
And to top it all off, FILESYSTEM_getLevelDirFileNames() is guaranteed
to either be called two times, or three times. This is because
editorclass::getDirectoryData() will call editorclass::loadZips(), which
will unconditionally call FILESYSTEM_getLevelDirFileNames(), then call
it AGAIN if a zip was found. Then once the function returns,
getDirectoryData() will still unconditionally call
FILESYSTEM_getLevelDirFileNames(). This smells like someone bolting
something on without regard for the whole picture of the system, but
whatever; I can clean up their mess just fine.
So, what do I do about this? Well, just like I did with text splitting
and binary blob extras, make the final for-loop - the one that does the
actual metadata parsing - more immediate.
So how do I do that? Well, PhysFS has a function named
PHYSFS_enumerate(). PHYSFS_enumerateFiles(), in fact, uses this function
internally, and is basically just a wrapper with some allocation and
alphabetization.
PHYSFS_enumerate() takes in a pointer to a function, which it will call
for every single entry that it iterates over. It also lets you pass in
another arbitrary pointer that it leaves alone, which I use to pass
through a function pointer that is the actual callback.
So to clarify, there are two callbacks - one callback is passed through
into another callback that gets passed through to PHYSFS_enumerate().
The callback that gets passed to PHYSFS_enumerate() is always the same,
but the callback that gets passed through the callback can be different
(if you look at the calling code, you can see that one caller passes
through a normal level metadata callback; the other passes through a zip
file callback).
Furthermore, I've also cleaned it up so that if editorclass::loadZips()
finds a zip file, it won't iterate over all the files in the levels
directory a third time. Instead, the level directory only gets iterated
over twice - once to check for zips, and another to load every level
plus all zips; the second time is when all the heap allocations happen.
And with that, level list loading now uses less STL templated stuff and
much less heap allocations.
Also, ed.directoryList basically has no reason to exist other than being
a temporary std::vector, so I've removed it. This further decreases
memory usage, depending on how many levels you have in your levels
folder (I know that I usually have a lot and don't really ever clean it
up, lol).
Lastly, in the callback passed to PhysFS, `builtLocation` is actually no
longer hardcoded to just the `levels` directory, since instead we now
use the `origdir` variable that PhysFS passes us. So that's good, too.
If PHYSFS_mountHandle() failed to mount a zip file, we would print
PhysFS's error message straight, without any surrounding context. This
seems a little weird, and doesn't maximize understanding for readers;
I've made it so now the error message is "Could not mount <zip file>:
<PhysFS error>".
When Ethan added PhysFS to the game, he put in a hardcoded check (marked
with a FIXME) that explicitly removed all filenames that were "data"
returned by PHYSFS_enumerateFiles(). Apparently this was due to a weird
bug with the function putting in "data" strings in its output in PhysFS
2.0.3; however, the game now uses PhysFS 3.0.2, and I could not
reproduce this bug on my system. (I also tested, and this also
straight-up ignores legitimate level filenames that just happen to be
"data" (without the .vvvvvv extension).)
After talking with Ethan in Discord DMs, I asked if we could remove this
check, and he said that we could. So I'm doing it now.
Just like I refactored text splitting to no longer use std::vectors,
std::strings, or temporary heap allocations, decreasing memory usage and
improving performance; there's no reason to use a temporary
heap-allocated std::vector to grab all extra binary blob indices, when
instead the iteration can just be more immediate.
Instead, what I've done is replaced binaryBlob::getExtra() with
binaryBlob::nextExtra(), which takes in a pointer to an index variable,
and will increment the index variable until it reaches an extra track.
After the caller processes the extra track, it is the caller's
responsibility to increment the variable again before passing it back to
getExtra().
This avoids all heap allocations and brings down the memory usage of
processing extra tracks.
If you configure the build with -DBUNDLE_DEPENDENCIES=OFF, then VVVVVV
will dynamically link with TinyXML-2 and PhysicsFS instead of using the
bundled source code in third_party/ and statically linking with them.
Unfortunately, it doesn't seem like distros package LodePNG, and LodePNG
isn't intended to be packaged, so we can't dynamically link with it, nor
can we use some system LodePNG header files somewhere else because those
don't exist.
UTF8-CPP is a special case, because no matter what, it's going to be
statically linked with the binary (it doesn't come as a shared object
file in any way). So with -DBUNDLE_DEPENDENCIES=OFF, we will use the
system UTF8-CPP header files instead of the bundled ones, but it will
still be statically linked in with the binary.
The main motivation for doing this is so if VVVVVV ever gets packaged in
distros, distro maintainers would be more likely to accept it if there
was an option to compile the game without bundled dependencies. Also, it
discourages modifying the third-party dependencies we have, because it's
always possible for someone to compile those dependencies without our
changes, with this CMake option.
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.
