-
Notifications
You must be signed in to change notification settings - Fork 194
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
WIP: autojustice #594
base: master
Are you sure you want to change the base?
WIP: autojustice #594
Conversation
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'll have to find someone more knowledgeable about the justice system to review the business logic, but overall it looks pretty good!
autojustice/justicetools.lua
Outdated
@@ -0,0 +1,176 @@ | |||
--@ module = true | |||
|
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.
if you want to keep this separate, this file should be moved to internal/autojustice/justicetools.lua
so it doesn't show up as a runnable script. However, together they are about 500 lines, which is about average for a script like this. It might be simpler if you just combine the two.
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 also think that it's simpler to combine the two. My only reasoning to keep these files separate is just to facilitate in case others might want get/change some info from the justice system.
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.
note that gui/control-panel
was just added. Until I can get a proper query interface that allows a tool to report when it can be enabled, could you add "autojustice" to the FORT_SERVICES
table in gui/control-panel
?
docs/autojustice.rst
Outdated
Options | ||
------- | ||
|
||
``-j``, ``--jailcitizen`` |
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.
in the code, you have these options taking an argument, but I don't see it documented here. This should be:
``-j``, ``--jailcitizen true|false``
now that you have configuration options, you should also write a simple GUI to configure them, which should also show statistics and other useful information. See gui/autofish
for an example. That can come in a different PR, though, if you like
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 will take a look on the GUI part later on.
Should I still add the "autojustice" entry to the gui/controrl-panel
even without a GUI?
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.
Yes, the control panel will automatically pick up the GUI config when it's added. Until then, autojustice will just not have a button for launching the gui
btw, this is because permanent residents are not "citizens". you might have to make a special check for this case |
be sure to merge |
- Added region comments
Looks good! If this is ready for merge, could you fix the pre-commit.ci errors and remove WIP from the PR title? |
I would appreciate an option to not have the script handle automatic interrogations for visitors. I do find that some visitors who turn out to be innocent want to leave after an interrogation, so I don't think it's necessarily a good idea to interrogate absolutely everyone. Following up implicated suspects is fine, that should be separated from this ideally. |
Testing the script out, I also noticed that goblins during sieges are queued for interrogation as if they were visitors. |
@Igualop could you address the issues drhead found before merge? |
Has there been any progress on this PR? |
I've been looking at this as I actually started working on a plugin with similar features that does the same thing, before it had been pointed out that this PR existed. @Igualop if you're still around and can remember stuff about this, I do have a question - for those |
autojustice
This is a script that automate most of the tedious parts of the justice system.
Features
Notes
There are a couple of things listed as TODO, but the core functions seems to be working fine.
This script was (afk) tested on a running fort for a dozen hours without issues, but since I'm totally new with df/dfhack/lua I can't guarantee that all the modifications made to the crime data are 100% correct.
"justicetools.lua" file contains helper methods used by the "autojustice.lua". Not sure if I should merge those two scripts or where to even put the "justicetools.lua".
I'm not sure I've correctly followed all naming conventions, feel free to comment on that.
There are a couple of options that can be added in case everything is working fine (i.e: auto convict citizens when the punishment is just jail time, add notifications to convictions and confessions).
Might need to test on worlds with multiple forts as crime reports seems to have a siteId field mapped on them.
Known Issues