Skip to content
This repository was archived by the owner on May 13, 2025. It is now read-only.

Make tilesets into a Resource - #297

Open
Koopa1018 wants to merge 33 commits into
masterfrom
refactor-tileset-resources
Open

Make tilesets into a Resource#297
Koopa1018 wants to merge 33 commits into
masterfrom
refactor-tileset-resources

Conversation

@Koopa1018

@Koopa1018 Koopa1018 commented May 29, 2024

Copy link
Copy Markdown
Collaborator

NOTE: This PR depends on #296

Description of changes

This PR changes the terrain system to use a TerrainSkin resource instead of raw tileset template images.

This change has many benefits. Most notably, terrains can now use textures of arbitrary size, since the texture size is no longer hardcoded to match the specific template layout. Texture reuse between terrain lumps is another benefit, as well as the theoretical ability to add arbitrary data to terrain skins (e.g. footstep sound).

Terrain lumps now automatically update when the terrain skin is modified. Oddly enough, it turns out the tools needed to implement this feature technically existed in the old system...nothing like a total rewrite to help you see what's already there, I guess :/

To preserve the single-image convenience of the old system (and compatibility with all the assets we made for it), an import plugin has been written which allows importing images as terrain skins. It comes with features to modify which rects are used for creating textures, and options to either pack the sliced textures into the resource or store them as separate image files. As this is a vanilla Godot importer plugin, the file conversion is almost completely seamless, allowing images to be dragged directly onto TerrainSkin fields in the Inspector, and to still be edited directly in image editing programs.

Issue(s)

Finishes much of Taiga #69 (ncei).

This is the barest-bones version, exporting the textures used by the terrain system.

Some textures have been renamed to remove potential ambiguity (corner -> endcap, shade -> shadow, edge -> side).
Because objectively it's *not* a tileset; tilesets are used in tilemap systems. The new name is somewhat in keeping with the existing DoorSkin, I think....
Simplest possible integration. In theory, the textures are being referenced instead of copied here, so there's no duplication....

Naturally, things are now completely broken. I gotta get a test terrain resource going.
In keeping with the resource file. Might have to update `test-tilesets` to align though....
This is the most expected behavior, I think--clearing the skin should also clear set textures. Otherwise you'd get all kinds of nasty postmortem data hanging on....
And since I'm renaming as well, I might as well replace the root node's texture variables with direct refs to the textures in the skin. (The reason I kept the variables was so I wouldn't have to replace all call sites in the border scripts, but. I'm retouching the border scripts anyway ( ͡° ͜ʖ ͡°))
Not my ideal case--I'd much prefer to be generating directly from terrain templates via importer plugin. Still, it does let me test, and let me verify that it does indeed work. (And it cuts out a lot of terrain_polygon.tscn! Maybe now it'll stop getting labeled "diff is too big to display by default.")
Silly me. I forgot that accessing the skin directly left no protection from null variables. Can't get a property from no skin--so get it from the root and let it sanitize for us.

Take this opportunity to update the variable names.
In the future, I'd like to use this set of textures for non-shadow purposes. I'm trying to keep changes limited to MVP terrain-skin resource implementation, but this one is going to be inconvenient to change in the future, so I'm changing it now.
There we go! This solves the problem where changing tileset texture wouldn't update the visuals until you moved a vertex. Probably could've implemented this without the resource, actually.
So stupid that you have to do this manually per field, but there you go. Better to be explicit than inflexible, right?
...so that when the terrain skin stays the same but its contents are modified, the terrain polygons will still update

After fighting my way through the quirks of Callables (don't use a lambda), I find that...it isn't actually doing anything. It appears my TerrainSkin changed emissions are coming out before the data is actually updated. That's kind of annoying.
Turns out the problem was twofold! Not only was I calling "changed" before it was actually changed, but I'd forgotten to set the resource as a toolscript, so the setters weren't even running in the editor!

Switching a terrain skin's texture now properly updates terrain lumps using it.
This does apparently happen when starting up the editor.
Not only is it a waste of GPU time to render no border texture, but...it appears that untextured polygons actually show up as white by default, rather than fully transparent. So if I don't do this, terrain skins with blank texture slots appear outright wrong!
Basically a stub at this point, but it's got a skeleton I can fill out, so that's a good start.
Did that in the rest of the codebase. Let's commit to it.
Now correctly creates TerrainSkin resources from the v1 template, which I can verify by importing jungle_normal_spritesheet.png as a TerrainSkin.

(new_atlastexture.tres references the tileset image, but it appears to be completely unused, so. In the bin :P)
Was hoping this would make it so TerrainPolygon didn't have to save a copy of the body texture in its own data. No dice; no reason to undo this tho.
And THIS is the key feature that ties everything together! It's now not only possible to import terskins from templates, it's possible to actually use the sized-up texture feature. As well as to support template formats other than just the original.
Size of 0 shall be interpreted as an explicit request not to use this texture. Can't have it erroring because of that.
Because when there's no graphic, it's cheaper to just not render it...and this is how we indicate not to render it.
Gone are these extra loose files. Now, it's just the one, as it was always meant to be.

It annoys me that terrain_polygon has gone back to storing a copy of the body texture, but hey, there's not much I can do about that. Apart from one thing, I guess.
This duplication is intolerable. Even with VSCode's multi-cursor feature, it was becoming impractical to add things to, and was already straining the limits of readability. I tried to add something new, and just went "nope."

Doing it this way also sets up for a potential future in which terrain skins can have arbitrarily many border textures. Not in this PR though.
This shouldn't be a breaking change, but it's still helpful to do these things as Formal Changes so everyone sees that This Is What We're Doing now.
Technically, this is the last key ingredient needed for the importer to be capable of everything either system could do. I'd like to add a system to detect when a texture has changed separate from the importer, to avoid overwriting people's work, but the system is technically usable at this point.
This is meant to be a safety feature. If the textures are external, it's probably so you can modify them by hand; consequently, it'd be real stupid if the system could overwrite your changes "on a whim" and leave you with all your work gone.
I was setting that path specifically so I could do this :P

(and now I've built change detection around it, so I'd better not leave room for mistakes to creep in.)
Version control noise waiting to happen, here. Let's just head that possibility....
@Koopa1018 Koopa1018 added enhancement New feature or request refactor Edits to the underlying code labels May 29, 2024
@Koopa1018
Koopa1018 requested review from GTcreyon and jaschutte May 29, 2024 09:32
Koopa1018 added 2 commits May 29, 2024 07:10
There we go. Just the top border showing when it shouldn't clearly suggests something silly like this (forgetting to add a condition to part of the code).
Move side and bottom below the top textures, because that just feels more intuitive.
@jaschutte

Copy link
Copy Markdown
Collaborator

With the dependency gone, is it ready for merge?

I...thought I already commited this?
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request refactor Edits to the underlying code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants