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.
