-
Notifications
You must be signed in to change notification settings - Fork 198
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
Misc improvement: suspendmanager.lua
has new suspension reason for unsupported constructions.
#839
Misc improvement: suspendmanager.lua
has new suspension reason for unsupported constructions.
#839
Conversation
suspendmanager.lua
Outdated
local tt = dfhack.maps.getTileType(n) | ||
if tt then | ||
local attrs = df.tiletype.attrs[tt] | ||
if TILETYPE_SHAPE_WALL_SUPPORT[attrs.shape] then return false end | ||
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.
You might consider breaking these bits out into a function.
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.
Yeah I agree that wasn't very clean of me. I've broken those bits out now and cleaned up the control flow of the function
…) for reading clarity
suspendmanager.lua
Outdated
} | ||
|
||
local CONSTRUCTION_FLOOR_SUPPORT = utils.invert{ | ||
df.construction_type.FLOOR, |
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.
df.construction_type.FLOOR, | |
df.construction_type.Floor, |
Floor does not seem to be upper-case (tested locally, and in the structures: https://github.com/DFHack/df-structures/blob/e6d83ccaee5b5a3c663b56046ae55a7389742da8/df.buildings.xml#L1007 )
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 catch, that must be why it doesn't work for floors!
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've made the change now - should work for that case
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.
It does :)
suspendmanager.lua
Outdated
@@ -433,6 +639,11 @@ function SuspendManager:refresh() | |||
end | |||
end | |||
|
|||
-- Check for construction jobs which may be unsupported |
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.
This block should be moved under below Internal reasons to suspend a job
, after the self.preventBlocking
check.
The terminology is not very good, but everything before this check is what is already suspended by dwarf fortress, and suspendmanager (/unsuspend) will never unsuspend it, and everything after is what suspendmanager is actively suspending.
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.
Ok, I misunderstood what preventBlocking
meant in this context. I think maybe that might warrant a rename due to the expanded scope of this script? perhaps something like preventRiskyJobs
or something?
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.
It's actually even wider than that, the suspended jobs on smoothing designation also fall in that category, they are not risky, they would just erase a smoothing job. Maybe falling back on the broader "smart" term would be best.
This issue is spread all across the suspension script, even in the UI and the suspend
/unsuspend
command line options, I suggest not to do that in that pr, maybe a separate issue?
…r self.preventBlocking check
I think I made a mistake copying and pasting the coordinates and forgetting to change them 🤦 I think I've fixed this and I've added comments on the directions to make it easier to quickly verify correctness |
For the terminology, I think that "would collapse" is the best for any user facing text (so reason description «Would collapse immediately», doc and changelog too?), it's the term used in the native UI «Something has collapsed on the surface!». «Unsupported» feels a bit ambiguous, and cave in is not that obvious to me. Disclaimer I'm not a native speaker though |
You're right, something about this was bothering me too. "Unsupported" sounds like it could be mistaken for a technical term (as in, it could be read like "this script doesn't support this type of construction"), rather than a game thing. |
You can also say it lacks a foundation, has no foundation, needs foundation, or requires a foundation, etc. |
Some poor guy is gonna misread this as fountain and pump water all over it hoping it will work |
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.
Nice!
This change to suspendmanager deals with DFHack/dfhack#3601. I've added a new suspension reason
UNSUPPORTED
and added some checks relating to it. The way it works is based on an observation: each tiletype shape that can provide support provides it in one of two ways - either it supports like a wall (all 6 ortho axes as well as orthogonally on the layer above, with an invisible floor), or it supports like a floor (orthogonally on the same level). The only other thing that can provide support is a support building (pillar), which provides support to floors above it or walls both above and below.I have tested this to a standard that I hope is rigorous enough although @plule says he's willing to take a shot at testing some other cases I may have missed.