-
Notifications
You must be signed in to change notification settings - Fork 218
Add autoceiling script and documentation #1515
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
base: master
Are you sure you want to change the base?
Conversation
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
SilasD
left a comment
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.
Okay, first, I'm not in charge here. Feel free to ignore anything I say.
I saw three areas that need attention:
- As far as I could tell, the documented command-line parameter to "Raise or lower flood-fill limits" is not implemented.
- Line 150 looks like it makes an invalid API call.
- Line 180 attempts to load core API as a module; this does not work.
Feel free to ignore my comments on working code.
I am not trying to force you to change the code, I just want to point out ways it could be better.
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.
Oops, somehow I placed this review twice. I hope it didn't get too messed up.
Okay, first, I'm not in charge of anything. Feel free to ignore anything I say.
I saw three areas that need attention:
- As far as I could tell, the documented command-line parameter to Raise or lower flood-fill limits is not implemented.
- Line 150 looks like it makes an invalid API call.
- Line 180 attempts to load core API as a module; this does not work.
Feel free to ignore my comments on working code. I am not trying to force you to change the code, I just want to point out ways it could be better.
I made some changes, and sorry it's late. I think the mod has potential and could be used in most sessions, so if anyone is interested in taking it over, you have my full permission.
Thank you for reviewing the code. I made modifications that I hope align with your input and suggestions. |
SilasD
left a comment
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.
No, this is not late at all. You were pretty timely, actually.
(Are you actively requesting that someone take it over? I'd be willing to, I think.)
I like this a lot. It looks a lot cleaner.
Again, feel free to ignore my comments; this looks pretty good.
I did spot a couple of areas that could be improved, and there's just one thing that makes me uncomfortable.
The thing that I think you should look at more closely is z_surface. It looks to me that you're assuming the surface is flat. That's not always true. I'm not sure what to do about it, though.
(Incidentally, when you've either dealt with or rejected a review comment, you can mark it as resolved, which hides it. I can't do it, even on my own comments, probably because I don't have reviewer or administrator privileges or some such.)
| local function xyz2pos(x, y, z) | ||
| return { x = x, y = y, z = z } | ||
| end |
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.
So the reason I mentioned xyz2pos() is that it's a standard API call.
You've basically reimplemented it, which is fine. Just saying you didn't need to.
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.
duplicating code that already exists in the code library is a no-no and will result in a review rejection, so you might as well fix this nw before i get around to finding it myself when i do the review. Don't Repeat Yourself also covers repeating someone else
| local footprint = {} | ||
| local visited = {} | ||
| local q = { { seed_x, seed_y } } | ||
| local queue = { { seed_x, seed_y } } |
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 see you're special-casing the very first queue[] element and visited[] element. I think that's fine,
but it would be more elegant if you could use push_if_ok(seed values) instead.
I'm not going to game out whether that would work with your current code. Just something to consider.
Maybe something like this would work":
local queue = {}
local queue_pos = 1
local function push_if_ok()
...
push_if_ok(seed_x, seed_y)Again, I didn't test this.
| local tt = get_tiletype(x, y, z0) | ||
| if is_walkable_dug(tt) then | ||
| visited[key] = true | ||
| table.insert(queue, { x, y }) |
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 wondering if your queue could directly store pos elements. i.e.
table.insert(queue, xyz2pos(x, y, z0))
Then you could pass the current queue element directly to your functions or to API calls.
You could also just copy a queue element directly into footprint[].
I mean, your code clearly works, but doing it that way seems like it would be more elegant.
Let's see, you still couldn't use a pos as a vistied[] key. You would still need to manually build key so you can test it in one call as an associative array/dictionary.
(There is an API call same_xyz(pos1, pos2) but you can't probe an associative array for the values of a table, while you can test for existence of string key in one probe.)
(Although... if it's the exact same table instead of a copy or a newly-constructed table, an existence probe would work. I think.)
I appreciate your comments, they have been helpful. There is an unresolved issue with this program. If the user digs into a mountain and then runs the program, the ceiling will bleed out of the cave entrance. I had to build some temporary walls before running the program, which saved me some time since I did not have to do the ceiling manually. I do not know how to fix this issue. Yes, I am actively looking for someone to take over this and the other PRs I create since I am not a programmer. If you are interested, feel free to take it over. I will close this PR and you can create a new one instead, or I'll follow whatever the proper procedure is for handing off a PR. Thank you |
|
@ab9rf I guess the plan is to transfer this PR to me. I think I want to copy/fork unboundlopez's autoceiling branch to my scripts repo. Is that the way? Last time I tried to figure out how to work with someone's branch, I ran into GitHub only letting me have one fork of a project. (That was related to scripts/entomb, and the author ended up adding me as a contributor / collaborator.) Should I try to work directly from command-line? I could try. I'm not at all sure how to make a PR from command-line. |
Recording.2025-11-09.114149.1.mp4
AutoCeiling
Summary: Place floors above dug areas to seal surface openings.
Tags: construction, automation, utility
AutoCeiling is a DFHack Lua script that automatically places constructed floors
above any dug-out area. It uses a flood-fill algorithm to detect connected dug
tiles on the selected Z-level, then creates planned floor constructions directly
above them to seal the area. This prevents surface collapse and stops creatures
from entering your fortress through unexpected openings. It’s especially useful
when building farms directly below the surface, since those areas are prone to
collapsing without warning and can leave open spaces that allow surface
creatures to breach your fort.
Usage
Examples
autoceiling
Run with default settings (4,000 tile flood-fill limit, no diagonal fill).
autoceiling t
Enable diagonal flood-fill connections (8-way fill).
autoceiling 6000
Raise flood-fill limit to 6,000 tiles.
autoceiling t 6000 or autoceiling 6000 t
Allow diagonals and increase fill limit to 6,000 tiles.
Options
t
Enables 8-directional (diagonal) flood fill mode.
Number
Sets the maximum number of tiles the flood fill can cover (default: 4000).
Moderators, please feel free to take ownership of anything I create. I'm happy to share and collaborate freely.