Looks like musicfade is an unused variable, anyway. I might remove it,
but I have some plans in the future that involve repairing what it was
intended for, so I'll hold off on removing it (and some other unused
variables in Music.cpp) for now.
As discussed earlier, some custom levels have taken advantage of the
fact that songs 0 and 7 loop and also fade in when using PPPPPP while
having an mmmmmm.vvv file present.
The problem is that it would index out-of-bounds if you did this, but
this UB hasn't caused an exception until my change to refactor
script-related vectors by removing their separate length-trackers.
Most of the code was already commented out, and those comments were
removed in earlier commits, but this removes all recording variables
from Game and simplifies the game-gamestate handling in main.cpp a
little bit.
Just a miscellaneous code cleanup.
There's no glitches that take advantage of the previous situation,
namely that 'temp' was a global variable in Logic.cpp and editor.cpp.
Even if there were, it seems like it would easily lead to some undefined
behavior. So it's good to clean this up.
This is the "Behavioral logic", "Basic Physics", and "Collisions with
walls" trio.
They were originally aligned but then I removed global args, thus
misaligning them. So now I'm re-aligning them back again.
Surprisingly not that many. It looks like at one point in development,
damage blocks were created for every single spike in the Tower, before
it was too laggy so they switched to directly checking collision with
the tile instead.
This removes a bunch of commented-out code that was clearly kept from
the Flash version, even though the Flash graphics API is much different
than SDL's. Also removes a bunch of TODOs that either say nothing, or
say something whose meaning has been totally lost to time due to being
completely vague, or something that's already been done and someone
forgot to remove the TODO.
Most of this is telecookie/quickcookie stuff, which was used in the
Flash version, but there's no longer any such thing as a save cookie.
Also one TODO that says to make a function that's now been made.
Unlike, say, the scriptclass i/j/k stuff, these tempstrings are only
purely visual, and there's no known glitches to manipulate them anyway.
Thus I think it's safe to make this cleanup.
It looks like this may have been used earlier in development, judging
from the name, obviously, but right now it seems like it's used as an
error message if a main game level is asked for an invalid room (well,
only two of them - the Lab and Warp Zone). It should probably be
formalized into an error system, if we want to keep teststring, and also
people would never see it anyway because I don't think there's a
reliable and consistent way to trigger loading a non-existent room.
I have seen someone manage to load a non-existent Warp Zone room only
one time, but even then this teststring didn't pop up. So this
teststring doesn't even trigger in the right circumstances.
Also, when it does pop up, as far as I can tell it will stay onscreen,
which is kinda annoying. So I'm just removing this ancient relic from
the code.
To be fair, it was more on the level of entire functions using different
indentation than the surrounding code, but it's not consistent enough
for me to justify leaving it alone.
Looks like this function was created because editorclass::load() takes
in a string by reference, not by value, and thus mutates it afterwards,
so if you passed a string in when you didn't want it to be mutated, bad
things would happen.
However, a better workaround for the above issue would simply to
duplicate the string and pass that string instead, thus the original
string wouldn't be affected.
To reiterate, I just want to remove the mixed indentation that just
randomly pops up in the middle of a file, because it gets annoying.
Thus, the indentation of a particular piece of code should simply match
the surrounding code. And I consider it completely fine that this file
switches from indenting 4-wide spaces to tabs starting from
Graphics::setcol() onwards. I don't think it's worth it to untabify
Graphics::setcol() and below.
This removes all indentation that suddenly switches in the middle of a
function. Most particularly egregious offenses are the ones made by the
person who has 2-wide tabs, but keeps tabbing up to make each
indentation level match up with the 4-wide spaces, so to them (and only
them) it will look just fine, but since by default tabstop is 8-wide,
their lines are pushed off all the way to the right.
This changes something like UtilityClass::String to help.String,
basically. It takes less typing this way, and is a neat effect of having
global args actually be global variables.
Now that it does nothing due to some earlier changes, it's a useless
function that does nothing. (Well, it was already a useless function,
but those earlier changes made it clearer just exactly how useless it
is.) So remove that function and remove all its callsites.
'swfStage' gets set to 'stage' in updategraphicsmode() but... that does
absolutely nothing, because they both contain exactly the same thing.
And these variables aren't referenced anywhere else. So I'm removing
both of these variables.
Although it keeps getting set to true and false in various places, it
never once gets checked, essentially deeming it a variable that's used
but does nothing.
The 'r', 'g', and 'b' arguments do absolutely nothing. Except unlike
Graphics::drawtile(), there's only one version of Graphics::drawtile2(),
so just remove those args and update callers.
The 'r', 'g', and 'b' arguments do absolutely nothing, even though
they're used in the version of Graphics::drawtile() that's more used. So
delete the other version without those extra arguments, and then remove
the extra arguments from the remaining version. And then update callers.
This removes all global args in all functions in titlerender.cpp.
Additionally, all 'dwgfx' has been renamed to 'graphics' in that file
(there are a lot of them, as you might guess).
This commit removes the passing around of global args in the logic
functions. Additionally, all 'dwgfx' has been replaced with 'graphics'
in Logic.cpp.
This removes global arg passing from all functions on editorclass.
Callers have been updated correspondingly. Additionally, all 'dwgfx' has
been replaced with 'graphics' in editor.cpp.
This commit removes all global args from the parameters of each function
on the scriptclass object, and updates all places they are called
accordingly. It also changes all instances of 'dwgfx' to 'graphics' in
Script.cpp.
Interestingly enough, it looks like editor.h depended on Script.h's
class define of the musicclass. I've temporarily placed the class define
in editor.h, but by the end of this patchset it'll be gone.
This removes global args from all functions on the Graphics class.
Callers of those functions in other files have been updated accordingly.
Of course, since Graphics.cpp is already in the Graphics namespace,
I do not need to change all 'dwgfx' to 'graphics' in Graphics.cpp.
This commit removes the global args being passed around from the
function args on the mapclass object, as well as updating all callers in
other files to not have those args. Furthermore, 'dwgfx' has been
renamed to 'graphics' in Map.cpp.
This commit removes all global args from functions on the entityclass
object, and updates the callers of those functions in other files
accordingly (most significantly, the game level files Finalclass.cpp,
Labclass.cpp, Otherlevel.cpp, Spacestation2.cpp, WarpClass.cpp, due to
them using createentity()), as well as renaming all instances of 'dwgfx'
in Entity.cpp to 'graphics'.
I've decided to call dwgfx/game/map/obj/key/help/music the "global args".
Because they're essentially global variables that are being passed
around in args.
This commit removes global args from all functions on the Game class,
and deals with updating the callsites of said functions accordingly. It
also renames all usages of 'dwgfx' in Game.cpp to 'graphics', since the
global variable is called 'graphics' now.
Interesting to note, I was removing the class defines from Game.h, but
it turns out that Graphics.h depends on the mapclass and entityclass
defines from Game.h. And also Graphics.h spelled mapclass wrong (it
forgot the "class") so I just decided to use that existing line instead.
This is only temporary and after all is said and done, at the end of
this pull request those class defines will be gone.
This is a refactor that turns the script-related arrays `ed.sb`, and
`ed.hooklist` into C++ vectors (`script.commands` was already a vector, it was
just misused). The code handling these vectors now looks more like idiomatic
C++ than sloppily-pasted pseudo-ActionScript. This removes the variables
`script.scriptlength`, `ed.sblength`, and `ed.numhooks`, too.
This reduces the amount of code needed to e.g. simply remove something from
any of these vectors. Previously the code had to manually shift the rest of
the elements down one-by-one, and doing it manually is definitely error-prone
and tedious.
But now we can just use fancy functions like `std::vector::erase()` and
`std::remove()` to do it all in one line!
Don't worry, I checked and `std::remove()` is in the C++ standard since at least
1998.
This patch makes it so the `commands` vector gets cleared when
`scriptclass::load()` is ran. Previously, the `commands` vector never actually
properly got cleared, so there could potentially be glitches that rely on the
game indexing past the bounds set by `scriptlength` but still in-bounds in the
eyes of C++, and people could potentially rely on such an exploit...
However, I checked, and I'm pretty sure that no such glitch previously existed
at all, because the only times the vector gets indexed are when `scriptlength`
is either being incremented after starting from 0 (`add()`) or when it's
underneath a `position < scriptlength` conditional.
Furthermore, I'm unaware of anyone who has actually found or used such an
exploit, and I've been in the custom level community for 6 years.
So I think it's fine.
Someone being mean could've overwritten the telesaves of unsuspecting
players, or unlocked a bunch of stuff which they shouldn't have for
those players, using things like the telesave() command and gamestates.
To prevent this, return early in Game::savequick(), Game::savetele(),
and Game::unlocknum() if we are in custommode.
Instead of passing the error codes out of the function, just handle the
errors directly as they happen, and fail gracefully if something goes
wrong instead of continuing.
Whenever you would press Alt+Enter, or Alt+F, or on macOS Command+Enter,
or on macOS Command+F, or F11, the game would print this useless error
message to console, every single time: "Error: failed: " and it would
concatenate SDL_GetError() after it, but most of the time SDL_GetError()
is blank, so it would print just that.
Instead, what the fullscreen shortcut will now do is check the result of
the relevant SDL functions, BEFORE it decides to print an error message.
And when it DOES print an error message, it will be less vague and will
say instead "Error: toggling fullscreen failed: <output of
SDL_GetError()>".
This means Screen::ResizeScreen() and Screen::toggleFullScreen() are now
int-returning functions. Ideally, every function interfacing with SDL
would return an error code, but that's too much for this simple patch.
Additionally, I took the opportunity to clean up the surrounding
formatting of the code a bit, most notably dedenting the
keypress-clearing stuff by one tab level, converting the
shortcut-handling code to spaces, and removing commented-out code.
Each check has been put in its own variable, so the final conditional is
more readable, and the ifdef is no longer right smack in the middle of
an if-statement.
Also the control flow has been changed (the "else" has been removed from
the shortcut-checking conditional) so I could put the variables closer
to the actual conditional itself. I don't think it affects anything,
though.
This patch allows the use of pressing F11 to toggle fullscreen, as well as
allowing the use of Right Command when using the Command+Enter/F shortcut on
macOS. Apparently Alt+Enter isn't the only shortcut to toggle fullscreen,
Alt+F also works, which I didn't know before.
I'm adding F11 as a shortcut because it's a far more natural shortcut to
toggle fullscreen than this Alt+Enter or Alt+F business, which seems to be a
relic mimicking some other games and some Microsoft stuff?
I'm also adding RCommand+Enter/F because I see no reason not to. If you can
use RAlt on non-macOS, why can't you use RCommand on macOS, too?
I also cleaned up the formatting relating to the shortcut code, and made sure
the closing parenthesis was outside the ifdef so my text editor wouldn't
highlight the parenthesis inside the non-macOS ifdef-branch as a dangling
closing parenthesis, because it assumes the one in the branch above is the
actual closing parenthesis and doesn't parse macros.
If you Alt+Tabbed while in fullscreen mode, the game would stay in
fullscreen instead of switching to windowed, but there was a chance it
would EITHER use the same internal resolution which would mismatch the
window resolution (don't know when exactly this happens, but still) and
stay being in an actual windowed mode, OR switch between
fullscreen/windowed every other time you re-focused the window, which is
annoying.
Now, whenever you Alt+Tab in fullscreen, the game will be in windowed
mode, and then when you re-focus it will go back to fullscreen.
Consistently.
Previously, the only way to guarantee that the game actually saved your
unlock.vvv after changing an option, was to ensure you pressed ACTION on
the "quit game" menu option.
This makes Alt+F4 graceful in that pressing it will also save
unlock.vvv, as it should. Now truly UN-graceful ways of exiting the
game, such as SIGSEGV, SIGABRT, or pkill -9 or pkill -15 will not save
unlock.vvv, as should be the case.
Text outline is not drawn on roomtext when you're actually playing the
game, so don't draw the outline in the editor, either.
FIQ mistakenly added text outline to roomtext in
ca9f577fc4.
There's an if-else chain that first deals with figuring out if there's
an entity where your left-click happened, and to do this it uses
edentat(), which returns a sentinel value of -1 if there is NOT an
entity where your cursor is.
It's very important to check that the value returned ISN'T -1 before you
start indexing the 'edentity' vector, since if you DO index it with that
-1, it'll result in Undefined Behavior because you're doing an
out-of-bounds array access.
Now, here's what the if-else chain looked like before:
if(tmp==-1 && ed.free(ed.tilex,ed.tiley)==0)
{
...
}
else if(edentity[tmp].t==1)
The bug here is very subtle but it was an easy oversight. Basically, if
'ed.free' ended up not being zero, control flow would jump to the next
"else if" over, which then ends up asking for the -1th index of
'edentity', which is Undefined Behavior.
This undefined behavior has now resulted in a crash on my system after
TerryCavanagh/VVVVVV#172, due it shuffling things around juuuuust enough
such that this UB would end up resulting in a segfault instead of
chugging along and working fine. For me and my system, this meant that
if my first left-click in the editor upon opening the game was me
placing down a tile and not placing down an entity, the game would
crash. But, it would be fine if I first placed down an entity and then
afterwards placed down tiles, because it's UB.
And I'm almost certain this was the cause of the very strange bug where
you couldn't hold down left-click for the foreground-placing tool (but
you COULD for the background-placing tool) that seemed to occur most
often on Windows (TerryCavanagh/VVVVVV#25).
The solution to this is to stick in another conditional in the tree
before any indexing occurs, such that there's no way any other
conditionals with the indexing in the conditional tree could end up
being hit. In summary, the if-else chain looks like this now:
if(tmp==-1 && ed.free(ed.tilex,ed.tiley)==0)
{
...
}
else if(tmp == -1)
{
//Important! Do nothing, or else Undefined Behavior will happen
}
else if(edentity[tmp].t==1)
This turns the array 'edentity' into a proper vector, and removes the need to
use a separate length-tracking variable and manually keep track of the actual
amount of edentities in the level by using the long-winded
'EditorData::GetInstance().numedentities'. This manual tracking was more
error-prone and much less maintainable.
editorclass::naddedentity() has been removed due to now functionally being the
same as editorclass::addedentity() (there's no more
'EditorData::GetInstance().numedentities' to not increment) and for also being
unused in the first place.
editorclass::copyedentity() has been removed because it was only used to shift
the rest of the edentities up manually, but now that we let C++ do all the
hard work it's no longer necessary.
This refactors the roomtext code to (1) not use ad-hoc objects and (2)
not use a separate length-tracking variable to keep track of the actual
amount of roomtext in a room.
What I mean by ad-hoc object is, instead of formally creating a
fully-fledged struct or class and storing one vector containing that
object, this game instead hacks together an object by storing each
attribute of an object in different vectors.
In the case of roomtext, instead of making a Roomtext object that has
attributes 'x', 'y', and 'text', the 'text' attribute of each is stored
in the vector 'roomtext', the 'x' attribute of each is stored in the
vector 'roomtextx', and the 'y' attribute of each is stored in the
vector 'roomtexty'. It's only an object in the sense that you can grab
the attributes of each roomtext by using the same index across all three
vectors.
This makes it somewhat annoying to maintain and deal with, like when I
wanted add sub-tile positions to roomtext in VVVVVV: Community Edition.
Instead of being able to add attributes to an already-existing
formalized Roomtext object, I would instead have to add two more
vectors, which is inelegant. Or I could refactor the whole system, which
is what I decided to do instead.
Furthermore, this removes the separate length-tracking variable
'roomtextnumlines', which makes the code much more easy to maintain and
deal with, as the amount of roomtext is naturally tracked by C++ instead
of us having to keep track of the actual amount of roomtext manually.
This fixes a source of undefined behavior, where the int returned by
ss_toi() would be random garbage memory if the string passed into it
would be empty. That's because if the string is empty, there are no
characters to parse, so nothing simply gets put into x.
The easiest way to pass an empty string in to ss_toi() would be to use
script commands with empty arguments.
The problem was that the code seemed to be wrongly copy-pasted from the
code for generating the trinket-found text boxes (to the point where
even the comment for the crewmate-found text boxes didn't get changed
from "//Found a trinket!").
For the trinket-found text boxes, they use y-positions 85 and 135 if not
in flip mode, and y-positions 105 and 65 if the game IS in flip mode.
These text boxes are positioned correctly in flip mode.
However, for the crewmate-found text boxes, they use y-positions 85 and
135 if not in flip mode, as usual, but they use y-positions 105 and 135
if the game IS in flip mode. Looks like someone forgot to change the
second y-position when copy-pasting code around.
Which is actually a bit funny, because I can conclude from this that it
seems like the code to position these text boxes in flip mode was
bolted-on AFTER the initial code of these text boxes was written.
I can also conclude (hot take incoming) that basically no one actually
ever tested this game in flip mode (but that was already evident, given
TerryCavanagh/VVVVVV#140, less strongly TerryCavanagh/VVVVVV#141, and
TerryCavanagh/VVVVVV#142 is another flip-mode-related bug which I guess
sorta kinda doesn't really count since text outline wasn't enabled until
2.3?).
So I fixed the second y-position to be 65, just like the y-position the
trinket text boxes use. I even took the opportunity to fix the comment
to say "//Found a crewmate!" instead of "//Found a trinket!".
Previously, when MMMMMM is installed but the user is using PPPPPP, niceplay would still restart the song even if it's the same. That has been fixed. In addition, Plenary and Path Complete no longer loop when MMMMMM is installed but PPPPPP is in use.
This fixes another way you could end up typing on a non-existent line in
the script editor.
In a script with only 1 line, which is empty, the game would let you
press backspace on it, removing the line. This results in you typing on
a non-existent line.
You will keep typing on it until you either close the script or press
Up. If you press Up, you will be unable to get back to the non-existent
line, for it doesn't exist - but the text you typed on the non-existent
line will still be there, until you close the script and re-open it.
This makes the "[Press ENTER to return to editor]" fade out after a few frames, allowing screenshots of custom levels to be cleaner and to make sure nothing is obscured while the user is editing their level.
This commit also adds alpha support in BlitSurfaceColoured, where it takes into account the alpha of the pixel *and* the alpha of the color.
`graphics::getRGBA(r,g,b,a)` was added to help with this.
Following discussion on TerryCavanagh/VVVVVV#153, I suggested that
instead of reverting my M&P guards from TerryCavanagh/VVVVVV#124 (which
would only revert it for The Final Level, The Lab, Overworld, and The
Tower, leaving Space Station 1 & 2 and The Warp Zone alone which could
potentially cause the same problem that motivated
TerryCavanagh/VVVVVV#153), we should initialize the map data with 0s
instead.
It turns out that the line `tstring=tstring[tstring.size()-1];` also appears once in Scripts.cpp.
This causes the game to segfault after activating a terminal with an empty line at the end of it.
I added a quick `if` around this line, and set `tstring` to an empty string when needed.
