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...
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.
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.
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.
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.
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 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.
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.
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.
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.
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>
It's perfectly acceptable to have both warp lines and a warping
background in the same room. Many levels do this exact thing, I would
say at least 30 or so levels, many of them popular and played by many,
and this has never caused any issues at all.
All that having both warp lines and warp BG does is make it so the
warping of the warping background gets overriden by the warp lines, but
make it so the background is still a warp background. So in effect, you
can have a warp background without any warping. This is perfectly
defined behavior. Except, for whatever reason, it's unintentional, and
the editor tries to prevent you from doing it.
Key word being "tries". The code to prevent having both warp types is
bugged (at least when you change the warp BG. The check when you place
warp lines seems to be solid). It compares the p1 and p2 attributes of
warp lines to the x-coordinate and y-coordinate of the room, despite p1
and p2 having nothing to do with room coordinates. p1 is the type of the
warp line and should be treated as an enum, and p2 is the offset of the
warp line from the top/left of the screen. This results in this check
sometimes working if you're unlucky, but never actually working properly
most of the time. This means people can first place warp lines, and then
change the warp background later, to have both warp lines and a warp
background.
Having these checks just further complicates the code, makes it more
error-prone, and just inconveniences people when they want to do
something that's perfectly fine to do. So it's best if we just remove
these checks.
This fixes a bug where opening a script, then closing the script but not
exiting the script list, then opening a script again (it can be the same
script as before) would result in you being unable to type in the
script. You would have to close the script, then close the script list,
then re-open the script list in order to be able to type properly.
This was caused by the fact that key.enabletextentry() was being called
when the script list was opened, but key.disabletextentry() was being
called when closing a script. So the fix for this is to simply move the
key.enabletextentry() to its proper place, which is when the script is
being opened, and not when the script list is.
After looking at pull request #446, I got a bit annoyed that we have TWO
variables, key.textentrymode and ed.textentry, that we rolled ourselves
to track the state of something SDL already provides us a function to
easily query: SDL_IsTextInputActive(). We don't need to have either of
these two variables, and we shouldn't.
So that's what I do in this patch. Both variables have been axed in
favor of using this function, and I just made a wrapper out of it, named
key.textentry().
For bonus points, this gets rid of the ugly NO_CUSTOM_LEVELS and
NO_EDITOR ifdef in main.cpp, since text entry is enabled when entering
the script list and disabled when exiting it. This makes the code there
easier to read, too.
Furthermore, apparently key.textentrymode was initialized to *true*
instead of false... for whatever reason. But that's gone now, too.
Now, you'd think there wouldn't be any downside to using
SDL_IsTextInputActive(). After all, it's a function that SDL itself
provides, right?
Wrong. For whatever reason, it seems like text input is active *from the
start of the program*, meaning that what would happen is I would go into
the editor, and find that I can't move around nor place tiles nor
anything else. Then I would press Esc, and then suddenly become able to
do those things I wanted to do before.
I have no idea why the above happens, but all I can do is to just insert
an SDL_StopTextInput() immediately after the SDL_Init() in main.cpp. Of
course, I have to surround it with an SDL_IsTextInputActive() check to
make sure I don't do anything extraneous by stopping input when it's
already stopped.
This makes the modifiers for removing tiles only work if you're using tools 0 or 1 (wall or background). In the future it might be worth it to add some extra subtools for spikes, but for now this should be fine.
Also, there was a small inconsistency with the tool drawn and the tool used. If you started holding down V, then started holding down Z, you would place tiles like you're holding V (9x9) but your cursor would look like you're holding Z (3x3). The `if` chain for drawing the resized cursors has been flipped around for this.
Okay, so basically here's the include layout that this game now
consistently uses:
[The "main" header file, if any (e.g. Graphics.h for Graphics.cpp)]
[blank line]
[All system includes, such as tinyxml2/physfs/utfcpp/SDL]
[blank line]
[All project includes, such as Game.h/Entity.h/etc.]
And if applicable, another blank line, and then some special-case
include screwy stuff (take a look at editor.cpp or FileSystemUtils.cpp,
for example, they have ifdefs and defines with their includes).
It was never used and didn't do anything. It looks like it was intended
to be used but I guess time ran out or something, but it's too late now
and level files don't have timestamps or anything, so might as well just
remove it.
Good thing too, because asctime() is apparently deprecated.
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.
This removes around megabyte from the binary, so a stripped -Og binary
went from 4.0 megabytes to 2.9 megabytes, and an unstripped -O0 binary
went from 8.1 megabytes to 7.1 megabytes, which means I can now finally
upload an unstripped -O0 binary to Discord without having to give money
to Discord for their dumb Nitro thing or whatever.
Whoops. Forgot to do this earlier when adding the Shift+F3 hotkey.
Otherwise the enemy type would become invalid and just turn into the
default square.
std::string is one of those special types that has a constructor that
just initializes itself to a blank state automatically. This means all
`std::string`s are by default already `""`, so there's no need to set
them. And in fact, cppcheck throws out warnings about performance due to
initializing `std::string`s this way.
There's no need to use a template here. Just manually call SDL_tolower()
or SDL_toupper() as needed.
Oh yeah, and use SDL_tolower() and SDL_toupper() instead of libc
tolower() and toupper().
They're always the same size, so there's no need for them to be vectors.
Also made the number of elements in ed.level/kludgewarpdir controllable
by maxwidth/maxheight.
I removed editorclass::saveconvertor() because I didn't want to convert
it to treat ed.contents like an array, because it's unused so I'd have
no way of testing it, plus it's also unused so it doesn't matter. Might
as well get rid of it.
map.contents always has 1200 tiles in it, there's no reason it should be
a vector.
This is a big commit because it requires changing all the level classes
to return a pointer to an array instead of returning a vector. Which
took a while for me to figure out, but eventually I did it. I tested to
make sure and there's no problems.
Again, basically no reason for it to exist on the class itself.
The usage of the variable was replaced with temp2 instead of temp
because there was already a temp variable in the function it was used
in.
Ved has this useful feature where instead of having to manually travel
to a room whose coordinates you know, you can just press G and type in
coordinates to go there.
VCE added this, but I changed the text to be "x,y" instead of "(x,y)"
because otherwise it could confuse someone into thinking they need to
type parentheses when in reality they don't need to and typing them will
just make it not work.
Also I made sure to add an error message if the user types in an invalid
format. Failing silently would just confuse people, and maybe they'll
start thinking the feature doesn't work or something like that. VCE
doesn't have this helpful error message.
Lastly, VCE has a bug where if you use the shortcut to go from one
horizontally/vertically warping room to another, the background of the
previous room will still be there and scroll off with the background of
the room you went to, instead of just having the new background only.
This is because they forgot a 'graphics.backgrounddrawn = false;'. But
don't worry, *I* didn't forget about it.
This is basically FIQ's patch from VCE, except he never upstreamed it
because he said something along the lines of it seeming to not fit the
purpose of upstream.
But anyway, it basically just de-duplicates all the text input and text
finishing handling code and cuts down on the large amount of copy-paste
in the editor functions. It makes things way more maintainable.
Interesting note, it seems like FIQ had the intent to refactor the text
input in editor settings (i.e. the level metadata details), but never
got around to it in VCE. Maybe we'll finish that job for him later.
Allowing users to reverse cycle tilesets/tilecols/enemies prevents them
from having to press the hotkey a zillion times in order to get to the
one they want if the one they want just happens to be behind the current
one they're on.
This tilecol conveniently lets players use one of the unpatterned Space
Station tilesets you see on the left side of tiles.png but never get to
use without Direct Mode.
It does have a few weird quirks, but it should be safe to use.
Previously, it was:
if (ed.settingsmod)
{
(Settings menu controls)
...
}
else
{
(Literally everything else
Also a bunch of copy-pasted ed.keydelay checks)
...
}
Now it is:
if (ed.settingsmod)
{
(Settings menu controls)
...
}
else if (ed.keydelay > 0)
{
ed.keydelay--;
}
else if (key.keymap[SDLK_LCTRL] || key.keymap[SDLK_RCTRL])
{
// Ctrl modifiers
...
}
else if (key.keymap[SDLK_LSHIFT] || key.keymap[SDLK_RSHIFT])
{
// Shift modifiers
...
}
else
{
// No modifiers
ed.shiftkey = false;
...
}
It might not counteract how completely huge this code is, but it's at
least organized better.
Also, I had to change the map resize logic around slightly, else it'll
get triggered any time you do a shift modifier keypress.
That way, they don't show up as "?: something else" and their proper
names are shown.
I didn't update the song numbers to include the newly-allowed songs
because otherwise it'd no longer correlate with what song numbers you
use for the music() simplified command.
This doesn't make the editor completely accessible on controller, but
it's a good start at least. VCE already let you move between rooms with
the D-Pad, though.
Disabling the one-way recolor if assets are mounted is needed to make
existing levels not look bad, but what about levels that want to use the
recolor anyway?
The best solution here is to just introduce another bool into the XML,
and make the re-color opt-in and only present if assets are mounted if
that tag is present.
One-ways have always had this problem where they're always yellow. That
means unless you specifically use yellow, it'll never match the tileset.
The best way to fix this without requiring new graphics or changing
existing ones is to simply re-tint the one-way with the given color of
the room. That way, the black part of the tile is still black, but the
yellow is now some other color.
