During review of #869, I looked at this part of the codebase again. I
have no idea how or why, but during the course of 2.4 this whole area
just became a mess.
The issues I fixed (in no particular order):
- Copy-pasting the code that loads from the binary blobs
- Making sure SDL_RWFromConstMem is used over SDL_RWFromMem wherever
possible
- Adding checks to make sure the index from the binary blob is valid
(it's possible it could not exist)
- Adding checks to make sure we gracefully handle
SDL_RWFromConstMem/PHYSFSRWOPS_openRead returning NULL
- Moving the pointer asterisk to the type instead of the name :)
This isn't necessary, but it does silence these annoying logs if you
pass an invalid argument or don't have data.zip:
[ERROR] Could not get window size: Invalid renderer
[WARN] Stats not loaded! Not writing unlock.vvv.
[ERROR] Could not get window size: Invalid renderer
[WARN] Settings not loaded! Not writing settings.vvv.
To do this, I've added FILESYSTEM_isInit().
We are no longer copy-pasting LodePNG source files directly.
As we can't rename lodepng.cpp to lodepng.c in the submodule itself, we
need to make a wrapper file, lodepng_wrapper.c, that #includes
lodepng.cpp, but gets compiled as C.
This prevents writing to unlock.vvv or settings.vvv if the game hasn't
made an attempt to load them yet. Otherwise, if the game aborted via
VVV_exit() because of, say, failure to parse a graphics file, it would
overwrite perfectly existing valid save data since it hasn't loaded it
yet.
Fixes#870.
Another cause of #870 is d0ffafe117, as a
bisect tells me. What that commit did is remove screenbuffer as a
pointer, since it's a statically-allocated object that _should_ always
exist, and it removed the `screenbuffer == NULL` guards in savestats()
and savesettings(). Unfortunately, those guards did something very
important - namely, they prevented writing to the save files when the
filesystem wasn't initialized. But that wasn't made clear, because it
seemed like the point of those guards was to prevent dereferencing NULL.
So instead, explicitly make it clear that
FILESYSTEM_saveTiXml2Document() needs to fail if the filesystem isn't
initialized. I've done this by adding an isInit bool to
FileSystemUtils.cpp.
Issue #870 showed one of the problems that this game has, namely that it
only sometimes checks SDL return values, and did not do so in this case.
Part of the cause of #870 is that Screen::GetWindowSize does not check
the return value of SDL_GetRendererOutputSize, so when that function
fails (as in the case where m_renderer is NULL and does not exist), it
does not initialize the out values, so it ends up writing uninitialized
values to the save files.
We need to make sure every function's return value is checked, not just
SDL functions, but that will have to be done later.
While reviewing #272, I noticed that the PR was passing these two
arguments through a helper function, even though they really shouldn't
ever change. To obviate the need to pass these through, I'm making them
global variables.
pathSep is just a string literal from PhysFS, while basePath is a whole
complicated calculation from SDL and needs to be freed. It will be freed
upon filesystem deinit (as is done with PhysFS and the STDIN buffer).
Additionally the logic in FILESYSTEM_init is simplified by no longer
needing to keep a retval variable or use gotos to free basePath in
there.
This lets any script name use capitals and spaces all they want, while
still being able to jump to them via iftrinkets() or similar.
The issue is that whenever tokenize() is ran, all spaces are stripped
and every argument is lowercased before being put into `words`. So, the
solution here is to create a raw_words array that doesn't perform space
stripping or lowercasing, and to refer to that whenever there's a script
command that loads a script. We keep the lowercasing and space removal
elsewhere to be more forgiving to newcomers.
This is technically a forwards compatibility break, but it's only a
minor one, and all levels that utilize it can still be easily modified
to work on older versions anyway.
As reported by Dav999, Victoria and Vermilion's trophy colors are
swapped again in 2.4. He points to
37b7615b71, the commit where I fixed the
color masks of every single surface to always be RGB or RGBA.
It sounded plausible to me, because it did have to do with colors, after
all. However, it didn't make sense to me, because I was like, I didn't
touch the trophy colors at all after I originally fixed them.
After I ruled out the RGBf() function as a confounder, I decided to see
whether intentionally reversing the color order in RGBf() to be BGR
would do anything, and to my surprise it actually swapped the colors
back around and it didn't actually look bad.
And then I realized: Swapping the trophy colors between RGB and BGR
ordering results in similar colors that still look good, but are simply
wrong, but not so wrong that they take on a color that no crewmate uses,
so it'd appear as if the crewmates were swapped, when in reality the
only thing that was swapped was actually the color order of the colors.
Trying to fix this by swapping the colors again, I actively confused
colors 33 and 35 (Vermilion and Victoria) with colors 32 and 34
(Vitellary and Viridian), so I was confused when Vermilion and Victoria
weren't swapping. Then as a debugging step, I only changed 34 to 32
without swapping 32 as well, and then finally noticed that I was
swapping Vitellary and Viridian, because there were now two Vitellarys.
And then I was reminded that Vitellary and Viridian were also wrongly
swapped since 2.0 as well.
And so then I finally realized: The original comments accompanying the
colors were correct after all. The only problem was that they were fed
into a function, RGBf(), that read the colors backwards, because the
codebase habitually changed the color order on a whim and it was really
hard to reason out which color order should be used at a given time, so
it ended up reading RGB colors as BGR, while it looked like it was
passing them through as-is.
So what happened was that in the first place, RGBf() was swapping RGB to
BGR. Then I came and swapped Vermilion and Victoria, and Vitellary and
Viridian around. Then later I fixed all the color masks, so RGBf()
stopped swapping RGB and BGR around. But then this ended up swapping the
colors of Vermilion and Victoria, and Vitellary and Viridian once again!
Therefore, swapping Vermilion and Victoria, and Vitellary and Viridian
was incorrect. Or at least, not the fix to the root cause. The root
cause would be to swap the colors in RGBf(), but this would be sort of
confusing to reason about - at least if I didn't bother to just type the
RGB values into an image editor. But that doesn't fix the real issue,
which is that the game kept swapping RGB and BGR around in every corner
of the codebase.
I further confirmed that there was no more RGB or BGR swapping by
deleting the plus-one-divide-by-three transformation in RGBf() and
seeing if the colors looked okay. Now with the colors being brighter, I
could see that passing it straight through looked fine, but
intentionally reversing it to be BGR resulted in colors that at a
distance looked okay, but were either washed out or too bright. At least
finally I could use my 8 years of playing this game for something.
So in conclusion, actually, 37b7615b71
("Fix surface color masks") was the real fix, and
d271907f8c ("Fix Secret Lab Time Trial
trophies having wrong colors") was the real regression. It's just that
the regression came first, but it wasn't really a regression until I did
the other fix, so the fix isn't the regression, the regression is...
this is hurting my brain. Or the real regression was the friends we made
along the way, or something like that.
This is the most trivial bug ever caused by the technical debt of those
god-awful reversed color masks.
---
This reverts commit d271907f8c.
Fixes#862.
In hindsight, the FAudio pointer will likely be in SoundTrack since we will
want to keep the mastering voice closer to the sounds and their source voice
arrays, while the MusicTrack will likely just be one source voice that gets
PCM from different streams.
This looks redundant but will actually help in the transition to FAudio; we
mostly want to keep the game logic the same while reimplementing the current
mixer, weirdness and all. Once that's done and confirmed to be stable and
consistent we can start cutting out the workarounds and quirks.
This meant making the track vectors static, but that's kind of what we do with musicclass anyway?
In any case, this will make the transition to FAudio MUCH less invasive.
This is quite simple. Just use a function pointer that switches out
which function we're going to use.
...Or not. C++ syntax makes this a bit awful since the function is a
member of a class. Did I mention how much I don't like C++?
Issue #849 suggested making integer be the default on Big Picture and
Steam Deck, but after thinking about it more, I think it's better and
more simple to just default to integer mode in general.
Reason being that people in Big Picture shouldn't expect the picture to
look different if they're out of Big Picture but still in fullscreen, or
have the picture look different in fullscreen depending on if they
launched the game for the first time in Big Picture or not. And besides,
the less lines of code, the better. So I'm just making integer mode the
default.
This enum is to just make each mode be readable, instead of mysterious
0/1/2 values. It's not a strictly-typed enum because we still have to
serialize it as ints in the XML, but it's better than just leaving them
as ints.
This also adds a NUM_SCALING_MODES enum, so we don't have to hardcode
that 3 when cycling scaling modes anymore.
This is mainly to make sure the game is definitely set to fullscreen in
Big Picture and on the Steam Deck, and to also remove windowed options
that wouldn't make sense if you're not on a desktop (toggling
fullscreen, resize to nearest). Those options would also be removed on
console and mobile too.
There's a bit of an annoying bug where if you launch the game in forced
fullscreen mode, but then exit and relaunch in normal mode, your game
will have fullscreen window sizes but it won't be fullscreen. This is
because forced fullscreen mode tries to preserve your non-forced
fullscreen setting, but due to the way window sizes are stored and
queried, it can't preserve the non-forced window size. This is a bit
difficult to work around, so I'm just putting in a FIXME here because we
can fix it later and I'd rather have a slightly buggy forced fullscreen
mode than not have one at all.
Closes#849.
Here's my notes on all the existing functions and what kind of time
formats they output:
- Game::giventimestring(int hrs, int min, int sec)
H:MM:SS
MM:SS
- Game::timestring()
// uses game.hours/minutes/seconds
H:MM:SS
MM:SS
- Game::partimestring()
// uses game.timetrialpar (seconds)
MM:SS
- Game::resulttimestring()
// uses game.timetrialresulttime (sec) + timetrialresultframes (1/30s)
MM:SS.CC
- Game::timetstring(int t)
// t = seconds
MM:SS
- Game::timestringcenti(char* buffer, const size_t buffer_size)
// uses game.hours/minutes/seconds/frames
H:MM:SS.CC
MM:SS.CC
- UtilityClass::timestring(int t)
// t = frames, 30 frames = 1 second
S:CC
M:SS:CC
This is kind of a mess, and there's a lot of functions that do the same
thing except using different variables. For localization, I also want
translators to be able to localize all these time formats - many
languages use the decimal comma instead of the decimal point (12:34,56)
maybe some languages really prefer something like 1時02分11秒44瞬...
Which I don't know to be correct, but it's good to be prepared for it
and not restrict translators arbitrarily to only changing ":" and "."
when we can start making the system better in the first place.
I added a new function, UtilityClass::format_time. This is the place
where all time formats come together, given the number of seconds and
optionally frames. I have simplified the above-mentioned functions
somewhat, but I haven't given them a complete refactor or renaming -
I mainly made sure that they all use the same backend so I can make the
formats consistent and properly localizable.
(And before we start shoving more temporary char buffers everywhere
just to get rid of the std::string's, maybe we need to think of a
globally used working buffer of size SCREEN_WIDTH_CHARS+1, as a
register of sorts, for when any line of text needs to be made or
processed, then printed, and then goes unused. Maybe help.textrow,
or something like that.)
As for this commit, the available time formats are now more consistent
and changed a little in some places. Leading zeroes for the first unit
are now no longer included, time trial results and the Super Gravitron
can now display hours when they went to 60 minutes before, and we now
always use .CC instead of :CC. These are the formats:
- H:MM:SS
- H:MM:SS.CC
- M:SS
- M:SS.CC
- S.CC (only used when always_minutes=false, for the Gravitrons)
Here's what changes to the current functions:
- Game::partimestring() is removed - it was used in two places, and
could be replaced by game.timetstring(game.timetrialpar)
- Game::giventimestring(h,m,s) and Game::timestring() are now wrappers
for the other functions
- The four remaining functions (Game::resulttimestring(),
Game::timetstring(t), Game::timestringcenti(buffer, buffer_size)
and UtilityClass::timestring(t)) are now wrappers for the "central
function", UtilityClass::format_time.
- UtilityClass::twodigits(int t) is now unused so it's also removed.
- I also added int UtilityClass::hms_to_seconds(int h, int m, int s)
This de-duplicates the code, simplifying the codebase and reducing the
number of code paths that needs to be maintained. It also adds
robustness checks to LoadIcon that weren't there before (checking that
loading the file succeeded and that decoding the file also succeeded).
Now, you might think that loading the image with alpha will change
things in some way. But actually, I tested it, and I'm pretty sure it
doesn't. Since my window manager, i3, doesn't display icons, I've had to
resort to this hacky multi-liner
( https://unix.stackexchange.com/a/48866 ) to dump the icon to a PAM
file. I don't know what a PAM file is and all my various attempts to
convert it into something readable failed. But what I did instead was
just grab the icon of the game before this commit (on 2.3, just to be
extra sure), and `diff`ed it with the grabbed icon now, and they end up
being the exact same file. So there's literally no difference.
The only other consideration is that LoadImage needs to be exported,
since it's implemented in GraphicsResources.cpp. I just opted to
forward-declare it right before LoadIcon in Screen.cpp, since it's
really the only other time it's used. No need to create a new header
file for it or anything.
This is just to simplify the function. I really don't see any point in
taking away the alpha for some images, other than to disappoint people
who mod the game assets. It just complicates loading the image with no
real gain. To reduce maintenance costs, let's remove this alternate code
path.
Also it's a default argument and I don't like default arguments.
This argument... doesn't do anything.
First off, setting it to true explicitly enables blending on the
resulting surface, which is kind of the exact opposite of the variable
name and is misleading to say the least? And secondly, SDL surfaces have
blending enabled by default anyways, so it still doesn't even do
anything.
It's also a default argument, and I'm not one to shy away from removing
such default arguments.
This includes:
- Removing the constructor in favor of actually being able to see that
there's an actual function called being made initializing the struct
- Removing the use of a reference in Screen::init() in favor of using a
pointer
- Adding the struct qualifier everywhere (it's not much typing),
although technically you could typedef it in C, but I'd rather much
not typedef just to remove a tag qualifier
I know earlier I removed the gameScreen extern in favor of using
screenbuffer, but that was only to be consistent. After further
consideration, I have found that it's actually really stupid.
There's no reason to be accessing it through screenbuffer, and it's
probably an artifact of 2.0-2.2 passing stack-allocated otherwise-global
classes everywhere through function arguments. Also, it leads to stupid
bugs where screenbuffer could potentially be NULL, which has already
resulted in various annoying crashes in the past. Although those could
be fixed by simply initializing screenbuffer at the very top of main(),
but, why not just scrap the whole thing anyway?
So that's what I'm doing.
As a nice side effect, I've removed the transitive include of Screen.h
from Graphics.h. This could've been done already since it only includes
it for the pointer anyway, but it's still good to do it now.
In aa7b63fa5f, I didn't notice that the
result was implicitly being converted to int by the min/max from before.
I instead added it to the existing char, but that resulted in a char
overflow (it's unsigned, so thankfully not undefined behavior).
But of course the entire point of that commit is to make it explicitly
clear when you are converting between types, intentionally or otherwise,
in min/max comparisons. So despite causing a regression (which I have
now fixed), at least it did its job.
It's been long overdue that this variable be named properly. 2.2 added
integer scaling mode (thanks Ethan), 2.3 renamed it to scaling mode. Now
2.4 will properly call it what it is so people won't be confused by it.
The ScreenSettings struct member is renamed from stretch to scalingMode
along with the Screen class member being renamed, as well as the
toggleStretchMode function being renamed to toggleScalingMode as well.
Unfortunately, due to compatibility, we can't change the <stretch> XML
tag.
VVV_min/max are functions that only operate on ints, and SDL_min/max are
macros that operate on any type but double-evaluate everything.
I know I more-or-less said earlier that SDL_min/max were dumb but I've
changed my mind and think it's better to use them, taking care to make
sure you don't double-evaluate, rather than trying to generate your own
litany of functions with either your own hand-rolled generation macros,
C++ templates, C11 generics, or GCC extensions (that last one you'd
technically use in a macro but it doesn't really matter), all of which
have more downsides than just not double-evaluating.
And the upside of not double-evaluating is that you're disencouraged
from having really complicated single-line min/max expressions and
encouraged to precompute the values beforehand anyway so the final
min/max is more readable. And furthermore you'll notice when you
yourself end up doing double-evaluations anyway. I removed a couple
instances of Graphics::len() being double-evaluated in this commit (as
well as cleaned up some other min/max-using code). Although the only
downside to those double-evaluations was unnecessary computation,
rather than checking the wrong result or having multiple side effects,
thankfully, it's still good to minimize double-evaluations where
possible.
The reason why the wall stuck flipping behavior happened in the first
place was because the code went like this:
if (jumppressed)
{
if (onground && gravitycontrol == 0)
{
gravitycontrol = 1;
}
if (onroof && gravitycontrol == 1)
{
gravitycontrol = 0;
}
}
Basically, if you were both on ground and on a roof (i.e. stuck in a
wall), you would flip, but then due to code order and the fact that the
statement is not connected to the previous one, you would immediately
unflip afterwards. But if you were already flipped then the only path
that can be taken is to unflip you, since it's the statement that
appears last.
52fceb3f69 replaces the onground/onroof
conditionals with any_onground/any_onroof, so any player entity would
allow you to flip. But otherwise the code is the same. So is that the
problem?
No; tracing it through with GDB reveals that when you flip,
gravitycontrol is being set to 1, but never being set to 0. And it turns
out that's because any_onroof is not getting set. And that happens
because of another thing that 52fceb3f69
did - which was to set any_onground/any_onroof to true if indeed any
player entity was on ground or on a roof.
Unfortunately, the way Leo did it was to make the two statements
mutually exclusive - an 'if'-'else if' instead of two separate
statements. So a single entity could not mark both any_onground and
any_onroof as true (and the majority of the time, you will be a single
entity).
Thus, the solution is to just drop that 'else'.
Fixes#855.
I noticed when going frame-by-frame in Vertigo that sometimes the
wrapping enemies at the top sometimes just "popped" in frame. This is
because the sprite warp code only draws the warping sprite of sprites at
the bottom of the screen if they're below y=210. However, the warp point
starts at y=232, and warp sprites can be at most 32x32, which is exactly
the case with the Vertigo sprites, which are exactly 32x32. So the warp
code should start warping sprites if they're below y=200 (232 - 32)
instead.
Horizontal warping also has this problem; it warps at x=320 and
starts drawing warp sprites at x=300, even though it should start
drawing at x=288 (320 - 32). I've gone ahead and fixed that as well.
This is just in case the background gets changed by a custom level or
something to be something that would otherwise result in bad contrast.
Also if it needs to go outside the box for some reason. And I just like
the look of the outline.
Whew, look at all those copy-pasted print statements!
Doing this because of the in-game timer feature. The text would
otherwise clash harshly with the timer otherwise. Even with the outline
it still clashes, but at least there's an outline so it's not as harsh.
