Skip to content

Don't include raiding units in Units::isActive; make getPosition return invalid coords #4959

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.

Sign up for GitHub

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

Draft
Bumber64 wants to merge 18 commits into
base: develop
Choose a base branch
from
Draft

Don't include raiding units in Units::isActive; make getPosition return invalid coords #4959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e2f1598
Update Units.cpp
Bumber64 Sep 24, 2024
6f51b36
Update Units.h
Bumber64 Sep 24, 2024
b0da75d
Update Lua API.rst
Bumber64 Sep 24, 2024
556a8bd
Update changelog.txt
Bumber64 Sep 24, 2024
8d1ddb7
Update RemoteTools.cpp
Bumber64 Sep 24, 2024
6f196e2
Update changelog.txt
Bumber64 Sep 24, 2024
2750fcf
Update Items.h
Bumber64 Sep 24, 2024
e9158d4
Merge branch 'DFHack:develop' into unit_is_active
Bumber64 Sep 26, 2024
2995ab9
Update changelog.txt
Bumber64 Sep 26, 2024
84e9a09
Merge branch 'DFHack:develop' into unit_is_active
Bumber64 Sep 27, 2024
7664545
Merge branch 'develop' into unit_is_active
Bumber64 Dec 11, 2024
2cd932a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 11, 2024
fdf8217
Merge branch 'develop' into unit_is_active
Bumber64 Dec 28, 2024
a53ed8b
Merge branch 'develop' into unit_is_active
Bumber64 Jan 4, 2025
342afcc
Make isUnitInBox return false for any invalid pos
Bumber64 Jan 4, 2025
b618d45
Change order of preserve-rooms check
Bumber64 Jan 4, 2025
33aa7db
Merge branch 'develop' into unit_is_active
myk002 Feb 21, 2025
15547fb
move changelog entries to current version
myk002 Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/changelog.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ Template for new versions:

## API
- ``Core::getUnpausedMs``: new API for getting unpaused ms since load in a fort-mode game
- ``Units::getPosition``, ``Items::getPosition``: return invalid coord for inactive units and items held by off-map units
- ``Units::isUnitInBox``, ``Units::getUnitsInBox``: don't include inactive units
- ``Units::isActive``: return false for off-map units

# 51.04-r1

Expand Down
2 changes: 1 addition & 1 deletion docs/dev/Lua API.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1658,7 +1658,7 @@ Units module

Returns the true *x,y,z* of the unit, or *nil* if invalid. You should
generally use this method instead of reading *unit.pos* directly since
that field can be inaccurate when the unit is caged.
that field can be inaccurate when the unit is caged, off map, or dead.

* ``dfhack.units.teleport(unit, pos)``

Expand Down
2 changes: 1 addition & 1 deletion library/RemoteTools.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@ static command_result ListUnits(color_ostream &stream,

if (in->scan_all())
{
auto &vec = df::unit::get_vector();
auto &vec = df::global::world->units.active;
Copy link
Member

@lethosor lethosor Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a backwards-incompatible change to the API that should at least be documented.

Copy link
Contributor Author

@Bumber64 Bumber64 Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. Everyone would've assumed raiding units didn't pass the "active" test, and the code hasn't been touched in 7-13 years, so I didn't suspect it would break anything (more than Toady already did).


for (size_t i = 0; i < vec.size(); i++)
{
Expand Down
2 changes: 1 addition & 1 deletion library/include/modules/Items.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ DFHACK_EXPORT df::building *getHolderBuilding(df::item *item);
// Get unit that holds the item or NULL.
DFHACK_EXPORT df::unit *getHolderUnit(df::item *item);

// Returns the true position of the item (non-trivial if in inventory).
// Get the true position of the item (non-trivial if in inventory). Returns invalid coord if holder off map.
DFHACK_EXPORT df::coord getPosition(df::item *item);

/// Returns the title of a codex or "tool", either as the codex title or as the title of the
Expand Down
2 changes: 1 addition & 1 deletion library/include/modules/Units.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ inline auto citizensRange(std::vector<df::unit *> &vec, bool exclude_residents =
DFHACK_EXPORT void forCitizens(std::function<void(df::unit *)> fn, bool exclude_residents = false, bool include_insane = false);
DFHACK_EXPORT bool getCitizens(std::vector<df::unit *> &citizens, bool exclude_residents = false, bool include_insane = false);

// Returns the true position of the unit (non-trivial in case of caged).
// Get the true position of the unit (non-trivial in case of caged or inactive). Returns invalid coord if dead or off map.
DFHACK_EXPORT df::coord getPosition(df::unit *unit);

// Moves unit and any riders to the target coordinates. Sets tile occupancy flags.
Expand Down
11 changes: 9 additions & 2 deletions library/modules/Units.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,11 @@ constexpr uint32_t exclude_flags2 = (

bool Units::isActive(df::unit *unit) {
CHECK_NULL_POINTER(unit);
return !unit->flags1.bits.inactive;
if (unit->flags1.bits.inactive)
return false;
else if (unit->flags1.bits.move_state || unit->flags1.bits.can_swap)
return true; // These are always unset when leaving the map
return linear_index(world->units.active, &df::unit::id, unit->id) >= 0;
Copy link
Member

@myk002 myk002 Sep 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Units::isActive is used in many many places. I'm worried that adding an O(N) search for every call will be a performance killer. I'm also concerned that changing the behavior of this function will break existing callers, both Lua and C++. I'd like to see some research done on those two points before merging this PR.

Copy link
Contributor Author

@Bumber64 Bumber64 Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a check on my fort, and all of my units in units.active had at least one of inactive, move_state, or can_swap set. There was an inactive unit without the other two flags set that had its pos directly above my well. It's possible the combination could represent a ghost or climbing unit? Probably not going to be a performance consideration given how infrequently those occur. Then there's the actual raiding units, which you won't hit while iterating units.active, and for which the check is mandatory when iterating units.all.

I went through all the uses of isActive, getPosition, getUnitsInBox, and isUnitInBox I could find, as well as the inactive flag. There's no change in behavior to anything iterating the units.active vector (or indirectly through forCitizens/getCitizens). isDead is used instead of isActive for the case of actual dead units. Anything iterating units.all wasn't properly considering raiding units, which was just RemoteTools.cpp/spectate.

I don't fully understand plugins/preserve-rooms.cpp but it didn't look like the changes break anything. You'd know better than me.

Copy link
Member

@ab9rf ab9rf Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That call to linear_index is going to (potentially) evict the entire L1D cache and up to half of the L2 cache on the executing core (more on older processors with smaller L1 and L2 caches), which is a substantial performance penalty that I do not want in any code that is likely to be run once (or more than once) per tick. The current test requires reading one cache line (that is likely already hot); as revised this will require reading at least one line per unit in active (probably closer to two), which is a way larger performance cost.

That test is not acceptable in a high-velocity code path. If we need this information in a high-velocity code path, we need to figure out a way to cache it (on an intra-tick basis) in a manner that doesn't require thousands of scattered reads every time it's used.

For example, timestream iterates over the active unit list, calling isActive for each unit. This will make timestream much slower because each operation in timestream's adjust_units after the call to isActive will proceed with a cold L1 cache, forcing everything to be pulled down from (hopefully) L2 or L3. Given that the whole point of timestream is to increase performance, this is, to me, not acceptable.

I'm on board with offering this as a secondary Unit predicate, but changing the semantics and performance of isActive in this manner just isn't acceptable.

Copy link
Contributor Author

@Bumber64 Bumber64 Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's considering the flag checks that should reduce it to raiding units only, not all active? And given those units shouldn't even be checked when properly iterating units.active instead of units.all to begin with?

Can we keep a cache directly in the Units module? Where would we maintain it from?

If there's a second predicate, that means you need one for Units::getPosition, etc. At that point it might be easier to document that you need to perform the scan yourself if you don't know which vector the unit's from and if the result impacts your useage.

}

bool Units::isVisible(df::unit *unit) {
Expand Down Expand Up @@ -665,7 +669,8 @@ bool Units::isGreatDanger(df::unit *unit) {

bool Units::isUnitInBox(df::unit *u, const cuboid &box) {
CHECK_NULL_POINTER(u);
return box.containsPos(getPosition(u));
auto pos = getPosition(u);
return pos.isValid() ? box.containsPos(pos) : false;
}

bool Units::getUnitsInBox(vector<df::unit *> &units, const cuboid &box, std::function<bool(df::unit *)> filter) {
Expand Down Expand Up @@ -750,6 +755,8 @@ bool Units::getCitizens(vector<df::unit *> &citizens, bool exclude_residents, bo

df::coord Units::getPosition(df::unit *unit) {
CHECK_NULL_POINTER(unit);
if (!isActive(unit))
return df::coord();
if (unit->flags1.bits.caged) {
if (auto cage = getContainer(unit))
return Items::getPosition(cage);
Expand Down
4 changes: 2 additions & 2 deletions plugins/preserve-rooms.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ static void handle_missing_assignments(color_ostream &out,
if (!hf || hf->died_year > -1 || was_expelled(hf))
continue;
if (auto unit = df::unit::find(hf->unit_id)) {
if (Units::isActive(unit) && !Units::isDead(unit) && active_unit_ids.contains(unit->id)) {
if (!Units::isDead(unit) && Units::isActive(unit) && active_unit_ids.contains(unit->id)) {
Copy link
Contributor Author

@Bumber64 Bumber64 Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved isActive after isDead, which is a simple flag check. Not sure how isActive compares to testing an unordered set. The latter is probably much faster for units that are actually raiding? In that case we should test isActive afterwards.

Or maybe switch to directly testing the inactive flag (since active_unit_ids already handles the raiding units). Just curious how it performs first, though, as a stress test.

// unit is still alive on the map; assume the unassigment was intentional/expected
continue;
}
Expand All @@ -458,7 +458,7 @@ static void handle_missing_assignments(color_ostream &out,
auto spouse_hf = df::historical_figure::find(spouse_hfid);
auto spouse = spouse_hf ? df::unit::find(spouse_hf->unit_id) : nullptr;
if (spouse_hf && share_with_spouse) {
if (spouse && Units::isActive(spouse) && !Units::isDead(spouse) && active_unit_ids.contains(spouse->id))
if (spouse && !Units::isDead(spouse) && Units::isActive(spouse) && active_unit_ids.contains(spouse->id))
{
DEBUG(cycle,out).print("assigning zone %d (%s) to spouse %s\n",
zone_id, ENUM_KEY_STR(civzone_type, zone->type).c_str(),
Expand Down