make deathcause an api for getting the cause of death#1434
make deathcause an api for getting the cause of death#1434ab9rf merged 13 commits intoDFHack:masterfrom
deathcause an api for getting the cause of death#1434Conversation
make it more api-like
Remove the console formatting Signed-off-by: Squid Coder <92821989+realSquidCoder@users.noreply.github.com>
Format to keep old usage working Signed-off-by: Squid Coder <92821989+realSquidCoder@users.noreply.github.com>
myk002
left a comment
myk002
left a comment
There was a problem hiding this comment.
could you remove the leftover parens from the return statements?
Also, needs a module declaration and standard side effects guard. See: https://docs.dfhack.org/en/latest/docs/dev/Lua%20API.html#importing-scripts
final fixes
localize some more functions and then rename the APIs and add comments
|
needs a changelog and documentation |
|
i have tested this PR locally and it doesn't cause any malfunction. i haven't confirmed that the added functionality works as expected, but at least it shouldn't introduce a regression. CI testing isn't working correctly for some reason i can't explain i will approve this once the API it introduces is documented |
SilasD
left a comment
SilasD
left a comment
There was a problem hiding this comment.
I saw some minor things, nothing that really justifies delaying a merge.
One performance-related, one deliberately crashing if an event record wasn't found at all, one that requires a histfig's unit to be in world.units.all as opposed to being flushed to disk.
| -- Returns the death event for the given histfig or nil if not found | ||
| function getDeathEventForHistFig(histfig_id) | ||
| local function getDeathEventForHistFig(histfig_id) | ||
| for i = #df.global.world.history.events - 1, 0, -1 do |
There was a problem hiding this comment.
This is a very big loop. >750000 entries in my current fort's world, year 515.
Since a histfig has a .died_year field, you might be able to early-out with not-found ( nil, probably?) when you reach an event that occurred in the previous year.
You might even be able to get really clever and use a binary search to narrow down to the proper year's range.
This assumes that world.history.events is sorted by event date, which I believe but have not verified.
Speaking of that loop, this code implicitly returns nil when no matching event is found. The caller doesn't check for that.
| local function getDeathCauseFromHistFig(histfig) | ||
| local histfig_unit = df.unit.find(histfig.unit_id) | ||
| if not histfig_unit then | ||
| qerror("Cause of death not available") |
There was a problem hiding this comment.
I don't like that an API function can deliberately crash.
Consider returning this string instead of calling qerror().
| qerror("Cause of death not available") | ||
| end | ||
|
|
||
| if not dfhack.units.isDead(histfig_unit) then |
There was a problem hiding this comment.
Actually I would suggest not finding the unit at all, and testing histfig.died_year == -1 instead of not dfhack.units.isDead().
You would want to fold in the string formatting that is in getDeathEventHistFigUnit().
dfhack.units.getReadableName() accepts histfigs as well as units.
The only other use you have for a unit is to get the race, which is also in a histfig.
|
@SilasD while i think ur comments should be looked over, they cover parts of code that was pre-existing. As such, there should probably be a new PR that covers the issues of the pre-existing deathcause scripts. |
6 participants
Made this on my phone, might have errors so please dbl check