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.
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.
