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.
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.
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.
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.
This macro is to make it so it won't be error-prone to write the
semi-confusing `(a % b + b) % b` statement, and you can just use an easy
macro instead.
Currently, the only places a positive modulo is needed is when switching
tilesets, enemies, and warp directions in the editor, as well as when
getting a tile in the tower, since towers just repeat themselves
vertically. Towers used this weird while-loop to sort of emulate a
modulo, which isn't half-bad, but is unnecessary, and I don't think any
compiler would recognize it as a modulo. (And if it's not optimized to a
proper modulo... what happens if the number being moduloed is really,
really big?)
Believe it or not, there are still some remnants of the ActionScript
coding standards in the codebase! And one of them sometimes pops up
whenever an integer division happens.
As it so happens, it seems like division in ActionScript automatically
produces a decimal number. So to prevent that, the game sometimes
subtracts off the remainder of the number to be divided before
performing the division on it.
Thus, we get statements that look like
(a - (a % b)) / b
And probably more parentheses surrounding it too, since it would be
copy-pasted into yet another larger expression, because of course it
would.
`(a % b)` here is subtracting the remainder of `a` divided by `b`, using
the modulo operator, before it gets divided by `b`. Thus, the number
will always be divisible by `b`, so dividing it will mathematically not
produce a decimal number.
Needless to say, this is unnecessary, and very unreadable. In fact, when
I saw these for the first time, I thought they were overcomplicated
_modulos_, _not_ integer division! In C and C++, dividing an integer by
an integer will always result in an integer, so there's no need to do
all this runaround just to divide two integers.
To find all of these, I used the command
rg --pcre2 '(.+?).+?-.+?(?=\1).+?%.+?([\d]+?).+?\/.+?(?=\2)'
which basically matches expressions of the form 'a - a % b / b', where
'a' and 'b' are identical and there could be any characters in the
spaces.
These were bfont_rect, bg_rect, foot_rect, and images_rect.
bg_rect was only used once to draw the ghost buffer in the editor, but
that was only because Ally didn't know you could just pass NULL in, cuz
the ghost buffer is the same size as the backbuffer.
getBGR, when used in FillRect, was actually passing colors in RGB order.
But now the masks are fixed, so remove it, and fix up all existing
getBGR colors to use getRGB instead.
Due to the mask inconsistencies, getRGB calls that were passed to
FillRect ended up actually being passed in BGR order. But now that the
masks are fixed, all these BGR colors look wrong. So, fix up all of them
(...that's a _lot_ of copy-pasted code...) to be passed in RGB order.
If it's at all possible to use `const std::string&` when passing
`std::string`s around, then we use it. This is to limit the amount of
memory usage as a result of the frequent use of `std::string`s, so the
game no longer unnecessarily copies strings when it doesn't need to.
This object basically had no reason to exist... it was just more verbose
to use, which really reminded me of Java. Anyway, this is the last thing
named after the editor for no reason when it should be a part of the
customlevelclass, so I moved its attributes to customlevelclass.
This fixes the fact that the name of the singular type is plural, but
the name of the plural array is singular. Which has always annoyed me,
too. Also this makes it more clear that custom entities don't have much
to do with the editor.
That's what it is - it's an entity in a custom level. Not something to
do with the editor, necessarily. Like before, the name of the XML
element will remain the same.
That's what edlevelclass is... so that's what it should be named. (Also
removes that "ed", too, making this less coupled to the in-game editor.)
Unfortunately, for compatibility reasons, the name of the XML element
will still remain the same.
This is a pretty hefty commit! But essentially, I made a new editorclass
object, and moved all functions and variables that only get used in the
in-game level editor to that class. This cleanly demarcates which things
are in the editor and which things are just general custom level stuff.
Then I fixed up all the callers. I also fixed up some NO_CUSTOM_LEVELS
and NO_EDITOR ifdefs, too, in several places.
This accompanies the editor.cpp -> CustomLevels.cpp change; I'll be
splitting out the editor functions in the next commit. The name of the
include guard has been changed as well, but not anything else.
This moves editorrenderfixed(), editorrender(), editorinput(),
editorlogic(), and their associated functions to a new file named
Editor.cpp - which is exactly what it says on the tin; it stores all the
functions related to the actual in-game editor loop. Also, the existing
editor.cpp has been renamed to CustomLevels.cpp.
