Here's what causes #401: After the fade to menu delay ticks down to 0,
the game calls game.quittomenu(), but the rest of mapinput() still
executes. This means that the block that detects your ACTION press gets
executed, because there's a check that fadetomenudelay is less than or
equal to 0, and, well, it is.
So if you've pressed ACTION on the exact frame that it counts down to 0,
then the game detects your ACTION press, then processes it accordingly,
and then sets the fadetomenudelay, which means it'll get reactivated the
next time you open the map screen. But at this point, you get sent to
TITLEMODE, because game.quittomenu() set game.gamestate accordingly.
(This is why resetting game.fadetomenu or game.fadetomenudelay in
game.quittomenu() or script.hardreset() won't fix this bug.)
The solution here is to add a game.fadetomenu check to the ACTION press
processing.
Same-frame state transition logic is hard... actually, any sort of thing
where two things happen on the same frame is really annoying.
This also applies to fadetolab and fadetolabdelay, too.
Fixes#401.
The game previously did this dumb thing where it lumped in all its
settings with its file that tracked your records and unlocks,
`unlock.vvv`. It wasn't really an issue, until 2.3 came along and added
a few settings, suddenly making a problem where 2.3 settings would be
reset by chance if you decided to touch 2.2.
The solution to this is to move all settings to a new file,
`settings.vvv`. However, for compatibility with 2.2, settings will still
be written to `unlock.vvv`.
The game will prioritize reading from `settings.vvv` instead of
`unlock.vvv`, so if there's a setting that's missing from `unlock.vvv`,
no worries there. But if `settings.vvv` is missing, then it'll read
settings from `unlock.vvv`. As well, if `unlock.vvv` is missing, then
`settings.vvv` will be read from instead (I explicitly tested for this,
and found that I had to write special code to handle this case,
otherwise the game would overwrite the existing `settings.vvv` before
reading from it; kids, make sure to always test your code!).
Closes#373 fully.
This fixes a deltaframe glitch where the "- Press ENTER to Teleport -"
prompt would show up for a split second if you exited the game while the
prompt was fully faded in, and then re-entered it.
cppcheck said: "Logical disjunction always evaluates to true".
Yes. Yes it does. Whoops. I learned how to specify ranges like this in
high school math and still screw it up...
In C++, when you have two variables in different scopes with the same
name, the inner scope wins. Except you have to be really careful because
sometimes they're not (#507). So it's better to just always have unique
variable names and make sure to never clash a name with a variable in an
outer scope - after all, the C++ compiler and standard might be fine
with it, but that doesn't mean humans can't make mistakes reading or
writing it.
Usually I just renamed the inner variables, but for tx/ty in editor.cpp,
I just got rid of the ridiculous overcomplicated modulo calculations and
replaced them with actual simple modulo calculations, because the
existing ones were just ridiculous. Actually, somebody ought to find
every instance of the overcomplicated modulos and replace them with the
actual good ones, because it's really stupid, quite frankly...
Whenever you delete all your save data, your settings aren't changed at
all, and you could resave them if you fiddled with a setting somewhere.
But the full game doesn't count Flip Mode as a setting, instead it
counts it as an unlock. This means deleting your save data would unset
Flip Mode in M&P, which would seem weird because in M&P it's just a
simple setting.
For consistency, Flip Mode shouldn't be unset when deleting save data in
M&P.
This commit fixes a bug that also sometimes occurred in 2.2, where the
teleporter sprite would randomly turn into a solid color and just be a
solid circle with no detail.
Why did this happen? The short answer is an incorrect lower bound when
clamping the teleporter sprite index in `Graphics::drawtele()`. The long
answer is bad RNG with the teleporter animation code. This commit fixes
the short answer, because I do not want to mess with the long answer.
So, here is what would happen: the teleporter's `tile` would be 6. The
teleporter code decrements its `framedelay` each frame. Then when it
reached a `framedelay` of 0, it would call `fRandom()` and essentially
ask for a random number between 0 and 6. If the RNG ended up being
greater than or equal to 4, then it would set its `walkingframe` to -5.
At the end of the routine, the teleporter's `drawframe` ends up being
its `tile` plus its `walkingframe`. So having a `walkingframe` of -5
here is fine, because its `tile` is 6.
Until it isn't. When its `tile` becomes 2, it still keeps its
`walkingframe` around. The code that runs when its `tile` is 2 does have
the possibility of completely resetting its `walkingframe` to be in
bounds (in bounds after its `tile` is added), but that only runs when
its `framedelay` is 0, and in the meantime it'll just use the previous
`walkingframe`.
So you could have a `walkingframe` of -5, plus a `tile` of 2, which
produces a `drawframe` of -3. Then `Graphics::drawtele()` will clamp
that to 0, which just means it'll draw the teleporter backing, and the
teleporter backing is a simple solid color, so the teleporter will end
up being completely and fully solid.
To fix this, I just made `Graphics::drawtele()` clamp to 1 on the lower
bound, instead of 0. So if it ever gets passed a negative teleporter
index, it'll just draw the normal teleporter sprite instead, which is
better.
This fixes the draw order by drawing all other entities first, before
then drawing all humanoids[1] after, including the player afterwards.
This is actually a regression fix from #191. When I was testing this, I
was thinking about where get a crewmate in front of another entity in
the main game, other than the checkpoints in Intermission 1. And then I
thought about the teleporters, because I remember the pre-Deep Space
cutscene in Dimension Open looking funny because Vita ended up being
behind the teleporter. (Actually, a lot of the cutscenes of Dimension
Open look funny because of crewmates standing behind terminals.)
So then I tried to get crewmates in front of teleporters. It actually
turns out that you can't do it for most of them... except for Verdigris.
And then that's what I realized why there was an oddity in WarpClass.cpp
when I was removing the `active` system from the game - for some reason,
the game put a hole in `obj.entities` between the teleporter and the
player when loading the room Murdering Twinmaker. In a violation of
Chesterton's Fence (the principle that you should understand something
before removing it), I shrugged it off and decided "there's no way to
support having holes with my new system, and having holes is probably
bad anyway, so I'm going to remove this and move on". The fact that
there wasn't any comments clarifying the mysterious code didn't help
(but, this *was* 2.2 code after all; have you *seen* 2.2 code?!).
And it turns out that this maneuver was done so Verdigris would fill
that hole when he got created, and Verdigris being first before the
teleporter would mean he would be drawn in front of the teleporter,
instead of being behind it. So ever since
b1b1474b7b got merged, there has actually
been a regression from 2.2 where Verdigris got drawn behind the
teleporter in Murdering Twinmaker, instead of properly being in front of
it like in 2.2 and previous.
This patch fixes that regression, but it actually properly fixes it
instead of hacking around with the `active` system.
Closes#426.
[1]: I'm going to go on a rant here, so hear me out. It's not explicitly
stated that the characters in VVVVVV are human. So, given this
information, what do we call them? Well, the VVVVVV community (at least
the custom levels one, I don't think the speedrunning community does
this or is preoccupied with lore in the first place) decided to call
them "villis", because of the roomname "The Villi People" - which is
only one blunder in a series of awful headcanons based off of the
assumption that the intent of Bennett Foddy (who named the roomnames)
was to decree some sort of lore to the game. Another one being
"Verdigris can't flip" because of "Green Dudes Can't Flip". Then an OC
(original character) got named based off of "The Voon Show" too. And so
on and so forth.
"Humanoid" is just a word for "crewmate or player" but without having to
say "crewmate or player". This is just to make it so humanoids get drawn
after all other entities get drawn, meaning humanoids will be drawn on
top.
I'm going to refactor drawing an entity out to a separate function, and
since I'm going to do that, I might as well make some things const to
clarify intent first and foremost and possibly improve performance or
compiler optimization.
My previous custom level forwards compatibility would only work if you
saved to the same filename as you read from. But what if you saved to a
new filename? Well, your extra XML is lost.
Not to worry, I've introduced a variable that remembers the filepath of
the currently-loaded level (the existing `filename` attribute is kind of
weird and not really suited for this purpose, so). I tried to make it a
simple `const char*`, but it turns out that's bad when you take the
`c_str()` of an `std::string` that then gets destroyed, so it's best to
use `std::string` in an `std::string` world.
So now when you save a level, it'll attempt to open the original file,
and if that doesn't exist then your extra XML gets lost anyway, but I
shouldn't be expected to keep your XML if you delete the original file.
Custom level files now have forwards compatibility - except that some
XML objects will simply discard contents and attributes they don't see
fit. For instance, edentities and level properties will not preserve
newer attributes or contents.
This is because preserving them would require having to track the XML
object as part of the edentity internally, because the edentity might
change places or even be deleted throughout the course of editing
someone's level.
I opted to not add support for preserving objects like these, because
frankly, the put-everything-in-one-file level format was and still is a
terrible idea, and we should probably switch to a new format in the
future that isn't single-file. So when that happens, there won't be a
need to preserve XML attributes on edentities.
When I encountered this function, I asked myself, why make a dedicated
function instead of casting to int?
Well, as it turns out, you can't just cast to int, because you have to
convert it to a string by doing help.String(number).c_str(), like all
the other ints. So it's actually more wasteful to do it the normal way
instead of using BoolToString().
That is, until my recent changes came along and made it so that you can
just provide an int and it'll automatically be converted to a string, no
questions asked. Now, it's more optimal to do a straight cast to int
instead of having to go through a middleman function. So this function
is getting axed in favor of casting to int instead.
This file is probably the biggest one, as there will be more settings
added in the future and we don't want people's settings to be erased. Of
course, this file will be migrated to a settings.vvv sometime later in
2.3 in order to prevent 2.2 from erasing 2.3 settings.
These XML functions will be useful for de-duplicating copy-pasted XML
handling code, while also making it so elements will get updated in
place instead of being thrown out and starting from scratch every time a
file is saved.
Now that tower, title, and horizontal/veritcal warp backgrounds all use
separate buffers, there's no longer any need to temporarily store
variables as a workaround for the buffers stepping on each other.
Instead of using the same tower buffer that gets used for towers, use a
separate buffer instead so there's no risk of stepping on the tower
buffer's toes at the wrong point in time.
This commit combined with the previous one fixes#369.
With the previous commit in place, we can now simply move some usages of
the previous towerbg to use a separate object instead. That way, we
don't have to mess with a monolithic state, or some better way to phrase
what I just said, and we instead have two separate objects that can
coexist side-by-side.
Previously, the tower background was controlled by a disparate set of
attributes on Graphics and mapclass, and wasn't really encapsulated. (If
that's what that word means, I don't particularly care about
object-oriented lingo.) But now, all relevant things that a tower
background has has been put into a TowerBG struct, so it will be easy to
make multiple copies without having to duplicate the code that handles
it.
Yet another set of temporary variables is on a global class when they
shouldn't be. These two are only used in tower background functions and
are never used anywhere else, so they're clearly temporaries.
For some reason, this `tl` is a `point`? But the only other time the
name `tl` is used elsewhere in the code is a float on a `textboxclass`.
Regardless, this is unused.
This function was only used in assigments to mapclass::towercol. But
that variable is unused, and has been removed, so after removing that
variable, this one is unused, too.
On the deltaframes of the tower background, there would be "pixel bleed"
if the tower background would be scrolling from the top. This is because
there wouldn't be any more pixels from above the screen to grab, because
the background rendering functions didn't draw any pixels above the
screen. But they couldn't draw any pixels above the screen, because that
was simply the end of the buffer. But now that the buffer is expanded,
we can now draw above the screen, and fix this glitchy interpolation
rendering.
In order to fix the weird title screen pixels at the top on deltaframes,
we'll need to have a bit more space at the top. Also to the left, in
case we need a background to scroll from the left in the future.
These commands now use the INBOUNDS_ARR() macro to convey intent, and to
make sure that if the size of the array changes in the future, that the
bounds check wouldn't end up being wrong. Also fixed some code style for
the flag() and ifcrewlost() commands.
Scripting crewmates apparently have a specific hardcoded rule in their
follow-player code that makes it so if they're in (10,5) and are to the
left of the line x=155, they will refuse to continue following the
player. This was clearly done to make it so Vitellary in the main game
wouldn't overlap the teleporter, and that was clearly done to make it so
it wouldn't look like he would go behind the teleporter, which would
look weird (I also noticed this in #513).
I stumbled across this code and thought that just like other weird
main-game code that shouldn't apply in custom levels (#136, #137, #144),
this should be fixed, too.
The window icon is simply another asset that can be customized by level
makers. And in fact, one of my levels changes the window icon. It's
simply named VVVVVV.png, but it doesn't sit in the graphics folder,
rather it sits in the root VVVVVV directory.
I noticed that this asset was missed when per-level assets loading was
added, so I decided to add it in.
There's a NULL check on screenbuffer because reloadresources() gets
called before screenbuffer's init() does.
The intent of #504 was to make it so oldxp/oldyp would never be messed
with for over-30-FPS stuff, but I forgot that I changed these
assignments in my over-30-FPS patch when I was doing #504. So these
assignments have been restored to the way they were in 2.2, and is
fixed now.
While my previous commit fixes the glitchy y-position when you get stuck
inside a conveyor, I noticed that getting inside a conveyor seems to
always push the player out, despite conveyors sharing the same code with
moving platforms, which has code to temporarily disable their own
collision when the player gets stuck inside them, so that the player
DOESN'T get pushed out.
Well, it turns out that the reason this happens is because conveyors in
a room that get placed during mapclass::loadlevel() get tile 1 placed
underneath them. This is mostly so moving platforms will collide with
them, because otherwise platforms don't collide with other platforms,
and a conveyor is considered a platform.
This means that a conveyor without any tiles behind it will simply get
the player stuck if they get inside it, and the player won't be pushed
out. This is bad, because conveyors don't move, so they'll be stuck
there forever until they press R (or save, quit, and load). This
situation doesn't come up in the main game, but it COULD come up in
custom levels that use the internal createentity() command to create
conveyors that don't have any tiles behind them.
It seems good to fix this as well, while we're at it fixing conveyor
physics, so I'm fixing this as well.
There is this issue with conveyors where if you collide with them, your
intended next y-position doesn't get updated to the position of the
conveyor, and then your y-position gets set to your intended next
y-position. This also applies to horizontally moving platforms.
This bug used to not produce any problems, if at all, until #502 got
merged. Since then, respawning from checkpoints that are on conveyors
would sometimes not update your y-position at all, making it possible to
get stuck inside a death loop that would require you to exit the game
and re-enter it.
But you can always reliably create this bug simply by going into the
editor and placing down a conveyor and checkpoint on top of each other.
Then enter and exit playtesting a bunch of times, and you'll notice the
glitchy y-position Viridian keeps taking on.
The root cause of this is how the game moves the player whenever they
stand on the top or bottom of a conveyor (or a horizontally moving
platform). The game sets their intended next x-position (newxp), then
calls obj.entitymapcollision() on them. This would be okay, except their
intended next y-position (newyp) doesn't get set along with their newxp,
so entitymapcollision() will use the wrong newyp, then find that there
is nothing that will collide with the player at that given newyp, then
update the yp of the player to the wrong newyp.
So, the platform logic simply doesn't set the player's newyp. Why does
the player have the wrong newyp? It's because moving platforms (and
conveyors) are updated first before all other entities are, and this
includes the code that checks the player for collisions with moving
platforms. That's right: the moving platform collision code gets ran
before the player properly gets updated. This means that the game will
use the newyp of the previous frame, which could be anything, really,
but it most likely just means the intended next y-position of the player
gets canceled, leaving the player with the same y-position they had
before.
Okay, but this bug only seems to happen when you put a checkpoint inside
a conveyor (or a moving platform that hasn't started moving yet) and
start playtesting from it, so why doesn't this bug happen more often,
then? Well, it's probably because of luck and coincidence. Most of the
time, if you're colliding with a conveyor or horizontally moving
platform, you probably have a correct newyp from the previous frame of
the game, so there'd be no problems. And before #502 got merged, this
previous frame would be provided by the player having to fall to the
surface due to the y-offset of their savepoint. However, if you make it
so that you immediately teleport on to a conveyor (because you died),
then this bug will rear its ugly head.
I got this warning during compilation because there were two nested for
loops both defining i. Better to have different names to make sure some
compilers won't overwrite the outer variable with the inner one.
I was investigating a desync in my Nova TAS, and it turns out that
the gravity line collision functions check for the `oldxp` and `oldyp`
of the player, i.e. their position on the previous frame, along with
their position on the current frame. So, if the player either collided
with the gravity line last frame or this frame, then the player collided
with the gravity line this frame.
Except, that's not actually true. It turns out that `oldxp` and `oldyp`
don't necessarily always correspond to the `xp` and `yp` of the player
on the previous frame. It turns out that your `oldyp` will be updated if
you stand on a vertically moving platform, before the gravity line
collision function gets ran. So, if you were colliding with a gravity
line on the previous frame, but you got moved out of there by a
vertically moving platform, then you just don't collide with the gravity
line at all.
However, this behavior changed in 2.3 after my over-30-FPS patch got
merged (#220). That patch took advantage of the existing `oldxp` and
`oldyp` entity attributes, and uses them to interpolate their positions
during rendering to make everything look real smooth.
Previously, `oldxp` and `oldyp` would both be updated in
`entityclass::updateentitylogic()`. However, I moved it in that patch to
update right before `gameinput()` in `main.cpp`.
As a result, `oldyp` no longer gets updated whenever the player stands
on a vertically moving platform. This ends up desyncing my TAS.
As expected, updating `oldyp` in `entityclass::movingplatformfix()` (the
function responsible for moving the player whenever they stand on a
vertically moving platform) makes it so that my TAS syncs, but the
visuals are glitchy when standing on a vertically moving platform. And
as much as I'd like to get rid of gravity lines checking for whether
you've collided with them on the previous frame, doing that desyncs my
TAS, too.
In the end, it seems like I should just leave `oldxp` and `oldyp` alone,
and switch to using dedicated variables that are never used in the
physics of the game. So I'm introducing `lerpoldxp` and `lerpoldyp`, and
replacing all instances of using `oldxp` and `oldyp` that my over-30-FPS
patch added, with `lerpoldxp` and `lerpoldyp` instead.
After doing this, and applying #503 as well, my Nova TAS syncs after
some minor but acceptable fixes with Viridian's walkingframe.
This commit restores the evaluation order of moving platforms and conveyors to
be what it was in 2.2. The evaluation order changed in 2.3 after the patchset
to improve the handling of the `obj.entities` and `obj.blocks` vectors (#191).
By evaluation order, I'm talking about the order in which platforms and
conveyors will be evaluated (and thus will take priority) if Viridian stands
on both a conveyor or platform at once, and they either have different speeds
or are pointing in different directions. Nowhere in the main game is there a
place where you can stand on two different conveyors/platforms at once, so
this is solely within the territory of custom levels, which is my specialty.
So what caused this evaluation order to change? Well, every moving platform
and conveyor in the game is actually made up of two objects: an entity, and a
block. The entity is the part that moves around, and the block is the part
that actually has the collision.
But if the entity is the part that moves around, and entities and blocks are
in entirely separate vectors, how is the block part going to move along with
it? Well, maybe you'd guess some sort of unique ID system, but spend some time
digging around the code and you won't find any trace of any (there's no
attribute on an entity to store such an ID, for starters).
Instead, what the game does is actually remove all blocks that coincide with
the exact top-left corner of the entity, and then create a new one. Destroying
and creating blocks like this all the time is hugely wasteful, but hey, it
worked.
So why did the evaluation order change in 2.3? Well, to understand that,
you'll need to understand 2.2's `active` system. Instead of having an object
be real simply by virtue of it existing, 2.2 had this system where the object
was only real if it had its `active` attribute set to true. In other words,
you would be looking at a fake object that didn't actually exist if its
`active` attribute was false.
On the surface, this doesn't seem that bad. But this can lead to "holes" in a
given vector of objects. A hole is simply an inactive object neighbored by
active objects (or the inactive object could be the first one in the vector,
but then have an active object immediately following it).
If you have a vector of 3 objects, all of them active, then removing the
second one will result in the vector containing an active object, followed by
an inactive object, followed by an active one. However, since the switch to
more properly use vectors instead of relying on this `active` system, there's
no longer any way for holes to exist in a vector. Properly removing an object
from a vector will just shift the rest of the objects down, so if we remove
the second object after the vector fix, then this will simply move the third
object into the slot of where the second object used to be.
So, what happens if you destroy a block and then create a new one in the
`active` system? Let's say that your `obj.blocks` looks like this, and here
I'm denoting each block by writing out its coordinates:
[30,60] [70,90] [80,100]
and that you want to update the position of the second one, because the entity
that that blocks belongs to has been updated. Okay, so, you delete that block,
which then makes things look like this:
[30,60] [-] [80,100]
and then afterwards, you create a new block with the updated position,
resulting in this:
[30,60] [74,90] [80,100]
Since `entityclass::createblock()` will find the first block slot that has a
false `active` attribute, it puts the new object in the same slot as the old
one. What has been essentially done here is that the slot of the block has
basically been reserved for the new block with the new position. Here, the
evaluation order of each block will stay the same.
But then 2.3 comes along and changes things up. So we start with an
`obj.blocks` like this again:
[30,60] [70,90] [80,100]
and we want to update the second block, like before. So we remove the second
block, resulting in this:
[30,60] [80,100]
It should be obvious that unlike before, where the third block stayed in the
third slot, the third block has now been moved to the second slot. But
continuing on; we are now going to create the new block with its updated
position, resulting in this:
[30,60] [80,100] [70,90]
At this point, we can see that the evaluation order of these blocks has been
changed due to the fact that the third block has now been moved to the slot
that was previously the slot of the second block.
So what can we do about this? Well, we can basically emulate what VVVVVV did
in 2.2, which is disable a block without actually removing it - except I'm not
going to reintroduce an `active` attribute or anything. I'll disable the
collision of all blocks at a certain position by setting their widths and
heights to 0, and then re-enable them later by finding the first block at that
same position, updating its position, and re-assigning its width and height
again.
The former is what `entityclass::nocollisionat()` does; the latter is what
`entityclass::moveblockto()` does. The former mimicks turning off the `active`
attribute of all blocks sharing a certain top-left corner; the latter mimicks
creating a new block - and it will only do this for one block, because
`entityclass::createblock()` in 2.2 only looked for the first block with a
false `active` attribute.
Now, some quirks relied on the previous behavior of destroying and creating
blocks, but all of these quirks have been preserved with the way I implemented
this fix.
The first quirk is that platforms passing through 0,0 will destroy all spike
hitboxes, script boxes, activity zones, and one-way hitboxes in the room. The
hitboxes of moving platforms, disappearing platforms, 1x1 quicksand, and
conveyors will not be affected.
This is a consequence of the fact that the former group uses the `x` and `y`
of their `rect`, while the latter group uses the `xp` and `yp` attributes. So
the `xp` and `yp` of the former are both 0. Meaning, a platform passing
through 0,0 destroys them all.
Having these separate coordinates seems like an artifact from the Flash days.
(And furthermore, there's an unused `x` and `y` attribute on all blocks,
making for technically three separate sets of coordinates! This should
probably be cleaned up, except for what I'm about to say...) But actually, if
you merge both sets of coordinates into one, this lets moving platforms
destroy script boxes and activity zones if it passes through the top-left
corner of them, which is probably far worse than the destruction being
localized to a specific coordinate that would never likely be reached
normally.
This quirk is preserved just fine without any special-casing, because instead
of destroying all blocks at 0,0, they just get disabled, which does the same
job. This quirk seems trivial to fix if I made it so that the position of a
platform's block was updated instantaneously instead of having one step to
disable it and another step to re-enable it, but I aim to preserve as much
quirks as possible.
The second quirk is that a moving platform passing through the top-left corner
of a disappearing platform or 1x1 quicksand will destroy the block of that
disappearing platform. This is because, again, when a moving platform updates,
it destroys all blocks at its previous position, not just only one block. This
is automatically preserved because this commit just disables the block of the
disappearing platform instead of removing it. Just like the last one, this
quirk seems extremely trivial to fix, and this time by simply making it so
`entityclass::nocollisionat()` would have a `break` statement, i.e. only
disabling the first block it finds instead of all blocks it finds, but I want
to keep all quirks that are possible to keep.
The last quirk is that, apparently, in order to prevent pushing the player
vertically out of a moving platform if they get inside of one, the game
destroys the block of the moving platform. If I had missed this edge case,
then the block would've been destroyed, leaving the moving platform with no
collision. But I caught it in my testing, so the block gets disabled instead
of destroyed. Also, it seems obtuse for those who don't understand why a
platform's block gets destroyed or disabled whenever the player collides with
it in `entityclass::collisioncheck()`, so I've put up a comment documenting it
as well.
The different platform evaluation order desyncs my Nova TAS, but after
applying this patchset and #504, my TAS syncs fine (save for the different
walkingframe from starting immediately on the ground instead of in the air
(#502), but that's minor and can be easily fixed).
I've attached a test level to the pull request for this commit (#503) to
demonstrate that this patchset not only fixes platform evaluation order, but
preserves some bugs and quirks with the existing block system.
The first room demonstrates the fixed platform evaluation order, by stepping
on the conveyors that both point into each other. In 2.2, Viridian will move
to the right of the background pillar, but in 2.3, Viridian will move to the
left of the pillar. However, after applying this patch, Viridian will now move
to the right of the pillar once again.
The second room demonstrates that the platform-passing-through-0,0 trick still
works (as explained above).
The last room demonstrates that platforms passing through the top-left corners
of disappearing platforms or 1x1 quicksand will remove the blocks of those
entities, causing Viridian to immediately pass through them. This trick is
still preserved after my patchset is applied.
Previously, setting the actual volume of the music was all over the
place. Which isn't bad, but when I added being able to press N to mute
the music specifically, I should've made it so that there would be a
volume variable somewhere that the game looks at if the music is
unmuted, and otherwise sets the actual volume to 0 if the game is muted.
This resulted in things like #400 and #505 and having to add a bunch of
special-cased checks like game.musicmuted and game.completestop. But
instead of adding a bunch of special-case code, just make it so there's
a central place where Mix_VolumeMusic() actually gets called, and if
some piece of code wants to set the music volume, they can set
music.musicVolume. But the music handling logic in main.cpp gets the
final say on whether to listen to music.musicVolume, or to mute the game
entirely.
This is how the music handling code should have been from the start
(when pressing N to mute the music was added).
Fixes#505.
The value of the macro might not change in the future, but it's there
for a reason. That reason being to improve code readability, because
otherwise 128 would just be a magic number that plopped in out of
nowhere. Sometimes the game uses MIX_MAX_VOLUME, other times it uses
128, so to be consistent I'm just going to enforce MIX_MAX_VOLUME
entirely.
This reverts commit cf5ad166e3.
My implementation will make it so single-case patches like this commit
won't be necessary anymore (there's no need to add a special-case check
for game.musicmuted, the way that I'm gonna do it). In fact, it's better
if I just revert the commit entirely.
It seems like the start point of a custom level and all checkpoints in
the game end up putting your spawn point one pixel away from the surface
it touches, which seems like an oversight. I'm going to enforce some
consistency here and make it so that all spawn points, whenever you
start from a start point or a checkpoint, will always be correctly
positioned flush with the surface they're standing on, and not one pixel
more or less than that.
An exotic checkpoint is a checkpoint with a sprite that is neither the
flipped checkpoint nor unflipped checkpoint. More specifically, it's a
checkpoint whose edentity p1 attribute is something other than 0 or 1.
Normally, whenever you touch an exotic checkpoint in-game, your
savepoint's y-position and gravitycontrol don't get touched. However, in
the editor, spawning from an exotic checkpoint means that your
gravitycontrol gets set to a value that is neither 0 nor 1. In this
invalid gravitycontrol state, Viridian is treated like they're flipped,
but they cannot unflip.
This is an inconsistency between the editor and in-game, so I'm fixing
it. Now, spawning from an exotic checkpoint in the editor will just set
your gravitycontrol to 0, i.e. unflipped.
So, 77a636509b fixed the fact that you
only got 1 frame of onground/onroof when standing on a vertical moving
platform, but removing those lines entirely means that it takes 1 frame
before the onground/onroof of 2 actually takes effect. This desyncs my
Nova TAS, so it seems important to fix.
The onroof/onground attributes are used to determine if the player is
standing on a surface and is eligible to flip. Most notably, it is an
integer and not a boolean, and it starts at 2, giving the player 2
frames to edge-flip, i.e. they can still flip 2 frames after walking off
an edge.
However, these attributes are unnecessarily reassigned in
movingplatformfix() (which is the function that deals exclusively with
vertically-moving platforms; horizontal moving platforms get their own
hormovingplatformfix()). Whoever wrote this misunderstood what
onroof/onground meant; they thought that they were booleans, and so set
them to true, instead of the proper value of 2. This ends up setting
onroof/onground to 1 instead of 2, causing a discrepancy with vertical
moving platforms and the rest of the surfaces in the game.
The bigger mistake here is duplicating code that never needed to be
duplicated. The onroof/onground assignment in gamelogic() works
perfectly fine for vertical moving platforms. Indeed, after testing it
with libTAS, I can confirm that removing the duplicate assignments
restores being able to edge-flip off of moving platforms with 2 frames
of leeway, instead of only 1 frame. It also doesn't change how long it
takes for the onroof/onground to get set when the player is recognized
as standing on a vertically-moving platform, either.
And so, it's better to not duplicate this code, because when you
duplicate it you run the risk of making a mistake, as I just
demonstrated.
By "unnecessary qualifiers to self", I mean something like using the
'game.' qualifier for a variable on the Game class when you're inside a
function on the Game class itself. This patch is to enforce consistency
as most of the code doesn't have these unnecessary qualifiers.
To prevent further unnecessary qualifiers to self, I made it so the
extern in each header file can be omitted by using a define. That way,
if someone writes something referring to 'game.' on a Game function,
there will be a compile error.
However, if you really need to have a reference to the global name, and
you're within the same .cpp file as the implementation of that object,
you can just do the extern at the function-level. A good example of this
is editorinput()/editorrender()/editorlogic() in editor.cpp. In my
opinion, they should probably be split off into their own separate file
because editor.cpp is getting way too big, but this will do for now.
This is a refactor that simply moves all temporary variables off of
entityclass, and makes it so they are no longer global variables. This
makes the resulting code easier to understand as it is less entangled
with global state.
These attributes were:
- colpoint1
- colpoint2
- tempx
- tempy
- tempw
- temph
- temp
- temp2
- tpx1
- tpy1
- tpx2
- tpy2
- temprect
- temprect2
- x (actually unused)
- dx
- dy
- dr
- px
- py
- linetemp
- activetrigger
- skipblocks
- skipdirblocks
Most of these attributes were assigned before any of the times they were
used, so it's easy to prove that ungloballing them won't change any
behaviors. However, dx, dy, dr, and skipblocks are a bit more tricky to
analyze. They relate to blocks, with dx, dy, and dr more specifically
relating to one-way tiles. So after some testing with the quirks of
one-way tiles, it seems that the jankiness of one-way tiles haven't
changed at all, either.
Unfortunately, the attribute k is clearly used without being assigned
beforehand, so I can't move it off of entityclass. It's the same story
with the attribute k that Graphics has, too.
This prevents users from being confused whenever they type a pipe in the
script editor, then save the level and load it again and see their
script lines unexpectedly splitting in two. Now if you attempt to type a
pipe, it simply won't happen at all.
Fixes#379.
It's possible that SDL_atoi() could call the libc atoi(), and if a
string is provided that's too large to fit into an integer, then that
would result in undefined behavior. To avoid this, use SDL_strtol()
instead.
Instead of copying to a temporary string, just use SDL_strncmp(). Also,
I checked the blame, and apparently I committed the line that used
strcmp() instead of SDL_strcmp(), for whatever reason. But that's fixed
now.
For some reason, the variable `k` is on entityclass and gets mutated in
createentity() and createblock(). Then updateentities() uses it without
checking if it's valid, because either `k` or the size of `entities`
could have changed in the meantime. To fix any potential undefined
behavior, these bounds checks should be added.
This fixes a bug where font_positions wouldn't get cleared after exiting
a custom level that had a font.txt if it didn't exist in the default
graphics, leading to messed-up-looking font rendering.
This bug was originally discovered by Ally.
You're intended to rescue Violet first, and not second, third, or
fourth, and especially not last.
If you rescue her second, third, or fourth, your crewmate progress will
be reset, but you won't be able to re-rescue them again. This is because
Vitellary, Verdigris, Victoria, and Vermilion will be temporarily marked
as rescued during the `bigopenworld` cutscene, so duplicate versions of
them don't spawn during the cutscene, and then will be marked as missing
later to undo it.
This first issue can be trivially fixed by simply toggling flags to
prevent duplicates of them from spawning during the cutscene instead of
fiddling with their rescue statuses.
However, there is still another issue. If you rescue Violet last, then
you won't be warped to the Final Level, meaning you can't properly
complete the game. This can be fixed by adding a `crewrescued() == 6`
check to the Space Station 1 Level Complete cutscene. There is
additionally a temporary unrescuing of Violet so she doesn't get
duplicated during the `bigopenworld` cutscene, and I've had to move that
to the start of the `bigopenworld` and `bigopenworldskip` scripts,
otherwise the `crewrescued() == 6` check won't work properly.
I haven't added hooks for Intermission 1 or 2 because you're not really
meant to play the intermissions with Violet (but you probably could
anyway, there'd just be no dialogue).
Oh, and the pre-Final Level cutscene expects the player to already be
hidden before it starts playing, but if you rescue Violet last the
player is still visible, so I've fixed that. But there still ends up
being two Violets, so I'll probably replace it with a special cutscene
later that's not so nonsensical.
I have no idea why neither conveyors and moving and disappearing
platforms rendered in the editor or in-game use
Graphics::drawentcolours(), but this needs to be bounds-checked just as
I did for the in-game rendering function.
The entity getters I'm referring to are entityclass::getscm(),
entityclass::getlineat(), entityclass::getcrewman(), and
entityclass::getcustomcrewman().
Even though the player should always exist, and the player should always
be indice 0, I wouldn't want to make that assumption. I've been wrong
before.
Also, these functions returning 0 lull you into a false sense of
security. If you assume that commands using these functions are fine,
you'll forget about the fact that `i` in those commands could be
potentially anything, given an invalid argument. In fact, it's possible
to index createactivityzone(), flipgravity(), and customposition()
out-of-bounds by setting `i` to anything! Well, WAS possible. I fixed it
so now they can't.
Furthermore, in the game.scmmoveme block in gamelogic(), obj.getplayer()
wasn't even checked, even though it's been checked in all other places.
I only caught it just now because I wanted to bounds-check all usages of
obj.getscm(), too, and that game.scmmove block also used obj.getscm()
without bounds-checking it as well.
When this is done, there is potential for a mistake to occur when
writing out the bounds check, which is eliminated when using the macro
instead. Luckily, this doesn't seem to have happened, but what's even
worse is I hardcoded 400 instead of using SDL_arraysize(ed.level), so if
the size of ed.level the bounds checks would all be wrong, which
wouldn't be good. But that's fixed now, too.
This is because if they are manually written out, they are more likely
to contain mistakes.
In fact, after further review, there are several functions with
incorrect manually-written bounds checks:
* entityclass::entitycollide()
* entityclass::removeentity()
* entityclass::removeblock()
* entityclass::copylinecross()
* entityclass::revertlinecross()
All of those functions forgot to do 'greater than or equal to' instead
of 'greater than' when comparing against the size of the vector. So they
were erroneous. But they are now fixed.
It's better to do INBOUNDS_VEC(i, obj.entities) instead of 'i > -1'.
'i > -1' is used in cases like obj.getplayer(), which COULD return a
sentinel value of -1 and so correct code will have to check that value.
However, I am now of the opinion that INBOUNDS_VEC() should be used and
isn't unnecessary.
Consider the case of the face() script command: it's not enough to check
i > -1, you should read the routine carefully. Because if you look
closely, you'll see that it's not guaranteed that 'i' will be initialized
at all in that command. Indeed, if you call face() with invalid
arguments, it won't be. And so, 'i' could be something like 215, and
that would index out-of-bounds, and that wouldn't be good. Therefore,
it's better to have the full bounds check instead of checking only one
bounds. Many commands are like this, after some searching I can also
name position(), changemood(), changetile(), changegravity(), etc.
It also makes the code more explicit. Now you don't have to wonder what
-1 means or why it's being checked, you can just read the 'INBOUNDS' and
go "oh, that checks if it's actually inbounds or not".
For some reason it called obj.getplayer() and did nothing with the
result. Weird. But it does say "Test script for space station" above.
Removing this fixes an 'unused variable' warning.
With the scope of these variables reduced, it makes analyzing this
function easier, as you can now clearly see all temporary variables are
actually initialized before they're used.
Since there's an INBOUNDS_ARR() macro, it's much better to specify the
macro for the vector is a macro for the vector, to avoid confusion.
All usages of this macro have been renamed accordingly.
Stuck prevention (pushing the player/supercrewmate out if they are
inside a wall) has been factored out into its own function, so it's no
longer copy-pasted but slightly tweaked just for the supercrewmate.
Instead of having two separate functions to move entities along vertical
moving platforms, one for the player and one for the supercrewmate, they
have been consolidated into one function.
In-level, they were made to be gray in #323. The editor does not reflect this however; they're still shown as
green. For the same reasons in #323, this adds special cases to draw the entities as gray.
Closes#372.
Also, I changed my name in contributors.txt to be my username as I didn't feel comfortable with it being my name.
Co-authored-by: Misa <ness.of.onett.earthbound@gmail.com>
The game has four different functions for the same XML schema:
Game::loadtele(), Game::savetele(), Game::loadquick(), and
Game::savequick(). This essentially means one XML schema has been
copy-pasted three different times.
I can at least trim that number down to being copy-pasted only one time
by de-duplicating the reading and writing part. So both Game::loadtele()
and Game::loadquick() now use Game::readmaingamesave(), and
Game::savetele() and Game::savequick() now use
Game::writemaingamesave().
This will make it take less work to add XML forwards compatibility
(#373).
Due to #464, standing inside a gravity line during a gotoroom that
occurs every frame will end up with the gravity line being gray instead
of being white. To temporarily fix this (until #464 is properly fixed),
I decided to add some kludge that colors it white if its onentity is 1.
I tested this patch with gravity lines in both constant-gotoroom and
normal environments, and it seems to be fine for both.
In 2.0, 2.1, and 2.2, calling flipgravity() on an entity that wasn't
rule 6 would change it to rule 7. In 2.3 currently, doing this will only
change it to rule 7 if it's already rule 6, starting with the
introduction of the change where if an entity was rule 7 it would be
changed to rule 6.
The crewmate conversion trick has been restored, but converting an
entity to a crewmate will change its rule to 6, not 7 like in pre-2.3.
If you want it to be changed to rule 7 instead of 6, you'd have to call
flipgravity() twice in 2.3 and only once in pre-2.3, which would make
maintaining compatibility between versions a bit harder.
So to fix this, I'm inverting it so that if you call flipgravity() on an
entity that isn't rule 7, it will be converted to rule 7, and only if
it's rule 7 will it be converted to rule 6.
This is a followup to b7cf6855b0 and
10ed0058fd.
In 2.2, if you had a duplicate player entity, there'd be no way to get
rid of it. Except for the recently-discovered Arbitrary Entity
Manipulation glitch, where you set `i` to the indice of the entity and
call flipgravity() on it, turning its rule to 7 and making it no longer
a player entity.
However, I patched this useful mechanic out when I made it so that you'd
no longer be able to convert entities with rule 0 to rule 6
(10ed0058fd, upheld in
b7cf6855b0), because doing so would mean
being able to softlock the game by not having any player entity.
So, in this patch, I'm making it so that you CAN convert duplicate
player entities to crewmates (and thus basically destroy them), but you
can't do that to the TRUE player entity (i.e. the first entity indice
that has rule 0, which is basically always indice 0).
This patch fixes a regression caused by commit
6b1a7ebce6.
When you spawn a crewmate with an invalid color, by doing something like
`createentity(100,100,18,-1,0)` (here the color is -1, which is
invalid), a white crewmate with the color of solid white (255, 255, 255)
would appear.
That is, until AllyTally came along and committed commit
6b1a7ebce6 (Make "[Press ENTER to
return to editor]" fade out after a bit) (PR #158). Then after that
commit, it would seem like the crewmate didn't appear, but in reality
they were just invisible, because they had an invisible color.
As part of Ally's changes, to properly support drawing text with a
certain amount of alpha, she made BlitSurfaceColoured() account for the
alpha of the color given instead of only caring about the RGB of the
color, discarding the alpha, and using the alpha of the surface it was
drawing instead. This effectively made it so the alpha of whatever it
was drawing would be 255 all the time, except for if you had custom
textures and your custom textures had translucent pixels.
However, the default color set by Graphics::setcol() if you didn't
provide a valid color index was 0xFFFFFF. Which is only (255, 255, 255)
but ends up having an alpha value of 0 (because it's actually
0x00FFFFFF). And all colors drawn with alpha 0 end up being drawn with
alpha 0 after 6b1a7ebce6. So
invalid-colored entities will end up being invisible.
To fix this, I just decided to add alpha to the default value instead.
In addition, I used getRGB() to be consistent with all the other colors
in the function.
