This replaces all calls to SDL_free with a new macro, VVV_free, that
nulls the pointer afterwards. This mitigates any use-after-frees and
also completely eliminates double-frees. The same is done for any
function to free specific objects such as SDL_FreeSurface, with the
VVV_freefunc macro.
No exceptions for any of these calls, even if the pointer is discarded
or zeroed afterwards anyway. Better safe than sorry.
This is a macro rather than a function that takes in a
pointer-to-pointer because such a function would have type issues that
require casting and that's just not safe.
Even though SDL_free and other SDL functions already check for NULL, the
macro has a NULL check for other functions that don't. For example,
FAudioVoice_DestroyVoice does not check for NULL.
FILESYSTEM_freeMemory has been axed in favor of VVV_free because it
functionally does the same thing except for `unsigned char*` only.
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.
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.
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.
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.
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.
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.
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.
These warnings are kinda spammy, and they make sense in principle.
vlog_error takes a format string, so passing it an arbitrary string
(even error messages from libraries) isn't a good idea.
In previous versions, the game mistakenly checked the wrong color
channel of sprites, checking the red channel instead of the alpha
channel. I abuse this in some of my levels. Then I broke it when
refactoring masks so the game now no longer checks the red channel but
seems to check the blue channel instead. So re-fix this to the previous
bug, and preserve the previous bug with a comment explaining why.
It now looks more like the FLIP macro in Render.cpp: The y-position is
simply the height of the area the object is being flipped in, minus the
y-position itself, minus the height of the object. So:
flipped_yp = constant - yp - height
This is just a mathematical simplification of the existing statement,
which is:
flipped_yp = yp + 2 * (constant/2 - yp) - height
Using algebra, the 2 distributes into the parentheses, so
flipped_yp = yp + constant - 2 * yp - height
And the two `yp`s add together, so
flipped_yp = constant - yp - height
It's more readable this way.
Also I am using a named constant instead of a hardcoded one.
Otherwise, the text will be in the wrong position compared to normal
mode.
PrintWrap is not used in Flip Mode yet, but it will be used on the map
screen in an upcoming change of mine. The FLIP macro in Render.cpp can't
help us there, since it would need to know the height of the wrapped
text at compile time, when the height is only figured out at runtime
based off of the string (or, well, right _now_ the string _is_ known,
but we are going to merge localization for 2.4, and it's better to
future-proof...), and only PrintWrap itself can figure out the height of
the text. (Or, well, I suppose you could call it from outside the
function, but that's not very separation-of-concernsy style.)
There's really no need to put the y-multiplication in a lookup table.
The compiler will optimize the multiplication better than putting it in
a lookup table will.
To improve readability and to hardcode things less, the new
SCREEN_WIDTH_TILES and SCREEN_HEIGHT_TILES constant names are used, as
well as adding a new TILE_IDX macro to calculate the index of a tile in
a concatenated-rows (row-major in formal parlance) array. Also, tile
numbers are stored in a temporary variable to improve readability as
well (no more copy-pasting `contents[i + vmult[j]]` over and over
again).
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.
RGBflip() does the exact same thing as getRGB(), now that all the
surface masks have been fixed. This axes RGBflip() and changes all
callers to use getRGB() instead. It is more readable that way.
By doing this, there is less copy-pasting. Additionally, it is now
easier to search for RGBf() - which is an ENTIRELY different function
than RGBflip() - now that the name of RGBf is no longer the first four
characters of some different, unrelated function. Previously I would've
had to do `rg 'RGBf[^\w]'` which was stupid and awful and I hated it.
Turns out, the r, g, and b arguments don't actually do anything!
There was a call to RGBf() in the function. RGBf() is just getRGB() but
first adds 128 and then divides by 3 to each of the color channels
beforehand. Unfortunately, RGBf() does not have any side effects, and
the function threw away the return value. Bravo.
This also reveals that the website images drawn in the credits in the
main menu are only recolored because of a stale `ct` set by the previous
graphics.bigprint(), and not because any color values were passed in to
drawimagecol()... What fun surprises the game has in store for me every
day.
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.
This fixes the color ordering of every SDL_Surface in the game.
Basically, images need to be loaded in ABGR format (except if they don't
have alpha, then you use RGB? I'm not sure what's going on here), and
then they will be converted to RGB/RGBA afterwards.
Due to the surfaces actually being BGR/BGRA, the game used to use
getRGBA/getRGB to swap the colors back around to BGRA/BGR, but I've
fixed those too.
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.
I've made a new function, Graphics::do_print(), that does the actual
text printing itself. All the interfaces of the other functions have
been left alone, but now just call do_print() instead.
I also removed PrintOffAlpha() and just calculated the center x-position
in bprintalpha() itself (like bigbprint() does) to make it easier to
de-duplicate code.
Text boxes have `r`, `g`, and `b`, and `tr`, `tg`, and `tb`. `tr`, `tg`,
and `tb` are the real colors of the text box, and `r`, `g`, and `b` are
merely the colors of the text box as the text box's alpha value is
applied to them.
Compare this with, say, activity zones (which are drawn like text boxes
but aren't text boxes): There is `activity_r`, `activity_g`, and
`activity_b`, and when they're drawn they're all multiplied by
`act_alpha`.
So just do the same thing here. Ditch the `tr`, `tg`, and `tb`
variables, and make `r`, `g`, and `b` the new `tr`, `tg`, and `tb`
variables. That way, there's simply less state to have to update
separately. So we can get rid of `textboxclass::setcol()` as well.
All parameters are now made const, to aid in the reader in knowing that
they aren't ever changed.
Useless comments have been removed and been replaced with helpful
comments.
Useless parentheses have been removed.
Spacing has been made consistent.
Declarations and code are no longer mixed.
I'm honestly not too sure why drawcustompixeltextbox ever existed? All
it seemed to do was draw even more horizontal/vertical tiles to finish
any gaps in the tiling... which was all completely unnecessary and
wasteful, because even the previous drawpixeltextbox implementation
covered all gaps in all custom level map sizes that I tried.
Anyway, that at least gets rid of one copy-pasted function.
This draws the remaining horizontal/vertical tile just beside the final
corner if the width/height is not a multiple of 8. (It'd be wasteful to
draw it if the width/height was a perfect multiple of 8, and result in
double-drawing translucent pixels if there were any.)
This has an advantage over the previous system of shifting the
horizontal/vertical tiling, in that custom corner textures don't look
weird due to overlapping like this. Now, custom horizontal/vertical
tiles _can_ look weird if they don't completely tile correctly (or if
they have translucent pixels), but that's better than mucking up the
corners.
`w` and `h` are provided alongside `w2` and `h2`. `w2` and `h2` are in
blocks of 8, while `w` and `h` are in pixels. Therefore, `w2` and `h2`
can just be figured out by diving `w` and `h` by 8.
Also, `xo` and `yo` were used to slide the horizontal/vertical tiling of
the text box a bit into one set of corners, so the horizontal/vertical
tiling wouldn't visibly overlap with the other corners, if using default
textures. This requires hardcoding it for each width/height of text box,
which isn't something that's generalizable. Also, it results in corners
that look weird if the corners have custom textures that don't adhere to
the same shape as default textures.
In the next commit I'll fix the non-multiple-of-8 text box dimensions
differently. Can't do it in this commit or the diff looks weird (at
least with my diff algorithm).
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.
Previously, Flip Mode rendering had to be complicated and allocate
another buffer to call FlipSurfaceVerticle, and it was just a mess.
Instead, why not just do SDL_RenderCopyEx, and let SDL flip the screen
for us? This ends up pretty massively simplifying the rendering code.
Originally this started as a "deduplicate a bunch of duplicated code in script commands" PR,
but as I was working on that, I discovered there's a lot more that needs to be done than
just deduplication.
Anything which needs a crewmate entity now calls `getcrewmanfromname(name)`, and anything which
just needs the crewmate's color calls `getcolorfromname(name)`. This was done to make sure that
everything works consistently and no copy/pasting is required. Next is the fallback; instead of
giving up and doing various things when it can't find a specific color, it now attempts to treat
the color name as an ID, and if it can't then it returns -1, where each individual command handles
that return value. This means we can keep around AEM -- a bug used in custom levels -- by not
doing anything with the return value if it's -1.
Also, for some reason, there were two `crewcolour` functions, so I stripped out the one in
entityclass and left (and modified) the one in the graphics class, since the graphics class also
has the `crewcolourreal` function.
This will wrap text on-the-fly, since I will be introducing text that
needs to be wrapped whose length we can't know in advance. (Or we can,
but, that'd be stupid.)
I took the algorithm from Dav999's localization branch, but it's not
like it's a complicated algorithm in the first place. Plus I think it
actually handles words that get too long to fit on a single line better
than his localization branch. The only difference is that I removed all
the STL, and made it more memory efficient (unlike his localization
branch, it does not copy the entire string to make a version with
newline separator characters).
It's quite rude to close the game. Especially if the user does not use
the console. They won't know why the game closed.
Instead, just return -1. All usages of font_idx() should be and are
bounds checked anyways. This will result in missing characters, but,
it's not like the characters had a font image in the first place,
otherwise we wouldn't be here. And if the user sees a bunch of
characters missing in their font, they'll probably work out what the
problem is even without having a console. And it's still far better than
abruptly closing the game.
And use WHINE_ONCE to prevent spamming the console.
Misa
Dav999-v
Ally