search

DFHack / df-structures

Dwarf Fortress data structure descriptions
https://github.com/DFHack/dfhack
118 stars 81 forks source link

units.xml: unit_flags1.dead name is grossly misleading and ought to be renamed #247

Closed PatrikLundell closed 6 years ago

PatrikLundell commented 6 years ago

As far as I can tell, the "dead" flag's function is rather one of "inactive" (which is a better name), as the flag is set on units arriving (visitors, invaders, critters) as well as ones leaving that have left the map but not the "fortress". The unfortunate name of the flag leads scripts (fix/dead-units.lua for instance) to treat units with that flag set as if they actually are dead (which, as far as I've seen so far is represented by unit_flags2.killed, although I don't know if there are cases where dead units don't have that flag set).

Renaming the flag to "inactive" would cause plugins using it to fail to compile (easily detected) and scripts using it to fail to work (a bit harder, as it requires active searching). However, each case where it IS used ought to be reviewed to ensure it is used in its actual capacity, not as an if it was an indicator that the unit is actually dead. Keeping the current name is likely to cause further harm as scripters will continue to assume the flag's function matches the name, in particular since the "killed" flag is buried in flags2, so scripters are likely to stop looking for suitable flags when "dead" is found.

lethosor commented 6 years ago

"inactive" sounds good to me. If I put up a list, are you able to help look through tools that use this flag? It shouldn't be too hard to stop them all from breaking, but whether they use the flag correctly is another matter.

PatrikLundell commented 6 years ago

With "set up a list" I assume you mean a list of plugins and scripts using it? Yes, I can do that, guided by that list.

lethosor commented 6 years ago

Yeah, that. Hopefully there aren't any library functions that are problematic, because tools that use those could be harder to track down.

lethosor commented 6 years ago

Units::isDead() in Units.cpp uses flags1.bits.dead. So does isAlive().

grep -lr flags1.dead scripts/:

scripts/adaptation.rb
scripts/deathcause.rb
scripts/devel/export-dt-ini.lua
scripts/exterminate.rb
scripts/fix/dead-units.lua
scripts/fix/feeding-timers.lua
scripts/fix/retrieve-units.lua
scripts/fix/stuck-merchants.lua
scripts/full-heal.lua
scripts/ghostly.lua
scripts/gui/room-list.lua
scripts/gui/unit-info-viewer.lua
scripts/starvingdead.rb
scripts/superdwarf.rb
scripts/warn-starving.lua

git ls-files plugins/|xargs grep -l flags1.bits.dead

plugins/advtools.cpp
plugins/cursecheck.cpp
plugins/diggingInvaders/diggingInvaders.cpp
plugins/dwarfvet.cpp
grep: plugins/isoworld: Is a directory
plugins/labormanager/labormanager.cpp
plugins/manipulator.cpp
plugins/petcapRemover.cpp
plugins/rendermax/renderer_light.cpp
plugins/siege-engine.cpp
grep: plugins/stonesense: Is a directory
plugins/tweak/tweak.cpp

grep -lr flags1.bits.dead plugins/stonesense:

plugins/stonesense/Creatures.cpp
PatrikLundell commented 6 years ago

I'm starting with the scripts. I intend to update progress here and generate a pull request with all/most of them when done.

lethosor commented 6 years ago

We should probably verify that dead units always have the killed flag set.

PatrikLundell commented 6 years ago

True. So far I haven't encountered any dead units without that flag, but I don't play adventure mode, and I don't generate ghosts.

I made a script that counted the number of "all" and "active" units with the killed flag set and for those units I checked the "dead" flag. The script was run on 5 fortresses downloaded from DFFD for fault finding, and in every case the number of active units with the "killed" flag was equal to the dead units list count as displayed by DF and no unit had the "killed" flag set without the "dead/inactive" one being set too. In some fortresses the number of active "killed" units was equal to the number of "killed" "all" units, while in some (probably subjected to fix/dead-units) the "active" number was smaller. While not a proof, it's a good indication the "dead/inactive" flag is always set when a unit is dead, and that the "killed" flag is as well. It can be noted that I don't think ghosts are present in the saves, and that they're all fortresses.

PatrikLundell commented 6 years ago

Units::isDead: The wrong flag is definitely used, as live inactive units are designated as dead. However, the construction might be an indication that flag2.killed might not be set on ghosts. If that's the case the scripts above would have to be revisited to determine what to do in the ghost case.

Units::isAlive: Here the case is more iffy, as is the isDead/isAlive pair, as the isInactive state isn't made available, giving the false impression that there are only two states of interest. I can imagine cases where I'd be interested in whether a unit is alive, as well as cases where I only want active living ones.

Basically Units::isActive ought to be added, and every usage of isAlive should be checked to exclude inactive units or not. With this logic isAlive should use a check against flags2.killed. Also, isDead needs to be checked as well, as its negation might be used (incorrectly) to try to find active living units. The NOT_LIVING check on isAlive already means isDead and isAlive aren't each others' negations, as undead fall in neither category (but !isDead should probably be checked anyway).

Although isUndead can be extracted as (not isAlive and not isDead), adding such a function ought to be contemplated as well, if for no other reason than to clarify the existence of this intermediate state.

Stonesense: The check happens to be correct, as inactive units aren't visible. It can be noted that there's no debug string for the "killed" flag, and the string for the current "dead" flag is "dead".

PatrikLundell commented 6 years ago
PatrikLundell commented 6 years ago

Dealing with Units.h/cpp is messy, as any changes will have ripple effects onto the files using the modified functions. There is already an isUndead function (at a different place in the file), but curiously enough, it's possible for a unit to be classified as both isAlive and isUndead if unit->curse.add_tags1.bits.OPPOSED_TO_LIFE is set but unit->curse.add_tags1.bits.NOT_LIVING is not, although I don't know if it happens in practice . Also, isUndead returns "true" even if the undead is dead, i.e. has been killed.

As indicated two posts up, I would change isAlive and isDead to check "killed" rather than "dead" and add an "isActive" function. "isUndead" would get an additional check to determine that the unit hasn't been killed. These changes would have "isAlive", "isDead", and "isUndead" (as well as "isSane") check only the explicit condition in their names, requiring an additional check against "isActive" if that is desired. This is a change of the functionality of "isAlive", "isDead", and "isSane" which currently return "false" for inactive units, but no change on that condition for "isUndead", but that function would be changed to filter out dead undead.

Leaving Units unchanged is not really an option, as it's inconsistent in the current state.

lethosor commented 6 years ago

I don't think isUndead needs to be changed. It's a trait like isGay, isTrained, isMerchant, etc. that is still relevant after death.

PatrikLundell commented 6 years ago

OK, I accept that argument, but that leaves another quirk, namely that a ghost is considered to be dead as well as undead, but you'd have to use isActive to determine that it's actually an ex ghost.

lethosor commented 6 years ago

That sounds accurate to me. Do we need an isGhost()? I fail to see how isActive would help if a ghost is visible, as that would mean the ghost is active.

PatrikLundell commented 6 years ago

Yes, an isGhost would be useful but not critical. (isDead and isUndead) => it's a ghost. Add isActive to that and you can determine whether it's "active" or not. However, there's no reason to force users to figure that out by themselves.

It's true isActive wouldn't be of use for something you select, but there's a fair bit of processing of the units in units.all and units.active as well.

The remaining hole I now see is to distinguish between an inactive ghost (e.g. on the way off map on a raid, if that's how raids are implemented and ghosts can be assigned to squads, which I think someone reported doing), and a "dead" one. I can't see a situation where you'd want to process either case, though, but DFHackers are ingenious...

PatrikLundell commented 6 years ago

If my searching is correct, the following code contains one of the strings isAlive, isDead, isUndead, or isSane (which are the names of Units functions modified by pull request 1297).

lethosor commented 6 years ago

Why do you care about isSane?

Also, "isdead" (not isDead) is present in lua. I found a couple results in scripts, so maybe you didn't search that folder?

$ egrep -nr 'isAlive|isDead|isUndead|isSane' scripts
scripts/emigration.lua:65:        or dfhack.units.isDead(unit)
scripts/emigration.lua:73:        or dfhack.units.isSane(unit)
scripts/emigration.lua:75:        or not dfhack.units.isDead(unit)
scripts/emigration.lua:96:        if dfhack.units.isSane(unit)
scripts/emigration.lua:97:        and not dfhack.units.isDead(unit)
scripts/gui/family-affairs.lua:66:        if dfhack.units.isSane(spouse) then
scripts/gui/family-affairs.lua:69:        if dfhack.units.isSane(spouse) == false then
scripts/gui/family-affairs.lua:73:        if dfhack.units.isSane(df.unit.find(source.relationship_ids.Lover)) then
scripts/gui/family-affairs.lua:76:        if dfhack.units.isSane(df.unit.find(source.relationship_ids.Lover)) == false then
scripts/gui/family-affairs.lua:281:    if dfhack.units.isCitizen(selected) and dfhack.units.isSane(selected) then
scripts/gui/unit-info-viewer.lua:752: if i.missing then --dfhack.units.isDead(unit)

The last one is in a comment but could be reviewed too.

PatrikLundell commented 6 years ago

isSane is using modified functions, and so users of that may be affected.

For lua I looked at the documentation (https://dfhack.readthedocs.io/en/latest/docs/Lua%20API.html#units-module), which claims the operations are spelled the same as the C(++) ones, e.g. "dfhack.units.isDead(unit)", which matches what you've found.

My search was on the "dfhack" folder generated by cloning DFHack, so the scripts folder (and sub folders) should have been included. However, there's a reason I don't trust Windoze's search function. Maybe it didn't like to look at lua file contents this time (it's happened before).

lethosor commented 6 years ago

Did you remember to initialize submodules or clone with --recursive? If not, the scripts folder will be empty.

A more complete search: 

$ egrep -nr 'isAlive|isDead|isUndead|isSane' scripts plugins library
scripts/emigration.lua:65:        or dfhack.units.isDead(unit)
scripts/emigration.lua:73:        or dfhack.units.isSane(unit)
scripts/emigration.lua:75:        or not dfhack.units.isDead(unit)
scripts/emigration.lua:96:        if dfhack.units.isSane(unit)
scripts/emigration.lua:97:        and not dfhack.units.isDead(unit)
scripts/gui/family-affairs.lua:66:        if dfhack.units.isSane(spouse) then
scripts/gui/family-affairs.lua:69:        if dfhack.units.isSane(spouse) == false then
scripts/gui/family-affairs.lua:73:        if dfhack.units.isSane(df.unit.find(source.relationship_ids.Lover)) then
scripts/gui/family-affairs.lua:76:        if dfhack.units.isSane(df.unit.find(source.relationship_ids.Lover)) == false then
scripts/gui/family-affairs.lua:281:    if dfhack.units.isCitizen(selected) and dfhack.units.isSane(selected) then
scripts/gui/unit-info-viewer.lua:752: if i.missing then --dfhack.units.isDead(unit)
plugins/buildingplan-lib.cpp:303:        if (DFHack::Units::isDead(unit))
plugins/df-ai/ai.cpp:555:        if (Units::isAlive(u) && Units::getPosition(u).isValid() &&
plugins/df-ai/stocks.cpp:1473:            if (u && Units::isCitizen(u) && Units::isDead(u))
plugins/df-ai/stocks.cpp:2527:            if (Units::isCitizen(*u) && !Units::isDead(*u) && (*u)->status.labors[unit_labor::HUNT])
plugins/dwarfmonitor.cpp:341:            if (Units::isDead(unit))
plugins/dwarfmonitor.cpp:586:            if (Units::isDead(unit))
plugins/dwarfmonitor.cpp:1375:            if (DFHack::Units::isDead(unit))
plugins/dwarfmonitor.cpp:1791:        if (DFHack::Units::isDead(unit))
plugins/misery.cpp:47:    if (Units::isDead(unit))
plugins/zone.cpp:792:    if( !isDead(unit) && !isUndead(unit)
plugins/zone.cpp:2082:            if (isDead(unit) || isUndead(unit)) {
plugins/zone.cpp:3035:        if(    isDead(unit)
plugins/zone.cpp:3036:            || isUndead(unit)
plugins/zone.cpp:3276:        if(    isDead(unit)
plugins/zone.cpp:3277:            || isUndead(unit)
plugins/zone.cpp:3305:        if(    isDead(unit)
plugins/zone.cpp:3306:            || isUndead(unit)
plugins/zone.cpp:3342:        if(    isDead(unit)
plugins/zone.cpp:3343:            || isUndead(unit)
plugins/zone.cpp:3379:        if(    isDead(unit)
plugins/zone.cpp:3380:            || isUndead(unit)
plugins/zone.cpp:3407:        if(    isDead(unit)
plugins/zone.cpp:3408:            || isUndead(unit)
plugins/zone.cpp:3442:        if(    isDead(unit)
plugins/zone.cpp:3443:            || isUndead(unit)
library/include/modules/Units.h:108:DFHACK_EXPORT bool isDead(df::unit *unit);
library/include/modules/Units.h:109:DFHACK_EXPORT bool isAlive(df::unit *unit);
library/include/modules/Units.h:110:DFHACK_EXPORT bool isSane(df::unit *unit);
library/include/modules/Units.h:148:DFHACK_EXPORT bool isUndead(df::unit* unit);
library/LuaApi.cpp:1588:    WRAPM(Units, isDead),
library/LuaApi.cpp:1589:    WRAPM(Units, isAlive),
library/LuaApi.cpp:1590:    WRAPM(Units, isSane),
library/LuaApi.cpp:1635:    WRAPM(Units, isUndead),
library/modules/Units.cpp:387:bool Units::isDead(df::unit *unit)
library/modules/Units.cpp:395:bool Units::isAlive(df::unit *unit)
library/modules/Units.cpp:404:bool Units::isSane(df::unit *unit)
library/modules/Units.cpp:408:    if (isDead(unit) ||
library/modules/Units.cpp:449:    if (!isSane(unit))
library/modules/Units.cpp:1562:bool Units::isUndead(df::unit* unit)
library/RemoteTools.cpp:597:            if (in->has_dead() && Units::isDead(unit) != in->dead())
library/RemoteTools.cpp:599:            if (in->has_alive() && Units::isAlive(unit) != in->alive())
library/RemoteTools.cpp:601:            if (in->has_sane() && Units::isSane(unit) != in->sane())
PatrikLundell commented 6 years ago

Yes. It's where I've done the script changes, so I'm sure it exists.

PatrikLundell commented 6 years ago
lethosor commented 6 years ago

command_result is a return type, not a function name. Multiple functions return that type.

PatrikLundell commented 6 years ago

True. Sloppy reading on my part. It should be ListUnits.

lethosor commented 6 years ago

@BenLubar pointed out that git grep works well here (and you could probably use it too!). Here's some better output. I think you've handled most of this. There's a thing in the ruby plugin that should probably be changed (in unit.rb; it already handles left/incoming units, but should probably check the killed flag).

git grep -nE 'flags1.bits.dead|flags1.dead|isDead'

docs/Lua API.rst:1186:* ``dfhack.units.isDead(unit)``
library/LuaApi.cpp:1588:    WRAPM(Units, isDead),
library/RemoteTools.cpp:597:            if (in->has_dead() && Units::isDead(unit) != in->dead())
library/include/modules/Units.h:108:DFHACK_EXPORT bool isDead(df::unit *unit);
library/modules/EventManager.cpp:540:        if ( ! unit->flags1.bits.dead ) {
library/modules/EventManager.cpp:679:        if ( unit->flags1.bits.dead )
library/modules/EventManager.cpp:726:        /*if ( unit->flags1.bits.dead )
library/modules/EventManager.cpp:930:        if ( unit1->flags1.bits.dead ) {
library/modules/EventManager.cpp:942:        if ( unit2->flags1.bits.dead ) {
library/modules/EventManager.cpp:955:            //if ( unit1->flags1.bits.dead || unit2->flags1.bits.dead )
library/modules/Units.cpp:387:bool Units::isDead(df::unit *unit)
library/modules/Units.cpp:408:    if (isDead(unit) ||
library/modules/Units.cpp:1582:    return !unit->flags1.bits.dead;
plugins/advtools.cpp:485:    if (unit->flags1.bits.dead)
plugins/buildingplan-lib.cpp:303:        if (DFHack::Units::isDead(unit))
plugins/cursecheck.cpp:164:        if(unit->flags1.bits.dead && ignoreDead)
plugins/cursecheck.cpp:220:                    unit->flags1.bits.dead ? "deceased" : "active",
plugins/diggingInvaders/diggingInvaders.cpp:392:            if ( unit->flags1.bits.dead )
plugins/dwarfmonitor.cpp:341:            if (Units::isDead(unit))
plugins/dwarfmonitor.cpp:586:            if (Units::isDead(unit))
plugins/dwarfmonitor.cpp:1375:            if (DFHack::Units::isDead(unit))
plugins/dwarfmonitor.cpp:1791:        if (DFHack::Units::isDead(unit))
plugins/dwarfvet.cpp:386:        if (!real_unit || real_unit->flags1.bits.dead || !real_unit->health->flags.bits.needs_healthcare) {
plugins/dwarfvet.cpp:627:        if ( unit->flags1.bits.dead || unit->flags1.bits.active_invader || unit->flags2.bits.underworld || unit->flags2.bits.visitor_uninvited || unit->flags2.bits.visitor ) {
plugins/labormanager/labormanager.cpp:1227:                        if (other && !(other->flags1.bits.dead ||
plugins/labormanager/labormanager.cpp:1250:                        !(*u2)->flags1.bits.dead &&
plugins/manipulator.cpp:1160:        if (unit->flags1.bits.dead)
plugins/misery.cpp:47:    if (Units::isDead(unit))
plugins/petcapRemover.cpp:67:        if ( unit->flags1.bits.dead || unit->flags1.bits.active_invader || unit->flags2.bits.underworld || unit->flags2.bits.visitor_uninvited || unit->flags2.bits.visitor )
plugins/rendermax/renderer_light.cpp:801:            if(def && !u->flags1.bits.dead)
plugins/ruby/unit.rb:60:            return :Dead if u.flags1.dead
plugins/ruby/unit.rb:135:            # return true if u.flags3.ghostly and not u.flags1.dead
plugins/siege-engine.cpp:1346:    if (unit->flags1.bits.dead ||
plugins/tweak/tweak.cpp:834:            unit->flags1.bits.dead = 1;
plugins/zone.cpp:792:    if( !isDead(unit) && !isUndead(unit)
plugins/zone.cpp:2082:            if (isDead(unit) || isUndead(unit)) {
plugins/zone.cpp:3035:        if(    isDead(unit)
plugins/zone.cpp:3276:        if(    isDead(unit)
plugins/zone.cpp:3305:        if(    isDead(unit)
plugins/zone.cpp:3342:        if(    isDead(unit)
plugins/zone.cpp:3379:        if(    isDead(unit)
plugins/zone.cpp:3407:        if(    isDead(unit)
plugins/zone.cpp:3442:        if(    isDead(unit)

git -C scripts grep -nE 'flags1.bits.dead|flags1.dead|isDead'

adaptation.rb:60:    next if u.flags1.dead
deathcause.rb:61:    if unit and not unit.flags1.dead and not unit.flags3.ghostly
devel/export-dt-ini.lua:493:    { 'Dead, Jim.', { df.unit_flags1.dead } },
emigration.lua:67:        or dfhack.units.isDead(unit)
emigration.lua:77:        or not dfhack.units.isDead(unit)
emigration.lua:99:        and not dfhack.units.isDead(unit)
exterminate.rb:67:    not u.flags1.dead and
exterminate.rb:79:            if u.flags1.dead
fix/dead-units.lua:20:    if flags1.dead and flags2.killed and unit.race ~= dwarf_race then
fix/feeding-timers.lua:20: if dfhack.units.isCitizen(unit) and not (unit.flags1.dead) then
fix/retrieve-units.lua:41:        if unit.flags1.dead and shouldRetrieve(unit) then
fix/retrieve-units.lua:47:            unit.flags1.dead = false
fix/stuck-merchants.lua:48:        if u.flags1.merchant and u.flags1.dead then
full-heal.lua:58:        if unit.flags1.dead then
full-heal.lua:63:        unit.flags1.dead = false
ghostly.lua:17:    if unit.flags1.dead then
ghostly.lua:18:        unit.flags1.dead = false
ghostly.lua:21:        unit.flags1.dead = true
gui/room-list.lua:144:       and sel_item.can_use and not sel_item.owner.flags1.dead
gui/unit-info-viewer.lua:411: self.dead = u.flags1.dead
gui/unit-info-viewer.lua:752: if i.missing then --dfhack.units.isDead(unit)
starvingdead.rb:35:            if (u.enemy.undead and not u.flags1.dead)
starvingdead.rb:44:                    u.flags1.dead = true
superdwarf.rb:32:                    if u = df.unit_find(id) and not u.flags1.dead
warn-starving.lua:123:        if rraw and not unit.flags1.dead and not dfhack.units.isOpposedToLife(unit) then
PatrikLundell commented 6 years ago

BenLubar's suggestion gratefully accepted.

Edit: Looking at tweak.cpp again, I strongly suspect the logic is incorrect. As far as I can see the function just hides a ghost by making it inactive (while also making it a non-ghost). If what I've understood is correct, a killed ghost is both inactive and killed (reports indicating the ghost flag remains set, affecting legends retroactively). However, I still don't know if the procedure for killing a normal unit (as used by exterminate) would work on a ghost as well.

lethosor commented 6 years ago

clear-ghostly probably works. I don't think there's a way to "kill" ghosts normally in vanilla DF (without putting them to rest), but given how long it has been around, I'd be surprised if it didn't work. I can handle the Ruby change. You didn't notice any others in the most recent list that were missed earlier, did you?

PatrikLundell commented 6 years ago

How can you tell if a ghost removed by the script is killed? As far as I can understand, the ghost is set to inactive, and thus will cease to affect anything in DF, but so did the units that disappeared incorrectly through fix/dead-units, with the obvious difference that the ghost is not supposed to ever reappear. However, I can understand if just shuffling it into a cupboard that's never opened is sufficiently good: it's probably not causing any harm.

I went through every unit on my git grep list (I added the other changed functions to the search as well), and as you saw it also resulted in the update of the files in PR 1298.

However, these entries from your search above were not caught, so I'll have to take a look at those: plugins/df-ai/ai.cpp:555: if (Units::isAlive(u) && Units::getPosition(u).isValid() && plugins/df-ai/stocks.cpp:1473: if (u && Units::isCitizen(u) && Units::isDead(u)) plugins/df-ai/stocks.cpp:2527: if (Units::isCitizen(u) && !Units::isDead(u) && (*u)->status.labors[unit_labor::HUNT])

lethosor commented 6 years ago

Oops, df-ai shouldn't have shown up. That's one of @BenLubar's plugins, but it's not tracked, and I thought git grep only searched tracked files.

(He'll probably want to double-check those cases, but it's not something you need to care about.)

PatrikLundell commented 6 years ago

Yes, I just looked and scratched my head since I couldn't find them on my machine...

Well. On to scripts, then.

And my comment above was incorrect. It should reference 1298.

PatrikLundell commented 6 years ago

And something different: I just updated my DF 0.44.10 installation with the latest develop using the installation-debug bat script. Previously I've gotten hordes of "deprecated feature used: update" warnings, but now I get 224 warnings, most or all saying "cl : command line warning D9002: ignoring unknown option 'std:c++11'...", where "..." is a reference to the project it complains about (including embark-assistant). Is that a sign of addressing the deprecated feature issue didn't work properly for Windoze, or is it a sign of my VC installation needing an update (which I'm not looking forward to, given that the current one only barely works)?

lethosor commented 6 years ago

What VC version are you using? It's something from VS2015, right?

We fixed a lot of warnings recently, maybe after 0.44.10-r1. DFHack/dfhack#1301 changed how we ask for C++11 support, but it was requested before using compiler flags, I think. It didn't produce any warnings on BuildMaster, so I'm not sure what's going on.

lethosor commented 6 years ago

Okay, /std:c++11 isn't actually valid: https://blogs.msdn.microsoft.com/vcblog/2016/06/07/standards-version-switches-in-the-compiler/ (so you must be using CMake < 3.10, which is when /std:c++11 is used as a fallback. That's fine, but probably explains why you got that warning but BuildMaster didn't.)

PatrikLundell commented 6 years ago

I'm using VC 2015 since the instructions said newer versions wouldn't work. CMake comes from Chocolatey as of whatever version https://chocolatey.org provided when I installed it, which wasn't a huge time ago (probably August 13 2017, as that's the date of the chocolatey folder under program data).

Anyway, unless I misread things, emmigration.lua, operation canLeave (line 58), doesn't work. It checks for both isDead and not isDead, for instance. It looks like the first half is reasons for not being eligible to leave, while isCitizen onwards seem to be reason to allow it to leave (I haven't checked what profession 103 is, but given the logic, it could be baby or child). Also, it doesn't make sense to forbid non dwarves to leave as visitors can now join the fortress. I'd like to remove the race check and reverse the isCitizen and isSane conditions, change the profession check to "==" (after checking the profession and using the enum name), replace the isDead check with "not isActive", and remove the "not isDead" check.

lethosor commented 6 years ago

I think the or not dfhack.units.isDead(unit) line should be removed. Not sure how that ever could have worked, but feel free to change it.

PatrikLundell commented 6 years ago

What about isCitizen and isSane and the child check? As I read it those checks would mean citizens and sane adult and baby dorfs couldn't emigrate (103 is child, and 104 is baby). Only insane dwarven children who are members of the civ but are not nor citizens can leave? I doubt it has worked since this function was changed to the current state.

lethosor commented 6 years ago

The current state is mostly what was imported in dfhack/dfhack#730, with some later name changes to make it work at all without errors (e.g. changing u to unit). I don't know where the imported version came from, but it can't have been the one that worked in 0.34.11.

The check should probably exclude babies and children, but please use a library function to check that (or at least don't use magic numbers for professions). It should probably exclude (instead of include) insane dwarves too, since the point is that sane dwarves are choosing to leave.

PatrikLundell commented 6 years ago

I managed to locate a CMake version indication in the chocolatey directory, and it says 3.9.1, so I guess I should try to reinstall it or upgrade it, but I won't risk that before this exercise is done.

lethosor commented 6 years ago

I'm going to get rid of the /std:c++11 flag, so the warning should go away. Feel free to upgrade CMake if you want, but it's not your fault and you don't strictly need to. (Not that upgrading is bad, of course.)

PatrikLundell commented 6 years ago

I've modified emigration.lua to work (i.e. not blow up) when run in a fortress, but the random fortress used might not have sufficiently stressed dwarves. I had to add a check against a non existent soul. It's been added to the Dead misuse DFHack/scripts#62 PR.

I don't think ghostly.lua is correct when it comes to the non standard paths through the script, but still don't know how adventure mode handles dead players, and have no idea what happens if you make them inactive.

lethosor commented 6 years ago

It would be pretty hard to select an off-map inactive unit in adventure mode, so I think ghostly is fine.

PatrikLundell commented 6 years ago

I suspect it might not be possible to get to the two non standard paths, so I agree it's probably fine functionally.

lethosor commented 6 years ago

Ok, so the next step is to rename dead to inactive. I'll do that tomorrow.

PatrikLundell commented 6 years ago

Well, there's still the matter of renaming all remaining uses of the flag in scripts and code to close the matter completely.

lethosor commented 6 years ago

I know, I'm doing that

lethosor commented 6 years ago

Changes to unit.h (including dependencies like unit_flags1.h) require recompiling a lot, so that's why the DFHack change is taking a while.

lethosor commented 6 years ago

Missed one in mousequery - luckily, it doesn't look like scripts are affected:

    df::unit_flags1 bad_flags;
    bad_flags.whole = 0;
    bad_flags.bits.dead = true;
PatrikLundell commented 6 years ago

Sneaky renaming... Serves them right that it blows up on them! Wait, you're the one it blows up in the face of ;)

lethosor commented 6 years ago

It's a fairly common pattern with item flags in some plugins, but this is the only place I've seen it done with unit flags.

Anyway, I think this is actually done now with https://github.com/DFHack/dfhack/commit/7a5e7c7c86a70fbcdeb20403720345ecb2c52364

lethosor commented 6 years ago

Got the commit message wrong, of course, but everything else looks ok to me.

PatrikLundell commented 6 years ago

A reasonably substantial refactoring of a single flag. Thanks for the cooperation!