-
Notifications
You must be signed in to change notification settings - Fork 493
optional tilesize arguments in load_texture()
#3440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
the spritesheet for gui/pathable is just an example, we can change how it looks whenever you want; but at least now it's correctly sized, and anyone can just replace it with whatever 32x64 'sheet. also- i didn't remove the on-off.png stuff because i didn't know if anything else was using those sprites. 🤔 |
load_texture()
|
could you rebase this onto develop? although it will be required for the SDL2 branch, it is just as useful in the pre-SDL2 code. |
docs/changelog.txt
Outdated
| - ``Textures.load_textures()``: can now load spritesheets with tilesizes other than 8x12. | ||
| - ``gui/pathable``: now uses 32x32 sprites. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Textures::load_textures() line should go under API, and the gui/pathable line won't be user-visible as a change in behavior so doesn't need to be mentioned.
Also, there has been a release, so the changelog changes will need to be synced and moved up to the new "Future" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm always glad when i don't have to write changelog entries! 😂
but yeah, i'll move that entry in the right section then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that the standard map "cursor" tiles are already available. the decision to include them in a pathable-specific png file should be informed by whether we intend to make these things theme-able. Do we? Committing to theme-ability should not be done lightly. it will increase our code complexity and texture footprint, since we'll need to duplicate sprites for every "namespace" -- like what is proposed here for the "can walk" sprite.
Is there enough of a call to make these things theme-able that we should do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... i realize just now that i wasn't thinking at all about "committing to theme-ability" when i made this. 🤔
honestly, i just wanted to test if we could use spritesheets bigger than 8x12, and if transparency would work in them.
I was super excited that a simple change in code let us load bigger spritesheets (great job, whoever made the rest of the code! 🤣)
...and so i made that texture. 🤔
Honestly, the thought of reusing a vanilla sprite didn't even cross my mind. 😅 all i wanted to do was testing custom textures.
but more to the point... Personally, i appreciate having the option to choose/modify this kind of stuff. (as an user/player)
Although, i agree that re-using a vanilla sprite might actually make more sense in gui/pathable's case.
I mean, having a custom texture would let us really fine-tune how gui/pathable would look, of course, but probably we don't need to.
We could get away with using some sprite from cursors.png or activity_zones.png. 🤔 i just gotta look into how gui/design/quickfort does it. (hopefully i can do it from Lua!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's where gui/quickfort uses the vanilla cursor: https://github.com/DFHack/scripts/blob/master/gui/quickfort.lua#L559
essentially:
local CURSOR_PEN = to_pen{ch='o', fg=COLOR_BLUE,
tile=dfhack.screen.findGraphicsTile('CURSORS', 5, 22)}
local GOOD_PEN = to_pen{ch='x', fg=COLOR_GREEN,
tile=dfhack.screen.findGraphicsTile('CURSORS', 1, 2)}
local BAD_PEN = to_pen{ch='X', fg=COLOR_RED,
tile=dfhack.screen.findGraphicsTile('CURSORS', 3, 0)}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you determine that a centered X is clearer than a mark in the upper corner? I think the upper corner would obscure the map less, but I can see how a centered mark might be preferable. What went into this decision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm a little torn about that design.
Like, generally, i wanted whatever maker we use to be in the middle of the tile, rather than a corner. 🤔 Mostly because i find it "easier" to tell which tile the marker is actually on.
also, since these sprites will be resized up and down (aka: zooming in/out will make them blurry), the X had to be "thick enough" to not become unreadable at smaller resolutions.
That's why the red X is so chonky. 😕 I tried thinner/thicker and bigger/smaller versions, but included this one because at the time it was an okay-enough compromise between "readability-when-zoomed-out" and "covering-the-tile-below"
(and i wanted to get some opinions on it)
Ideally i would've preferred a very thin "X" on the whole tile, but then it wouldn't work well when zoomed out.
(i could try again though)
moreover, i tried creating some sort of "contrast" between walkable/unwalkable tiles by having the former being kind of a "semi-transparent tile overlay", and the latter being kind of "a symbol pasted on top of the tile"
but honestly-- if we used some other sprite from vanilla, we could avoid having this conversation entirely 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could use the same scheme I use in gui/quickfort, maybe? the red box indicating "bad" and the green diamond indicating "good"? as long as the green diamond is visible enough when scaled.
library/include/modules/Textures.h
Outdated
| /** | ||
| * Get the first texpos for the pathable 32x32 sprites. It's a 2x1 grid. | ||
| */ | ||
| DFHACK_EXPORT long getPathableTexposStart(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's going to be more of these. I think we should name these according to which grid they are designed for to keep their intended usage clear. getPathableMapTexposStart()? getMapPathableTexposStart()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call.
I can't decide on some things though...
my gut feeling is that this word, be it "Map" or anything else, should be a prefix.
the upside is that all "Map" textures will be listed together in my text editor's autocomplete. 🤔
the downside is that if your script/plugin has multiple textures, they will not be grouped together by the autocomplete thing. 😕
in the end i think i'd be more okay with the first case. All "Map" textures should be listed/grouped together, but the more i think about it, the more this seems like a not-so-important problem 🤣 and i've been thinking about it for far too long 😳
oh yeah-- what about old function names? Should those get a prefix as well?
(also do we call it "Map" or... what else? honestly my alternatives sucked so much i'm not even going to mention them 🤣)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming the older ones isn't as important, I feel. the "Interface" layer could be the "default" and "Map" (or whatever) could just follow the new standard.
+1 for making it a prefix, though the verb ("get") should probably still go first. getMapPathableTexposStart()? (or if we end up using standard cursors, then this goes away entirely, but it will come up again soon enough since we need tiles for the unsuspend overlay)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes yes, the "prefix"+texturename is meant to be sandwiched between get and TextposStart of course!
i mean, "get" is immovable. "TextposStart" is very explicative and works well. We can only work on what's between those two.
i'm guessing, like, if a script will need multiple textures, its function-names (or whatever these are called) might look like this:
getDesignTextposStart
getDesignButtonsSelectedTextposStart
getDesignButtonsUnselectedTextposStart
getMapDesignTilesTextposStart
and... like, in this case the "Map" might look a bit out of place, but in the end i think it's more important to "separate" the map textures from normal textures, than to group all of a tool's textures together. 🤔
|
oh by the way-- the |
That's the |
load_texture()load_texture()
|
do you need help getting this PR in shape for merge? |
|
It seems that 50.09 will be SDL2-based, so it's fine to keep this targeted at the SDL2 branch. I'll see if I can fix it up a little and merge. |
makes Textures.cpp's
load_texture()function accept optional arguments for the spritesheet's tile size.This means we can have sprites as big as we want!
as an example, i've added a dedicated spritesheet for gui/pathable, with two 32x32 sprites that look like this:
Dwarf_Fortress_O3VbhMy8nj.webm
this should help fix #3438
we just need to choose how the suspended/planned sprites should look like, and add a 'sheet for those. 🤔