This adds color support to the output of the console on Windows. Now if
you're using Windows 10 build 1511 or later (I think it's build 1511
anyway; they added more VT sequence support in later versions), you will
see colors by default. This isn't due to Windows helping in any way;
this commit has to specifically enable it with SetConsoleMode() because
by default, Windows won't enable color support unless we enable it. (Or
if it's enabled in the registry, but having to go through the registry
to enable basic shit like that is completely fucking stupid.)
I tested this in my Windows 10 virtual machine and it's completely
working.
Previously, we were using `color_enabled` to mean both that the color
was supported and that it was enabled by the user (which it is enabled
by default). But this logic doesn't work well if the color check
function is called again and ends up enabling color after the user
disabled it. To fix this, just separate the two so the user controls one
`color_supported` variable and the `color_enabled` variable is separate.
Check both of them in order to print color, of course.
This adds the `-console` command-line option (for Win32 only) so the
game can spawn an attached console window which will contain all console
output.
This is to make it easier for people to debug on Windows systems.
Otherwise, the only way to get console output would be to either compile
the application as a console app (i.e. switch the subsystem to console)
- which is undesirable for regular users as this makes it so a console
is always spawned even when unwanted - or launch the game with shell
arguments that make it so output is redirected to a file.
As a result, color checking support is factored out of vlog_init() into
its own function, even though we don't support colors on Windows.
Using SDL_GetTicks() to seed the Gravitron RNG caused many
reproducibility issues while syncing https://tasvideos.org/7575S . To
fix this, add a frame counter, which is a number that is incremented
every frame and never resets, and use it instead.
If someone needs to switch back to SDL_GetTicks() for old TASes, then
provide the -seed-use-sdl-getticks command-line option for them.
In its previous location, it would only print the value of `s` after it
had been mutated by `splitmix32` four times, and it doesn't get used
after that, so the print isn't very useful.
Mixing code and declarations here is fine because starting from a few
months ago, we compile with C99 and if we ever need to compile with C90
then it's trivial to add braces surrounding the declarations.
If a music track has a loop comment with a negative value, ignore all
comments of the track. This is just to prevent any weirdness from
happening as it's safer to just let the track loop improperly. Also log
to the console to let users know.
This is the same thing that SDL_mixer does now:
libsdl-org/SDL_mixer@e819489459
This commit happened as a result of discussion on the VVVVVV Discord
server about SDL_mixer 2.0.4 behavior with weird loop comment values
(e.g. octal input with leading zeroes). This is simply updating the code
to be in line with what newer versions of SDL_mixer do.
Just in case it happens. Comments aren't really important to the game
(at worst a track will just loop in the wrong place) so it's fine to
carry on here and ignore all comments if this happens.
This does the following:
- Const-qualify variables if they are not modified
- Place each statement onto their own separate lines
- Place the asterisk with the type instead of the variable name
- Combine declarations and initializations where possible
They weren't ever being used, and nobody really ever uses the return
value from the printf family of functions anyway. They return how many
bytes were actually printed, but if it's less than you expected then
there's not much you can really do about them. Also the vlog_* functions
were computing them inaccurately because I only set the return value to
the return value of vprintf when there's other print functions being
called, but regardless there's no reason to have a return value here
anyway.
The hilariously-named WIN32_LEAN_AND_MEAN slims down the number of
header files included in the already-massive `windows.h`. I know people
say Moore's law and precompiled headers and all that (well, we don't use
precompiled headers), but they kinda forgot about virtual machines, and
there's no reason not to define this and slim down the number of headers
anyway.
This started when I saw the warning that GetVersionExW was deprecated,
then looked it up and found StackOverflow answers saying that you should
basically detect the feature directly instead of checking the version,
which makes sense to me. Then I found that I could probably detect color
support by using GetConsoleMode and GetStdHandle. But then I asked
myself what the point was unless you could get color output directly in
the terminal, which it seems like you really can't if your app is a GUI
app. (I have no idea why Windows makes this pointless distinction
between console and GUI apps...)
I tested Command Prompt, PowerShell, Windows Terminal (which is just
PowerShell again), and even Git Bash (MINGW64), but none of them will
ever give the console output of a GUI app such as VVVVVV. The closest I
got is that Git Bash doesn't seem to detach the process, but it will
simply produce no output.
At this point I feel like it's not worth it keeping this code around if
it didn't even work in the first place, so I'm removing it. People can
always enable color by using the -forcecolor command-line argument
anyway.
I'm fine with putting the release version in a header file, thus
necessitating the need to recompile every file that includes it if it's
changed, simply because it's not supposed to be changed that often.
The SDL_arraysize is necessary because sometimes we'll have subreleases
(e.g. 2.4.1, 2.4.2, 2.4.3), and who knows, maybe we'll get to 2.10
someday.
This reworks how the commit hash and date are compiled so that if
they're changed (and they're changed often), only one source file needs
to be recompiled in order to update it everywhere in the game, no matter
how many source files use the hash or date.
The commit hash and date are now declared in InterimVersion.h (and they
need `extern "C"` guards because otherwise it results in a link fail on
MSVC because MSVC is stupid).
To do this, what now happens is that upon every rebuild,
InterimVersion.in.c is processed to create InterimVersion.out.c, then
InterimVersion.out.c is compiled into its own static library that is
then linked with VVVVVV.
(Why didn't I just simply add it to the list of VVVVVV source files?
Well, doing it _now_ does nothing because at that point the horse is
already out of the barn, and the VVVVVV executable has already been
declared, so I can't modify its sources. And I can't do it before
either, because we depend on the VVVVVV executable existing to do the
interim version logic. I could probably work around this by cleverly
moving around lines, but that'd separate related logic from each other.)
And yes, the naming convention has changed. Not only did I rename
Version to InterimVersion (to clearly differentiate it from
ReleaseVersion, which I'll be adding later), I also named the files
InterimVersion.in.c and InterimVersion.out.c instead of
InterimVersion.c.in and InterimVersion.c.out. I needed to put the file
extension on the end because otherwise CMake wouldn't recognize what
kind of language it is, and I mean like yeah duh of course it doesn't,
my text editor doesn't recognize it either.
This fixes a regression where the game ignored the amount of frames you
held down a direction if you released the direction during death.
Previously, the game only checked the amount of frames you held down a
direction if you were able to control the player. If you weren't able to
control the player (e.g. during the death animation), then the number of
frames it counted didn't change. This also meant that if you were
holding a direction before you died, but released it during death, the
game wouldn't zero out the number of frames you held it.
This behavior was useful because it meant you could keep the
deceleration momentum that you normally get by holding a direction for 5
frames just by holding a direction for less than 5 frames after dying,
if you had the rest of the hold frames before you died. This behavior is
what's used in https://tasvideos.org/7575S at around frame 7200.
Unfortunately, #609 made it so that the direction hold processing
happened even if the player didn't have control, meaning that it would
zero the hold frames during the death animation in the TAS, thus
desyncing it when it performed the maneuver it relied on the extra
momentum for after Viridian respawns.
The solution here is to just add the check back in again.
Fixes#887.
While fixing #885, I noticed that I had a bunch of
`special/stdin.vvvvvv` entries saved in my `levelstats.vvv`. At once I
knew that the dumb `special/stdin` hack that actually checks if the
filename passed is `special/stdin` was to blame.
STDIN playtesting was first merged, I knew in the back of my mind that
it was a bit of a dumb hack, but I didn't know it would cause
consequences like showing up in `levelstats.vvv`. For now, I'll just
have to patch it, but hopefully in the future I'll remove the dumb hack
entirely. Commenting both instances of the dumb hack with instructions
to grep for it should help maintainers out.
This is useful to investigate any TAS desync/reproducibility issues
relating to RNG, because even though I specifically separated the
Gravitron RNG away from other RNG and made it not dependent on the
system libc rand() function, there's still apparently some differences
in RNG execution between systems, resulting in TASVideos submission 7575
( https://tasvideos.org/7575S ) not syncing for everyone except the
author.
It seems that SDL_GetTicks(), which is used to seed the xoshiro RNG, is
not reliably consistent between systems, so in the future I will
probably replace it with a counter that is incremented each frame
starting from game startup, which is probably better.
This fixes the following warnings:
desktop_version/src/Music.cpp: At global scope:
desktop_version/src/Music.cpp:240:23: warning: non-static data member initializers only available with ‘-std=c++11’ or ‘-std=gnu++11’ [-Wc++11-extensions]
240 | Uint8 *wav_buffer = NULL;
| ^
desktop_version/src/Music.cpp:414:32: warning: non-static data member initializers only available with ‘-std=c++11’ or ‘-std=gnu++11’ [-Wc++11-extensions]
414 | Uint8* decoded_buf_playing = NULL;
| ^
desktop_version/src/Music.cpp:415:32: warning: non-static data member initializers only available with ‘-std=c++11’ or ‘-std=gnu++11’ [-Wc++11-extensions]
415 | Uint8* decoded_buf_reserve = NULL;
| ^
desktop_version/src/Music.cpp:416:21: warning: non-static data member initializers only available with ‘-std=c++11’ or ‘-std=gnu++11’ [-Wc++11-extensions]
416 | Uint8* read_buf = NULL;
| ^
These warnings are because the non-static data members (i.e. data
members that will be different for each instance of a class) are being
initialized at the same time as they're being declared, which means
that's what their value will be when the class instance is initialized.
However, this is only a C++11 feature, and we don't use C++11. Luckily,
we don't need to do this, and this is in fact redundant because we
already zero out the class instance in its constructor using
SDL_zerop(). Therefore, we should remove these initializers to fix
compliance with C++03.
Instead, for any number that isn't in the list of number words, just
return the regular Arabic numerical representation (i.e. just convert it
to string). It's better than having "Lots" or "???", neither of which
really tell you what the number actually is.
As already described in cc61194bed, as
well as Ved's commits from the last almost two weeks, starting VVVVVV
from Ved for playtesting could be made a lot faster by "preloading" the
game - letting it do all its asset loading in the background without
creating a window - and then waiting until the level is passed in via
stdin. There's only one problem left with this approach: VVVVVV
currently expects the starting position to be passed via command line
arguments, which isn't known yet at the time we'd like to start VVVVVV.
Therefore, this commit allows passing the starting position via the
level XML, instead of via arguments.
The extra XML looks like this, and is added next to the <Data> tag:
<Playtest>
<playx>214</playx>
<playy>112</playy>
<playrx>100</playrx>
<playry>100</playry>
<playgc>0</playgc>
<playmusic>4</playmusic>
</Playtest>
This is handled similarly to how the equivalent arguments are handled:
when the level metadata is loaded for CLI playtesting, we also try to
find this tag, and if it exists, it sets the same variables that the
arguments would have otherwise set.
There's always been a bit of an inconsistency in the game where enabling
invincibility would make spikes so solid that enemies and moving
platforms would treat spikes as solid and bounces off of them.
This fixes that by adding an `invincible` parameter to collision
functions, so the functions will only treat spikes as solid if that
parameter is true, and the parameter passed will only be true if it's
called for an entity that is a humanoid and invincibility mode is
enabled.
Also, for clarity, `spikecollide` is renamed to `towerspikecollide`
because it's only used for tower spikes. And as a small optimization,
`checktowerspikes` returns early if invincibility mode is enabled.
This is a minor optimization to streamline the experience of Ved
playtesting. Previously, the user would have to wait for all the assets
to load when launching playtesting (most of the time, I suspect, is
taken up by loading music from a vvvvvvmusic blob). With this
optimization, however, the game can be launched in the background and
its assets can be loaded, while it blocks on STDIN input. During this
time, the user in Ved will be choosing where to start playtesting. After
Ved provides STDIN input, then the window will be created and appears
instantaneously.
This also fixes a related issue in which providing an invalid
playtesting level name would result in a brief window flash that gets
instantly destroyed. With this, if the level is invalid then no window
is ever shown at all.
Probably should have done this earlier in 2.3, but better late than
never.
This makes it easier for third-party programs like Ved to detect what
version of the game this is.
Slightly quick-n-dirty for now, I'll de-duplicate the version number
later, and add commit hash and date if applicable.
Without this, entering in-game and opening the map with missing graphics
will result in a segfault. This is because even if the image doesn't
exist, it's still pushed into the `images` std::vector as a NULL
pointer. And it segfaults because we dereference it (to get things like
their width and height). In contrast, passing them to SDL is fine
because SDL always checks for NULL first.
There are three different places where we call PHYSFS_openRead. This
commit makes sure all of them print a statement upon failure along with
the PhysFS reason for failure, and assigns the log level of each print
as so:
- FILESYSTEM_loadFileToMemory: Debug print (previously no
print existed in the first place), because some files (such as
font.txt) may or may not be needed, but if it isn't then no need to
print and worry the user. The game will error anyway if a critical
file like a graphics file is missing.
- FILESYSTEM_loadBinaryBlob: Debug print (previously info print),
because sometimes it's not needed, such as mmmmmm.vvv. I remember one
user being worried that the game printed "Unable to open file
mmmmmm.vvv" when it's not critical unlike vvvvvvmusic.vvv (and that
file is assumed to exist if data.zip exists anyways). Though maybe we
should move to loose-leaf files to save on memory usage (and so we
don't have to use special tools to modify a binary blob)...
- FILESYSTEM_loadZip: Error print. If we're calling this function, we
really do expect the zip to be loaded, and if it fails because we
can't open the file in the first place, then it would be good to know
why.
This commit adds a new string formatting system to replace uses of
`SDL_snprintf` and string concatenation.
Making our own string formatting system has been briefly discussed
during the review of the localization branch, and on the VVVVVV
Discord. It's inspired by Python's format strings, but simpler.
This is primarily to benefit localization - strings will be easier to
understand (`Now using %s Tileset` → `Now using {area} Tileset`,
`"%s remain"` → `"{n_crewmates|wordy} remain"`), translators can change
the word order for their language's grammar (`%1$s` is a POSIX
extension), and this system is also less error-prone (making the format
string not align with the actual arguments won't result in a crash or
UB).
It also integrates our needs better - particularly the "wordy" numbers
without having to have a `help.number_words(n).c_str()` at the
callsite, translators can opt in and out of wordy numbers per string,
and this should also make it easier to solve #859.
This commit adds the formatting system itself, and changes one
`SDL_snprintf` in the code to use it as a small demo (the rest should
probably be done in the localization branch to avoid more unneeded
work).
The system is described in full detail in VFormat.h and in the pull
request description.
This removes the magic numbers previously used for controlling the fade
mode, which are really not readable at all unless you already know what
they mean.
0: FADE_NONE
1: FADE_FULLY_BLACK
2: FADE_START_FADEOUT
3: FADE_FADING_OUT
4: FADE_START_FADEIN
5: FADE_FADING_IN
There is also the macro FADEMODE_IS_FADING, which indicates when the
intention is to only check if the game is fading right now, which wasn't
clearly conveyed previously.
I also took the opportunity to clean up the style of any lines I
touched. This included rewriting if-else chains into case-switches,
turning one-liner if-then statements into proper blocks, fixing up
comments, and even commenting the `fademode == FADE_NONE` on the tower
spike checks (which, it was previously undocumented why that check was
there, but I think I know why it's there).
As for type safety, we already get some by transforming the variable
types into the enum. Assignment is prohibited without a cast. But,
apparently, comparison is perfectly legal and won't even give so much as
a warning. To work around this and make absolutely sure I made all
existing comparisons now use the enum, I temporarily changed it to be an
`enum class`, which is a C++11 feature that makes it so all comparisons
are illegal. Unfortunately, it scopes them in a namespace with the same
name as a class, so I had to temporarily define macros to make sure my
existing code worked. I also had to temporarily up the standard in
CMakeLists.txt to get it to compile. But after all that was done, I
found the rest of the places where a comparison to an integer was used,
and fixed them.
Previously, it was copy-pasted and slightly different, when really, they
ought to both be the exact same code.
It kind of pains me that the room name, glitch name, and hidden name
don't own their own memory, but, that's to be addressed later.
What's a bit annoying is that the `temp` variable used in
`teleporterrender` also ends up being reused later in the function. In
this case, I opted to just redeclare them when they are used anyway, to
make it clearer.
Apart from `teleporterrender` no longer calling `map.area` or caring
about `map.custommode`, it also no longer cares about
`graphics.fademode` being 0. I could never actually get this condition
to be false in practice, and I have absolutely no idea why it's there.
I'm guessing it could be some weird edge case rendering issue if the
screen is fully black? But I wouldn't know how to trigger that, and
anyway it should probably be fixed elsewhere. So I'm just going to
remove that conditional.
It's quite rare, though possible, that during finalstretch you could see
a glitchy tileset that looked like this:
https://i.imgur.com/V7cYKDW.png
This happened because final_mapcol, the variable that controls which
color of finalstretch is rendered, could end up being 7. Normally, it's
in the range of 1..6, which perfectly correlates with the Warp Zone
tilesets in tiles2.png, and the higher the number the farther back in
the tileset it goes from the gray Warp Zone tileset. However, if it's 7,
then it'll start grabbing tiles from the Ship plus some unused blank
tiles, which does not look pretty in the slightest.
This happened because it's possible, though exceedingly unlikely, that
fRandom(), a function which returns a float between 0..1, could return
exactly 1. fRandom() calls rand(), which returns a result between 0 and
RAND_MAX, and divides it by RAND_MAX. This value is implementation
dependent, but required to be at least 32767, and on most systems is
2147483647. Even taking the value of 32767, that means there's a 0.003%
chance that you could get this glitchy tileset when the game cycled the
color in finalstretch. But of course, playing the game for long periods
of time will eventually increase this chance - cycling the color 1,000
times (around 17 minutes of playing) will result in the chance being 3%.
Then as the calculations in the finalstretch color cycling logic calls
fRandom(), then multiplies by 6 and adds 1, it was possible for
fRandom() to return exactly 1, then have
6 added to it, resulting in final_mapcol being 7.
To fix this, just decrement the multiplication by fRandom() to multiply
by 5 instead of 6. Now the only possible numbers that calculation can
produce would be 1..6.
This fixes a limitation where the level filename had to be the exact
same name as the name of the zip, because the game used the name of the
level to identify the zip of which to load assets, and this also made it
impossible to use assets for more than one level in a zip.
Instead, we just look up where the level came from, so we can always
load its assets regardless of its filename.
Additionally, the zip structure checks can go away too, simplifying the
code further.
This WOULD be a huge breaking change, if it weren't for the fact that no
one uses them. Which is why I'm removing them, to simplify the code.
I asked on the VVVVVV Discord whether anyone used them or was even aware
of them and basically the answer was no. I go on Distractionware and no
one uses them. And why would they, when they'd have to distribute the
level .vvvvvv file separately? Better to just distribute everything in
one zip. And it's quite a bit obscure that you have to suffix the file
with .data.zip anyway.
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.
