Skip to content
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

Use pathability groups (thanks @myk002) to detect stranded citizens #833

Merged
merged 32 commits into from
Oct 16, 2023

Conversation

azrazalea
Copy link
Contributor

@azrazalea azrazalea commented Sep 11, 2023

  • Create a command that can detect stranded dwarves
  • Add periodic checking and warning box similar to warn-starving
  • Add ability to react to warning box with ignore
  • Save these ignores persistently
  • Update GUI to change view based on the ignored status of selected units.
  • Add command-line options to configure ignores
  • Add command-line ability to get status
  • Add command-line ability to get status + walkability group numbers
  • Allow periodicity to be configured?
  • Add ignore group action (toggleable? Yeah let's do it)
  • Figure out how to get mouse buttons to work
  • Improve UI, scrollbox, bigger window, ??
  • On reflection, sort the groups in the list ascending by size so that the worst off stranded are on the top.
  • Look into adding to autostart list?

@azrazalea
Copy link
Contributor Author

So I understand in general how we're doing the calculations to get the pathability group (using modulus 16 and getting the remainder), and I can see that it works and what the results are, but I don't know enough about the data structures involved to know why that works.

If anyone an point me to some resources or explain how map blocks/block.walkable works that'd be awesome!

@azrazalea
Copy link
Contributor Author

Fixes DFHack/dfhack#3658

@azrazalea
Copy link
Contributor Author

I have no idea what a good time interval is for this. Without being able to ignore 4 times a day is definitely too often, but after ignores are added maybe it'll be easier?

@myk002 myk002 linked an issue Sep 11, 2023 that may be closed by this pull request
Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

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

looks pretty solid! great job!

changelog.txt Outdated Show resolved Hide resolved
docs/warn-stranded.rst Outdated Show resolved Hide resolved
docs/warn-stranded.rst Outdated Show resolved Hide resolved
docs/warn-stranded.rst Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
@myk002
Copy link
Member

myk002 commented Sep 11, 2023

So I understand in general how we're doing the calculations to get the pathability group (using modulus 16 and getting the remainder), and I can see that it works and what the results are, but I don't know enough about the data structures involved to know why that works.

If anyone an point me to some resources or explain how map blocks/block.walkable works that'd be awesome!

This just happens to be one of the few things we have documented about the data structures. https://docs.dfhack.org/en/latest/docs/api/Maps.html

tl;dr - blocks represent a 16x16 group of tiles, and the modulus transforms the map-local coordinates to block-local coordinates.

@myk002
Copy link
Member

myk002 commented Sep 11, 2023

  • Allow periodicity to be configured?

nah. as long as the control panel commandline runs it "often enough", that will be fine.

@myk002
Copy link
Member

myk002 commented Sep 11, 2023

maybe a warn-stranded status command would be useful - prints out the currently stranded units and marks those which are ignored.

@azrazalea
Copy link
Contributor Author

maybe a warn-stranded status command would be useful - prints out the currently stranded units and marks those which are ignored.

Agreed! That was the kind of thing I was thinking with the "add command line options to manage ignores" point!

I believe I have fixed all the review comments besides the indentation (my editor is set to 2, but I at least normalize it). I'll fix that shortly, I need to head to sleep!

On the topic of not just finding single dwarves: Do you think everybody not part of the biggest group (regardless of how many people are with them) should show up? That would mean if you have N groups you still get everyone who's not with the big group.

Thank you for all the great feedback, working on a new project is always FUN and you've helped a lot. I'll be sure to add you to the authors at the top next time I work on this.

@myk002
Copy link
Member

myk002 commented Sep 11, 2023

On the topic of not just finding single dwarves: Do you think everybody not part of the biggest group (regardless of how many people are with them) should show up? That would mean if you have N groups you still get everyone who's not with the big group.

I think this sounds correct. From a player's perspective, wouldn't you want to know about all stranded units? Totally open for debate, though, if you have other opinions on what is best here

Thank you for all the great feedback, working on a new project is always FUN and you've helped a lot.

Absolutely! I'm glad you found our codebase comprehensible : ) there's quite a bit to learn here.

I'll be sure to add you to the authors at the top next time I work on this.

That is totally not necessary -- I actually try to discourage people from adding their names to these files. Over the life cycle of this script, many people will make contributions. An author's name at the top of a particular file can diminish the work of other people who have contributed. Attribution is better handled by git blame and the commit history.

That said, I very much encourage you to add your name to our official authors list so you can be immortalized there : )

gui/control-panel.lua Outdated Show resolved Hide resolved
@azrazalea
Copy link
Contributor Author

azrazalea commented Sep 11, 2023

I think this sounds correct. From a player's perspective, wouldn't you want to know about all stranded units? Totally open for debate, though, if you have other opinions on what is best here

I like the idea. With an eye toward being able to make a fancy UI if this becomes super popular in the future, I'm thinking of doing the following:

  • List all units part of any group besides the largest count one
  • Sort the list to where groups of units are together, and display an index-based group number (1, 2, 3 etc) next to the name (I know the UI framework can support various ways of making this better but I don't know which one I want to go for and it is the hardest part for me)
  • Allow the user to specify a group number to ignore instead of having to ignore one by one (Input box? Hotkey that looks at currently selected unit and ignores their group?).
  • Allow all functionality on the command line as well, including status and ignoring by unit ID (status will have to display the unit ID for them) or group.

@azrazalea
Copy link
Contributor Author

Oh, I really wanted to implement a zoom to unit. It looks like df 50 broke that for dfhack though?

@myk002
Copy link
Member

myk002 commented Sep 12, 2023

I know the UI framework can support various ways of making this better but I don't know which one I want to go for and it is the hardest part for me)

I believe a standard widgets.List would suffice here, with the text generated from the group number and the unit name. you can attach auxiliary data to the list item by sticking a data table in the list choice. e.g. https://github.com/DFHack/scripts/blob/master/internal/caravan/movegoods.lua#L499

Hotkey that looks at currently selected unit and ignores their group?).

This sounds simple to implement. A HotkeyLabel that gets the currently selected list item and then iterates over that unit's group

Oh, I really wanted to implement a zoom to unit. It looks like df 50 broke that for dfhack though?

Opening up the unit info panel is not straightforward, but you can zoom to the unit's location (dfhack.gui.revealInDwarfmodeMap) and highlight the tile that the unit is on.

@azrazalea
Copy link
Contributor Author

azrazalea commented Sep 12, 2023

Opening up the unit info panel is not straightforward, but you can zoom to the unit's location (dfhack.gui.revealInDwarfmodeMap) and highlight the tile that the unit is on.

Oooh that function is what I was looking for. The ones I found prior to this were using older code and a lot of it was commented out or just broken. I was searching for zoom though and that's why I missed it.

I believe a standard widgets.List would suffice here, with the text generated from the group number and the unit name. you can attach auxiliary data to the list item by sticking a data table in the list choice. e.g. https://github.com/DFHack/scripts/blob/master/internal/caravan/movegoods.lua#L499

Excellent, that looks like it does everything UI-wise I want and more so I'll use it as an example for things.

One logic thing I'm working through:

  • We definitely need to not use ignored units when calculating if anyone is stranded since if they are the only stranded we should say no one is.
  • We can't include ignored units when calculating the biggest group (after determining that at least one non-ignored unit is stranded) since theoretically the ignored units could cause a different group to be biggest. This will mean if your biggest group of dwarves has a lot of ignores it might not be chosen as the main group. This is a little weird but I feel like the alternative might be weirder?
  • We need to add back in the ignored units at the end in the appropriate groups so we can unignore them and can tell who is with whom. Ignored units from the main group should listed as group 1 Main Group I think if any exist so they are obvious?

@azrazalea
Copy link
Contributor Author

azrazalea commented Sep 12, 2023

With that last commit I did the grouping refactor and got zooming working. Need to figure out the UI (I need a scrollbox and it currently just spills into the actions list, the window being bigger to a max when the list is bigger would be nice to) but everything is functional. I'll update my list of TODOs.

@azrazalea
Copy link
Contributor Author

I was actually playing last night (instead of just writing) and found an unexpected benefit of this script:

My current fortress is in a reanimator biome so I keep the bridged entrance closed unless I have a reason to open it.

So every time there is a migrant wave warn-stranded triggers and shows me everyone in the migrant wave! It's actually pretty helpful for my play style.

@myk002
Copy link
Member

myk002 commented Sep 12, 2023

So every time there is a migrant wave warn-stranded triggers and shows me everyone in the migrant wave! It's actually pretty helpful for my play style.

Ha, nice : ) That information is annoyingly difficult to get in vanilla, but it's coming up more often in recent DFHack tools. The latest development (not yet released) version of the squad assignment screen can sort by arrival time, which helps, and eventually we'll have manipulator back, which can sort all units by arrival time and let you perform actions on them.

@azrazalea azrazalea force-pushed the warn_stranded branch 5 times, most recently from 7621d17 to 927a1eb Compare September 19, 2023 06:20
@azrazalea azrazalea marked this pull request as ready for review September 19, 2023 06:21
@azrazalea
Copy link
Contributor Author

@myk002 Alright, considering this complete if you could do a real review please!

docs/warn-stranded.rst Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
docs/warn-stranded.rst Show resolved Hide resolved
docs/warn-stranded.rst Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
},
widgets.Panel{
frame={h=5},
autoarrange_subviews=true,
Copy link
Member

Choose a reason for hiding this comment

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

this layout seems to use space inefficiently. some of these could potentially be distributed horizontally with non-autoarranged custom frames.

gui/control-panel.lua Outdated Show resolved Hide resolved
docs/warn-stranded.rst Outdated Show resolved Hide resolved
docs/warn-stranded.rst Outdated Show resolved Hide resolved
docs/warn-stranded.rst Outdated Show resolved Hide resolved
changelog.txt Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

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

After playtesting, I have the following notes:

  • the central position of the window makes it hard to see the unit that it's zooming to. how about making the window narrower and making it appear on the right side of the screen?
  • the min resize isn't set quite right, should be resize_min, not min_size (though that would have made sense to call it that if I had thought of it at the time)
  • the list should expand with the window as it is resized instead of being set to a constant height
  • widgets.List has some bad behavior around clicking -- it triggers on_submit() in addition to on_select(). this has the side effect that clicking on a list item to zoom has the side effect of toggling the ignored status. How about changing the ignore gesture to double click? that would match well with shift-double click toggling the group.
  • insane citizens should be included (right?)

I'll see if I can express this in code change suggestions

warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
warn-stranded.lua Outdated Show resolved Hide resolved
@myk002 myk002 merged commit d83b1c9 into DFHack:master Oct 16, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

stranded dwarf detector
2 participants