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

Combine stacks of coins, make combine work in adventure mode #1227

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Crystalwarrior
Copy link
Contributor

@Crystalwarrior Crystalwarrior commented Jul 3, 2024

Allow combine to work in adventure mode and also combine stacks of coins

Note that it's safer to use the (z) -> Items tab to combine stacks currently, as doing so from the (i)nventory will not update the interface to reflect the change until you quit out of the inventory and come back. Currently, this is resolved by closing the inventory interface, prompting the player to bring it up manually themselves.

Copy link
Contributor

@Ozzatron Ozzatron left a comment

Choose a reason for hiding this comment

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

Functionality looks good. Main thing I was concerned about is coins max stacking at 500 (which is what you've done). Some of the user feedback could be improved though.

@myk002
Copy link
Member

myk002 commented Jul 19, 2024

until you quit out of the inventory and come back

can you synthesize this with sent keystrokes?

@Crystalwarrior
Copy link
Contributor Author

until you quit out of the inventory and come back

can you synthesize this with sent keystrokes?

The thing is, when you press the "examine" button, the inventory is still technically 'open'. But I can't send the 'inventory' keystroke from the screen that's examining the item, meaning I'd have to create a hook to wait on the user to come back to the inventory to refresh it. Alternatively, I would re-generate the inventory myself from code in the background but I'm sure to miss something. Therefore the safest route is to simply close the inventory interface and tell the user that happened.

@Bumber64
Copy link
Contributor

Bumber64 commented Feb 2, 2025

Therefore the safest route is to simply close the inventory interface and tell the user that happened.

Generally, closing a widget in the way you're doing it isn't safe and can cause memory leaks. Toady hasn't provided us a way to close stuff while calling the proper destructors.

@@ -39,6 +41,8 @@ Commands
Search all stockpiles.
``here``
Search the currently selected stockpile.
``item``
Search the currently selected item.
Copy link
Member

Choose a reason for hiding this comment

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

by "item" do you mean "container"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any item that could contain anything, yeah

Copy link
Member

Choose a reason for hiding this comment

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

In that case, I would suggest naming the option container. Moreover, I would like to see an option inventory, that will go through the selected unit's inventory. If removing inventory_items turns out to be complicated, restricting this to the containers found in the inventory should be sufficient.

if dfhack.world.isAdventureMode() and df.global.game.main_interface.adventure.inventory.open then
-- refresh the inventory to prevent stale item references
print('!! closed adventure mode inventory screen to prevent stale item references !!')
df.global.game.main_interface.adventure.inventory.open = false
Copy link
Member

Choose a reason for hiding this comment

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

why can't we send LEAVESCREEN here? If we're in a "details" screen, we can send LEAVESCREEN twice, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do we do that?

Copy link
Member

Choose a reason for hiding this comment

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

gui.simulateInput(dfhack.gui.getDFViewscreen(), 'LEAVESCREEN')
and then if you're not back at the default adventurer view (use dfhack.gui.matchFocusString), then send it again

@chdoc
Copy link
Member

chdoc commented Feb 8, 2025

I just ran this on my adventurer's backpack

> combine-adv item --dry-run

will combine 3 MEAT items from 2 stacks into 1
will combine 902 COIN items from 30 stacks into 3

The problem with this result is that I have gold and silver coins from several different civilizations. Combining coins solely based on their material is just fundamentally wrong. I think it is sufficient to check whether the coins have the same coin_batch. Here are some of the batch numbers from the coins in my current adventurers inventory:

batch:	1	Ozomong Edri Gibdak gold coins [11]
batch:	1	Ozomong Edri Gibdak gold coins [3]
batch:	0	Ozomong Edri Gibdak silver coins [20]
batch:	4	Behal Ener Upek Stramdapen silver coins [102]
batch:	3	Behal Ener Upek Stramdapen gold coins [12]
batch:	1	Ozomong Edri Gibdak gold coins [22]
batch:	15	Zobo Ngerxung gold coins [17]
batch:	15	Zobo Ngerxung gold coins [3]
batch:	1	Ozomong Edri Gibdak gold coins [6]
batch:	0	Ozomong Edri Gibdak silver coins [76]
batch:	4	Behal Ener Upek Stramdapen silver coins [92]
batch:	3	Behal Ener Upek Stramdapen gold coins [32]
batch:	3	Behal Ener Upek Stramdapen gold coins [13]
batch:	0	Ozomong Edri Gibdak silver coins [32]
batch:	1	Ozomong Edri Gibdak gold coins [22]
batch:	0	Ozomong Edri Gibdak silver coins [8]
batch:	3	Behal Ener Upek Stramdapen gold coins [43]
batch:	1	Ozomong Edri Gibdak gold coins [9]
batch:	16	Zobo Ngerxung silver coins [5]
batch:	0	Ozomong Edri Gibdak silver coins [2]

@chdoc
Copy link
Member

chdoc commented Feb 8, 2025

One possible solution is to change the check for crafted items (around line 200) to:

    elseif item:isCrafted() then
        if df.item_type[stack_type.type_id] == 'COIN' then
            comp_key = ('%s+%d'):format(stack_type.type_id, item.coin_batch)
        elseif item:getQuality() == df.item_quality.Masterful then
            comp_key = ('%s+%s+%s+%s+%s'):format(stack_type.type_id, item.mat_type, item.mat_index, item:getQuality(), item:getMaker())
        else
            comp_key = ('%s+%s+%s+%s'):format(stack_type.type_id, item.mat_type, item.mat_index, item:getQuality())
        end
    else

(why can one only provide suggestions for code that has actually been changed...)

@myk002
Copy link
Member

myk002 commented Feb 8, 2025

why can one only provide suggestions for code that has actually been changed...

hear hear

@Bumber64
Copy link
Contributor

Bumber64 commented Feb 9, 2025

Combining coins solely based on their material is just fundamentally wrong. I think it is sufficient to check whether the coins have the same coin_batch.

Might add an option for combining coins of different years. Civs definitely need to remain separate because it impacts ability to trade them, IIRC.

@Bumber64
Copy link
Contributor

Seems like you should be able to just stack all items on a tile. Fort mode says it's restricted to a stockpile, but it'd be handy to just use it anywhere. (Targeting a container mostly does this, but it's a different view mode.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants