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...
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.
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.
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.
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.
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.
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.
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".
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.
So, I was staring at VVVVVV code one day, as I usually do, and I noticed
that warp lines had this curious code in entityclass::updateentities()
that set their statedelay to 2, and I thought, hm, maybe the pre-2.1
warp line bypass is caused by this statedelay. And, it doesn't seem like
this is the primary code used to detect if the player collides with warp
lines, the actual code is commented with "Rewritten system for mobile
update" and bolted-on in gamelogic() instead of properly being in
entityclass::entitycollisioncheck().
So, after getting tripped up on the misleading indentation of that
"Rewritten system" block, I removed the rewritten system, re-added
collision detection for rule 7 (horizontal warp lines), and after
checking the resulting behavior, it appears to be nearly identical to
that of warp lines in 2.0.
You see, if you use warp lines to flip up from the top of the screen
onto the bottom of the screen, close to the edge of the bottom of the
screen, Viridian's head will display on the top of the screen in 2.0. In
2.1 and later, this doesn't happen, confirming that my theory is
correct. I also performed warp line bypass multiple times in 2.0 and
with my restored code, and it is pretty much the exact same behavior.
So now, the pre-2.1 warp line bypass glitch has been re-enabled in
glitchrunner mode.
I noticed that if I have a large amount of entities in the room, the
game starts to freeze and one frame would take a very long time. I
identified an obvious cause of this, which is that the entity collision
checking in entityclass::entitycollisioncheck() is O(n²), n being the
number of entities in the room.
But it doesn't need to be O(n²). The only entities you need to check
against all other entities are the player and the supercrewmate. You
don't need to "test entity to entity" if 99% of the pairs of entities
you're checking don't involve the player or supercrewmate.
How do we make it O(n)? Well, just hoist the rule 0 and type 14 checks
out of the inner for-loop. That way, the inner for-loop won't be
unconditionally ran, meaning that in most cases it will always be O(n).
However, if you start having large amounts of duplicate player or
supercrewmate entities (I don't know why you would), it would start
approaching O(n²), but I feel like that's fair in that case. But most of
the time, it will be O(n).
So that's how collision checking is now O(n) instead.
This makes it so the fix for crewmates' drawframes being wrong for
1-frame is fixed for all crewmates regardless of when they get created.
Sure, crewmates created in mapclass::loadlevel() have their drawframes
fixed there, but for crewmates that get created from scripting (such as
Violet when gotorooming to the Ship teleporter room after Space Station
1), this fix doesn't apply to them. But now it does, and Violet will no
longer be facing the wrong way for 1 frame when teleporting to the Ship
teleporter room in the Space Station 1 Level Complete cutscene.
If the player is stuck in a wall (which shouldn't happen in the first
place), their sprite would always default to being flipped, even if they
were unflipped.
Being stuck in a wall is characterized by having both positive onfloor
and onground.
Some levels rely specifically on the fact that certain script boxes are
loaded using gamestates, instead of directly loading the script and
bypassing the gamestate system. Then weird things could happen. This
restores compatibility with those levels.
mapclass::twoframedelayfix() doesn't need to be updated because the
point of that function is to bypass the gamestate system entirely
anyways.
This moves the teleporter, activity prompt, and trophy text "don't draw"
conditionals to the part where the game checks collision with them,
instead of whenever the game draws them.
This makes it so that the game smoothly does the fade-in/fade-out
animation instead of suddenly stopping rendering them whenever their
"don't draw" conditions apply. Now, the "Press ENTER to activate
terminal" prompt will no longer suddenly disappear whenever you activate
one, and the "- Press ENTER to Teleport -" prompt will smoothly fade
back in after teleporting, instead of suddenly popping in on screen.
Including a header file inside another header file means a bunch of
files are going to be unnecessarily recompiled whenever that inner
header file is changed. So I minimized the amount of header files
included in a header file, and only included the ones that were
necessary (system includes don't count, I'm only talking about includes
from within this project). Then the includes are only in the .cpp files
themselves.
This also minimizes problems such as a NO_CUSTOM_LEVELS build failing
because some file depended on an include that got included in editor.h,
which is another benefit of removing unnecessary includes from header
files.
There were many different ways I could've fixed it, but one thing that
stood out to me was the fact that touching the teleporter wasn't
guaranteed to set its onentity to 0, even though it should be. So now,
every time Viridian touches the teleporter, the teleporter's onentity
will be set to 0, and thus there's no chance of the teleporter
interrupting its own teleport animation and softlocking the game.
We should still do what I suggested in #391, namely setting
game.hascontrol to true if the game is in gamestate 0 and script.running
is false, and also always allowing Esc/Enter to be pressed regardless of
game.hascontrol. But this softlock is fixed now.
Fixes#391.
Instead of using gamestates, just directly use the 'script' attribute of
a script box if it is non-empty.
This is accomplished by having to return the index of the block that the
player collides with, so callers can inspect the 'script' attribute of
the block themselves, and do their logic accordingly.
To avoid going through gamestates, we'll need to carry the name of the
script on the script box itself. And to do that, we'll need to set the
'script' attribute of script boxes when translating edentities into real
entities in custom levels.
I ran the game through cppcheck and it spat out a bunch of member
attributes that weren't being initialized. So I initialized them.
In the previous version of this commit, I added constructors to
GraphicsResources, otherlevelclass, labclass, warpclass, and finalclass,
but flibit says this changes the code flow enough that it's risky to
merge before 2.4, so I got rid of those constructors, too.
Since they're always fixed-size, they don't need to be dynamically-sized
vectors.
entityclass::customcrewmoods is now a proper bool instead of an int now,
and I replaced the hardcoded constant 6 with a static const int Game
attribute to make it easier to change.
Their drawframe needs to be incremented by 2 instead of 1, because
they're double-sized.
Animation type 3 is used by the cloud emitter in The Solution is
Dilution, animation type 6 is used by the radar dish in Comms Relay.
Animation type 4 is used by the maverick bus in B-B-B-Busted, but it's
not noticeable since it spawns offscreen. This bug would cause all of
those entities to appear incorrectly for the deltaframes between the
tick the room got loaded and the next tick after that.
This is noticeable in flibit's tweet showing off my over-30-FPS patch:
https://twitter.com/flibitijibibo/status/1273983014930993153
The only reason why gray Warp Zone entities were green originally was
because there is a giant concatenated list of tileset+tilecol
combinations, and by using tileset 3 tilecol 6 you're using the entry
of tileset 4 tilecol 0, which is the green Ship tileset.
So without interfering with the green Ship tileset's entry, I've decided
that the best thing to do is to just add special cases. The enemy color
was easy enough to fix. The platform color was also easy to fix.
However, there exist no existing textures for gray conveyors, so at that
point I decided to just tint the existing green one gray, and then I did
the same for platforms.
This patch is very kludge-y, but at least it fixes a semi-noticeable
visual issue in custom levels that use internal scripts to spawn
entities when loading a room.
Basically, the problem here is that when the game checks for script
boxes and sets newscript, newscript has already been processed for that
frame, and when the game does load a script, script.run() has already
been processed for that frame.
That issue can be fixed, but it turns out that due to my over-30-FPS
game loop changes, there's now ANOTHER visible frame of delay between
room load and entity creation, because the render function gets called
in between the script being loaded at the end of gamelogic() and the
script actually getting run.
So... I have to temporary move script.run() to the end of gamelogic()
(in map.twoframedelayfix()), and make sure it doesn't get run next
frame, because double-evaluations are bad. To do that, I have to
introduce the kludge variable script.dontrunnextframe, which does
exactly as it says.
And with all that work, the two-frame (now three-frame) delay is fixed.
What happens here is that the entity gets created and then gets
immediately updated on the next frame, but there's no time for their
walkingframe of 0 to be rendered, so it'll look like they have just
started with walkingframe 1. However in the delta-timestep rendering
it'll render with walkingframe 0. So we need to fix their drawframe and
increment it when creating them.
This is because their oldxp wasn't being updated when they move (or
rather, teleport) and wrap around the screen.
These enemies are ZZT centipedes, but they're referred to as ASCII
snakes in comments in the code.
Okay, so the problem here is that Graphics::setcol() is called right
before a sprite is drawn in a render function, but render functions are
done in deltatime, meaning that the color of a sprite keeps being
recalculated every time. This only affects sprites that use fRandom()
(the other thing that can dynamically determine a color is help.glow,
but that's only updated in the fixed-timestep loop), but is especially
noticeable for sprites that flash wildly, like the teleporter, trinket,
and elephant.
To fix this, we need to make the color be recalculated only in the
fixed-timestep loop. However, this means that we MUST store the color of
the sprite SOMEWHERE for the delta-timesteps to render it, otherwise the
color calculation will just be lost or something.
So each entity now has a new attribute, `realcol`, which is the actual
raw color used to render the sprite in render functions. This is not to
be confused with their `colour` attribute, which is more akin to a color
"ID" of sorts, but which isn't an actual color.
At the end of gamelogic(), as well as when an entity is first created,
the `colour` is given to Graphics::setcol() and then `realcol` gets set
to the actual color. Then when it comes time to render the entity,
`realcol` gets used instead.
Gravitron squares are a somewhat tricky case where there's technically
TWO colors for it - one is the actual sprite itself and the other is the
indicator. However, usually the indicator and the square aren't both
onscreen at the same time, so we can simply switch the realcol between
the two as needed.
However, we can't use this system for the sprite colors used on the
title and map screen, so we'll have to do something else for those.
Just to make sure it's extra smooth. Not that it will be noticeable, and
you can't access the Secret Lab in slowmode without modifying the game,
but it's nice to have this.
