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

Logistics auto-forbid/claim functionality and stockpile overlay #4546

Conversation

xBlackTalonX
Copy link
Contributor

No description provided.

plugins/logistics.cpp Show resolved Hide resolved
plugins/logistics.cpp Show resolved Hide resolved
plugins/lua/stockpiles.lua Outdated Show resolved Hide resolved
plugins/lua/stockpiles.lua Outdated Show resolved Hide resolved
plugins/lua/stockpiles.lua Outdated Show resolved Hide resolved
plugins/lua/stockpiles.lua Outdated Show resolved Hide resolved
plugins/logistics.cpp Show resolved Hide resolved
@xBlackTalonX xBlackTalonX changed the title Stockpile auto-forbid/unforbid logistics functionality Stockpile auto-forbid/unforbid logistics functionality [wip] May 3, 2024
@xBlackTalonX xBlackTalonX force-pushed the stockpile-auto-forbid-and-unforbid-logistics-functionality branch from 4528709 to 183394f Compare May 5, 2024 05:28
@xBlackTalonX xBlackTalonX marked this pull request as draft May 5, 2024 05:33
@xBlackTalonX xBlackTalonX force-pushed the stockpile-auto-forbid-and-unforbid-logistics-functionality branch 3 times, most recently from ae5129f to c51ab27 Compare May 6, 2024 03:25
@xBlackTalonX xBlackTalonX changed the title Stockpile auto-forbid/unforbid logistics functionality [wip] Stockpile auto-forbid/unforbid logistics functionality May 6, 2024
@xBlackTalonX xBlackTalonX marked this pull request as ready for review May 6, 2024 03:32
@xBlackTalonX
Copy link
Contributor Author

I'd welcome more feedback but I believe the comments have been addressed and its ready for another review.

@xBlackTalonX xBlackTalonX force-pushed the stockpile-auto-forbid-and-unforbid-logistics-functionality branch from c51ab27 to e87d1f1 Compare May 6, 2024 16:28
@xBlackTalonX
Copy link
Contributor Author

found in testing that I needed to move the overlay down a bit to avoid overlap with the custom stockpile settings - pushed that tweak and ready for review

@myk002
Copy link
Member

myk002 commented May 6, 2024

will review tonight. Thanks!

Comment on lines 116 to 117
like autotrade, automelt, autodump, autoforbid/unforbid, or autotrain. There
are also buttons along the top frame for:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
like autotrade, automelt, autodump, autoforbid/unforbid, or autotrain. There
are also buttons along the top frame for:
like autotrade, automelt, or autotrain. There are also buttons along the top frame for:

Copy link
Member

Choose a reason for hiding this comment

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

these are just examples; no need to make the list complete

Comment on lines 137 to 138
if (c.get_int(STOCKPILE_CONFIG_FORBID) == -1)
c.set_int(STOCKPILE_CONFIG_FORBID, 0);
Copy link
Member

Choose a reason for hiding this comment

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

This migration code should be in plugin_load_site_data, not here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank for this feedback, I was both overlooking and not understanding plugin_load_site_data

I've moved the same logic to ~line 180 within plugin_load_site_data but I'm still not sure I understand,
if (c.key() == CONFIG_KEY) continue;

I put the mitigation after that and before,
watched_stockpiles.emplace(c.get_int(STOCKPILE_CONFIG_STOCKPILE_NUMBER), c);

It seems to work there. But I presume that it is like migrate_old_keys in that the logic would be unnecessary if v50.current saves are no longer loadable? Should I include a comment to this effect?

Copy link
Member

@myk002 myk002 May 12, 2024

Choose a reason for hiding this comment

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

if (c.key() == CONFIG_KEY) continue;

that line is there because I was silly with the naming and the plugin-wide config is also matched by CONFIG_KEY_PREFIX. We don't want to treat the plugin-wide config as a stockpile-specific config. They're different schemas.

It would have been better to give the stockpile configs a prefix with an additional path component. Right now the plugin-wide config is named logistics/config and the stockpile configs are named like logistics/432 (where the number is the stockpile building id). What would have been smarter is to name the stockpile configs like logistics/stockpile/432 so we could use logistics/stockpile as the prefix instead of just logistics.

Units::isTamable(unit) &&
!Units::isDomesticated(unit) &&
!Units::isMarkedForTraining(unit);
return !item->flags.bits.forbid &&
Copy link
Member

Choose a reason for hiding this comment

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

I suggest moving the forbid check above the call to get_caged_item so we don't waste time on caged units if cage is forbidden anyway

}

bool can_designate(color_ostream& out, df::item* item) override {
return true;
Copy link
Member

Choose a reason for hiding this comment

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

you cannot forbid artifacts. this should return !is_artifact(item)

Copy link
Member

Choose a reason for hiding this comment

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

(which the DumpStockProcessor should also be checking for -- could you add that check there too?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I didn’t know that, thanks!

out.print("logistics: marked %d item(s) for dumping\n", dump_count);
if (0 < train_count)
out.print("logistics: marked %d animal(s) for training\n", train_count);
logistics_cycle(out);
Copy link
Member

Choose a reason for hiding this comment

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

the reason this logic is kept separate from logistics_cycle is because logistics_cycle is called in response to manual triggers. We want this auto-running version to be quiet when it does nothing. we want the manually-called version to be clear and verbose when nothing happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I’ll revert that.

It feels like there is a lot of duplicate code in a few places like this one example that could be refactored, I didn’t want to go for a bunch in this PR, but separately, do you believe it worth while to reduce the duplication in this or is there a better longer term spot for these features?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For instance, what are your thoughts on leaving the call to logistics_cycle in plugin_onupdate but making the quiet aspect an optional input parameter?
image

@@ -545,6 +555,7 @@ function StockpilesOverlay:onRenderFrame()
self.subviews.melt:setOption(config.melt == 1)
self.subviews.trade:setOption(config.trade == 1)
self.subviews.dump:setOption(config.dump == 1)
self.subviews.forbid:setOption(config.forbid+1)
Copy link
Member

Choose a reason for hiding this comment

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

this is a bit opaque. I suggest self.subviews.forbid:setOption(config.forbid) instead and changing the options in the widget to:

                        options={{label='Forbid', value='0'},
                                 {label='Forbid', value='1', pen=COLOR_LIGHTRED},
                                 {label='Unforbid', value='2', pen=COLOR_LIGHTBLUE}},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is :getOptionValue() expensive? After changing to your suggestion this seems nice and simple,
image

But unless :getOptionValue() is expensive then it seems like we'd want to do the same for the other values? Once that was done, would we really even need to getStockpileConfigs (which does hit every watched stockpile when I'd think we just need this one)?

I also couldn't quite figure out how to pass the value and the subviews' view_id to the on_change callback function. Note that while all my coding abilities are very very rusty, this is my first time with lua (and I already love it and hate it! lol). I did see that it will call the callback with (newvalue, oldvalue) but whenever I tried to reference value or view_id I couldn't figure out how.

@@ -428,10 +428,10 @@ end
StockpilesOverlay = defclass(StockpilesOverlay, overlay.OverlayWidget)
StockpilesOverlay.ATTRS{
desc='Shows a panel when a stockpile is selected for stockpile automation.',
default_pos={x=24, y=-6},
default_pos={x=5, y=-11},
Copy link
Member

Choose a reason for hiding this comment

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

This position covers the categories. I think it might be better closer to where it was.
image

however, with the increased height, in its old position, it now completely covers the "add stockpile" button, which is also not good.

what is the least bad option here? should we keep everything on two lines and make the widget even wider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrestled with this too, what I originally thought would be a nice easy layout turned out to look crappy to my eye. I’ll look again but wider looked awkward last time I tried it AND I really really want import/and export to come back to this overlay and hoped to tackle that next.

What do you think about not showing this overlay when the categories are up? When it’s lower it also obscures the tooltips of the lower left navigation buttons. I liked the current location because of its alignment with the stockpile frame and the tooltips and then later realized it’s uncomfortable overlap with the customize stockpile categories frame. In the resolutions I tested I didn’t get content overlap but I did dislike the look and started contemplating if it’s really valuable enough to show when customizing the stockpile category? I can envision several additions to this overlay that are going to need more space and I really can’t imagine another location other than lower left… I’ll play around with some options and post them in discord for discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I wanted to ask because I saw that other open PR about it, should I call it ‘claim’ instead of unforbid? I take it that’s the new ingame language used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe it could stay open on the stockpile category settings screen but move locations since the stockpile button is already covered?

Any tips on resolution testing? I had tried 1920x1080 and 1024x768 and didn’t have the overlap shown in your screenshot, which appears to be 1920x1200? More vertical pixels and we have less vertical space! haha - or am I overlooking other UI scaling features?

Copy link
Member

Choose a reason for hiding this comment

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

I find the best way to test is to have DF in windowed mode and then dynamically resize the window from very large to the smallest size (where DF starts scaling the graphics instead of reducing the column/row count)

Comment on lines 61 to 62
'forbid items',
'[un][forbid]'))
Copy link
Member

Choose a reason for hiding this comment

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

why are these in the opposite order from the other columns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I played with this too and the result came from,

  1. Showing un-forbidden items / un-forbidden-able items is basically useless
  2. Showing stockpiles with auto-unforbid enabled is useful, same for auto-forbid
  3. Showing forbidden / forbid-able items is semi-valuable
  4. Space is limited…

So to get all the underlined headers on a single line but also add all 4 new columns would’ve been low value and too many characters using up the limited horizontal space. A single [un][forbid] header was space conscious and I believed was appropriate since both can’t be enabled simultaneously. To my eye it looked better having both of those to the right of ‘forbid items’ - are you suggesting it be to the left of ‘forbid items’? I can try that.

Other thoughts when I was doing this, my [setting] concept was born out of trying to have a single heading for two checkboxes, perhaps it would be worth the horizontal space to make other headers follow that format?
[melt] [trade] [dump] [train] [un][forbid]

Or perhaps I’ve misunderstood your concern? I’ll post some comparison screenshots in discord of some options.

Copy link
Member

Choose a reason for hiding this comment

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

I was fine with the column contents - I was just curious about the order. Other columns have the checkboxes before the numbers instead of after

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. I'll try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
image

So I remember why I changed the order now, but I don't like my reasoning. But since it was your actual question: 'forbid items' causes the horizontal distance between checkboxes and item counts to be larger than any other section and it starts to look like it is more associated with training than forbidden items. At one point I reversed the columns and removed the 'forbid' from 'forbid items' hoping to effectively label both 'forbid' and save on that horizontal space.

Thoughts on something closer to this?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is as addictive as the game! Once again I couldn't leave the couple nitpicks I saw alone in the prior screenshot, so here's a refreshed version,
image
Looking forward to any feedback!

Definitely going to work now! lol

Copy link
Member

Choose a reason for hiding this comment

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

This is as additive as the game!
Definitely going to work now! lol

welcome to my life : )

Copy link
Member

Choose a reason for hiding this comment

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

a good way to grab the user's configured hotkey

You certainly can get the key that a keybinding resolves to (follow the code that displays hotkey hints to see how), but most DFHack hotkeys use CUSTOM_<foo> keybindings, which are not usually changed by the player. E.g. the hotkey for training is CUSTOM_CTRL_A, so it's perfectly reasonable for you to just use a as the marker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's perfectly reasonable for you to just use a as the marker

I’ll leave my concept of a hotkey matching on my wishful-thinking backlog and just match letters with the existing vanilla+dfhack experience (i.e. use [a] for autotrain enabled).

…but just to clarify the idea (maybe this is a case of a bad idea with problems that stimulates better idea)…

Concept: retain DFHack CUSTOM_CTRL_<letter> hotkey pattern for Stockpiles Overlay ToggleHotkeyLabels, BUT dynamically change its <letter> to correspond to the currently configured vanilla DF hotkey letter for that matching vanilla designation. This applies most directly to melt, dump, and forbid.
Example: So when Vanilla DF hotkey for Dump designations is configured for [i, d] then Stockpile Overlay would match the letter d for dump and configure its auto-dump toggle hotkey with [ctrl+d]. And this same letter (lowercase) would also be what is displayed here in the logistics status output to denote which stockpiles have auto-dump enabled ([d]). That way, the mental “muscle memory” of dump = ‘d’ applies in all three experiences:
a.) Vanilla DF dump designation
b.) Stockpile Overlay auto-dump
c.) Text output denoting dump, such as logistics status output
And this like-for-like letter-to-function experience would hold true even when the user customizes their DF keys (which is a popular DF Workshop item as you likely know and grudgingly deal with). The actual hotkey combination ([i, d] vs [ctrl+d]) would not be the same, just the letter would match.
Obvious Risk: (and presumably deal breaker) A user could configure letters for these functions which in vanilla DF that conflicts with something else in one of these DFHack experiences. This likelihood would then necessitate hotkey fallback logic to a set of unique defaults when a conflict exists. This becomes particularly high maintenance (and even confusing to a user experimenting with changing a particular DF hotkey). Especially if this pattern was extended to additional DFHack experiences. So… dead concept.

Copy link
Member

@myk002 myk002 May 10, 2024

Choose a reason for hiding this comment

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

Obvious Risk

yeah

@@ -91,6 +111,8 @@ local function print_status()
print(('Total items marked for melting: %5d'):format(global_stats.total_melt))
print(('Total items marked for trading: %5d'):format(global_stats.total_trade))
print(('Total items marked for dumping: %5d'):format(global_stats.total_dump))
print(('Total items marked forbidden: %7d'):format(global_stats.total_forbid))
print(('Total items unforbidden: %12d'):format(global_stats.total_unforbid))
Copy link
Member

Choose a reason for hiding this comment

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

"unforbidden" sounds like an action. how about Total items not marked forbidden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really questioned if this line should even be there at all… it’s basically a total item count where forbidden items aren’t counted. In my testing fort I think that was something like 25,000 items! And maybe it’s good for telling you just how many items your dwarves are dealing with but it seems like an odd plugin for providing that value.

fwiw, I read unforbid as an action and unforbidden as the state. My question about adopting the “claim” and “claimed” language feels very relevant to this. But I completely agree this line sticks out as awkward and out of place, especially when it’s values can be very large.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's still valuable, at least for completeness sake. It's the complement to the forbidden count

Copy link
Member

Choose a reason for hiding this comment

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

something seems off in the output here:
image

with the current code, the number of total dump items should equal the number of total forbid items, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the trouble is that forbidden items, of which you have 17 in that stockpile, now return false to the DumpStockpileProcessor's can_designate() method,
image

When the stockpile items are scanned by each processor those forbidden items don't add to either can_designate_counts or designated_counts
image

And when those ProcessorStats get passed over to logistics.lua it is the sum of those designated and can_designate values for each processor which is printed as the total designatable items in the column,
local function get_dstat(stats) return ('%d/%d'):format(stats.designated, stats.designated + stats.can_designate) end

So in the example of the dump column, it hasn't counted the forbidden items as designatable for dump, so that total is 17 short of the total items designatable for forbid, where they were included in the sum due to their designated state.

Therefore, I believe it's working as designed? But I'm not sure if it's too confusing for users. I kinda like the resulting information provided about how many items are in the stockpile and how many of them are forbidden or designated for melt, dump, or trade. If I've understood correctly, I think my two-cents is to keep it this way. But if you have ideas for something clearer to the user I'm open to changing it.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's working as intended. Forbidden items are intended to be "invisible", and the behavior here is in line with that. They will be highlighted in the "marked forbidden" count of the output, and I think that is correct to do so.

@xBlackTalonX
Copy link
Contributor Author

Thank you again for the feedback!! I expect I can tackle this tonight.

@xBlackTalonX xBlackTalonX marked this pull request as draft May 7, 2024 17:30
@xBlackTalonX
Copy link
Contributor Author

Oh, do you prefer I not force push? Do you squash when you merge? I’m open to your preferences.

@myk002
Copy link
Member

myk002 commented May 7, 2024

Oh, do you prefer I not force push? Do you squash when you merge? I’m open to your preferences.

Force pushes do make the review harder to track because past comments get lost. We don't squash commit, but we also don't worry about messy commit histories either. When I do history spelunking, I look at the PR, not the commit.

To match in game language instead of 'unforbid' and 'mark'.
For stockpiles without configs shown by 'logistics status', the config
bools (such as 'data.melt_masterworks' and data.melt.enabled) were being
initialized to 'nil' and unable to be indexed. This fix ensures they are
initialized to 'false' for stockpiles without a config.
Making the values of the the forbid CycleHotkeyLabel in the stockpile
overlay ints matches the config values persisted in the game save. This
makes setting and retrieving the overylay value 1-to-1 and clearer.
We want the auto-running cycle to be quiet when it does nothing. But we
also want the manually-called version ('logistics now') to be clear and
verbose, even when nothing happens.
Add auto-forbid and claim to logistics plugin and stockpile overlay
With the addition of the forbid column there was an increased need for
optimizing the use of horizontal space. This tightens up the headers
while also making the data presented easier to read.
@myk002
Copy link
Member

myk002 commented May 16, 2024

Is this PR still in progress? Please mark it as "Ready for review" when I should take another look at it

@xBlackTalonX
Copy link
Contributor Author

xBlackTalonX commented May 16, 2024 via email

@myk002
Copy link
Member

myk002 commented May 16, 2024

yeah, I think it's ok to only display the overlay on the map screen and not the stockpile customization screen. How about here:
image

that is, x=2, y=-2

@xBlackTalonX
Copy link
Contributor Author

My goodness is that ever atrocious lol - but yes, I was testing this poorly. For now I'm reverting it to what it was as the edge cases for that are far less worse imo.

Mostly looking for some feedback on if I should break this up or maybe what I need to scrap to get a first part merged (such as not exposing it in the UI until we have a better method).

@xBlackTalonX
Copy link
Contributor Author

@myk002 Looking for feedback on breaking this up or getting some aspect of it merged while we work out other aspects like the overlay position. To that end, some notes below - probably missing some things, feel free to ask of course,

logistics.lua
Refactor:

  • Removed extra loop through stockpiles to grab longest stockpile name length (moved to getStockpileData loop)
  • Renamed some variables for consistency within their local scope to avoid multiple definition of 'name' and 'stat'
  • Added new function group section-header comments
  • Added comments to print_stockpile_data to assist understanding of table spacing and layout
  • Precalculate 'designatable' when getStockpileData creates the 'data' table for easier accessibility

Style

  • Changes usages of 'Forbid' to 'Claim' (command line works with claim or unforbid)
  • Changed usages of 'Mark' to 'Designate'
  • Grouped and styled 'logistics' plugin feature status output

Fixes:

  • ensured data.{type}.enable and data.melt_masterworks get initialize to boolean

Features

  • added forbid column for logistics forbid/claim
  • added calculation and footer output of total number of designated and designatable items
  • enhanced appearance and information conveyed within table header (and new footer)

Not ready to merge imo, but the functionality is there, just not sure about my lua stuff in particular, I don't know any coding norms and welcome feedback so I learn.

@xBlackTalonX xBlackTalonX changed the title Stockpile auto-forbid/unforbid logistics functionality WIP: Logistics auto-forbid/claim functionality and stockpile overlay May 17, 2024
@myk002
Copy link
Member

myk002 commented May 17, 2024

the list of changes looks good. after some more experimenting, I think the overlay would look good at

            "x": 5,
            "y": 44

I'll review the code itself when I get a chance (hopefully later today, maybe this weekend)

@xBlackTalonX
Copy link
Contributor Author

xBlackTalonX commented May 17, 2024 via email

docs/changelog.txt Outdated Show resolved Hide resolved
plugins/logistics.cpp Outdated Show resolved Hide resolved
plugins/logistics.cpp Show resolved Hide resolved
plugins/logistics.cpp Outdated Show resolved Hide resolved
plugins/lua/logistics.lua Outdated Show resolved Hide resolved
plugins/lua/logistics.lua Outdated Show resolved Hide resolved
plugins/lua/logistics.lua Outdated Show resolved Hide resolved
plugins/lua/stockpiles.lua Outdated Show resolved Hide resolved
@@ -508,6 +508,16 @@ function StockpilesOverlay:init()
{label='Training', value=false}},
initial_option=false,
on_change=self:callback('toggleLogisticsFeature', 'train'),
}, widgets.CycleHotkeyLabel{
view_id='forbid',
frame={t=2, l=0, w=16},
Copy link
Member

Choose a reason for hiding this comment

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

can you put some other toggles down on t=2 and make the overlay panel a bit shorter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was thinking that would be possible - I had some crazier ideas too but for now I'll see if I can get it looking good with options in distributed in two rows.

@myk002
Copy link
Member

myk002 commented May 18, 2024

No need to split this into separate PRs. I think it's fine as it is.

@myk002 myk002 marked this pull request as ready for review May 18, 2024 20:58
@myk002 myk002 changed the title WIP: Logistics auto-forbid/claim functionality and stockpile overlay Logistics auto-forbid/claim functionality and stockpile overlay May 18, 2024
@xBlackTalonX
Copy link
Contributor Author

I think these are the changes but I actually didn't mean to push them yet (was trying to test, had to fight with git first, when that was fixed I managed to push before I remembered I was trying to build and test). Anyway - personally I'd prefer to small this up into a different PR but I'll comment once it's tested and get a review on this response to the comments.

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.

code all looks good. only outstanding item is rearranging the overlay and testing

@@ -168,8 +168,8 @@ local function print_status()
print((' - Items designated for trading: %5d'):format(global_sp_stats.total_trade))
print((' - Items designated for dumping: %5d'):format(global_sp_stats.total_dump))
print((' - Animals designated for train: %5d'):format(global_sp_stats.total_train)) -- TODO: fix to include all animals designated for training
print((' - Forbidden items : %5d'):format(global_sp_stats.total_forbid)) -- TODO: this should exclude unreachable items and buildings
print((' - All (unforbidden) items : %5d'):format(global_sp_stats.total_claim)) -- TODO: same ^^ implement in logistics.cpp or use item script api? (reqscript('item'))
print((' - Forbidden items : %5d'):format(global_sp_stats.total_forbid)) -- TODO: this should exclude unreachable items
Copy link
Member

Choose a reason for hiding this comment

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

these numbers only involve items in stockpiles. "reachable" isn't really a concern here, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, as it was previously written, I'm pretty sure it isn't limited to only items in stockpiles. I created the totals for the items in stockpiles and they don't match. Where you pointed out I could be using get_bad_flags for a bitwise compare, that's looping through all items in play to get total counts of melt designations.

I agree, this plugin isn't about summaries for all items... but it was there and makes sense in the context of, "what do I have designated for melt?" Extending it for, "what do I have designated as forbidden," causes the paradigm to break down because we often have so many items forbidden beyond what we care about. And that's where it's a bit odd for animals being trained as well... "wait, I thought I had more animals being trained! ...oh, I do, just not animals in stockpiles."

The more I thought about it the more I liked the summary's existing function to be a total of everything in the fort, but that's when I noted these as needing improvement. And secondarily, I thought it would be cool to show, "stockpiled" / "total" for each line, which would further clarify why this summary is there rather than just using the new totals in footer of the stockpiles table.

But I dunno... that's basically just my thought process. Maybe I'm overthinking it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@@ -168,8 +168,8 @@ local function print_status()
print((' - Items designated for trading: %5d'):format(global_sp_stats.total_trade))
print((' - Items designated for dumping: %5d'):format(global_sp_stats.total_dump))
print((' - Animals designated for train: %5d'):format(global_sp_stats.total_train)) -- TODO: fix to include all animals designated for training
Copy link
Member

Choose a reason for hiding this comment

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

this is only concerning animals in cages in stockpiles. I don't think we need to show all animals designated for training here

@xBlackTalonX
Copy link
Contributor Author

Fixed up in that last commit, interested in feedback on the layout. And yeah, ready for testing, not that I haven't been using it while developing it, my only fears are my more recent changes being something stupid.

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 good to me. I'm happy merging this as-is, but feel free to test/tweak further. There's always the next PR.

@myk002 myk002 merged commit 3444c3d into DFHack:develop May 19, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants