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

It takes too long time to move rags #26107

Open
lcy03406 opened this Issue Oct 7, 2018 · 5 comments

Comments

7 participants
@lcy03406
Contributor

lcy03406 commented Oct 7, 2018

Describe the bug
It takes too long time to move rags

To Reproduce
My speed is 110. Inventory is empty or has 100 rags. On ground in same tile there are 100 rags or nothing.

  • with [d]/[,] keys, drop/pickup 1 rag = 6s
  • with [d]/[,] keys, drop/pickup 100 rags = 9m
  • in advanced [/] screen, from pos [i] to [5] or [i] to [5], move 1 rag = 6s
  • in advanced [/] screen, from pos [i] to [5], with key [M] or [m], move 100 rags = 6m18s
  • in advanced [/] screen, from pos [5] to [i], with key [M] or [m], move 100 rags = 15m24s
  • in advanced [/] screen, from pos [i] to [5], with key [,], move 100 rags = 9m30s
  • in advanced [/] screen, from pos [5] to [i], with key [,], move 100 rags = 9m6s

Expected behavior

  • Moving a stack of rags should be way more efficient than moving one by one. We humans don't repeat 100 times bending and standing.
  • Time consumed should be consistent no matter which key user pressed.

Versions and configuration(please complete the following information):

  • OS: [Windows 8]
  • Game Version [0.C-33116-ga36162e (build 8023)]
  • Graphics version [Tiles]
  • Mods loaded [ "dda",
    "no_npc_food",
    "filthy_morale",
    "Medieval_Stuff",
    "realguns",
    "bio_mod",
    "DinoMod",
    "npc_traits",
    "mutant_npcs",
    "blazemod",
    "Tanks",
    "deoxymod",
    "boats",
    "StatsThroughSkills",
    "RL_Classes",
    "more_classes_scenarios" ]
@SwagNaut

This comment has been minimized.

Show comment
Hide comment
@SwagNaut

SwagNaut Oct 7, 2018

Contributor

Dropping many small items in general takes to long. For example dropping 250 cash cards at once should be way faster than dropping them individually.

Contributor

SwagNaut commented Oct 7, 2018

Dropping many small items in general takes to long. For example dropping 250 cash cards at once should be way faster than dropping them individually.

@cake-pie

This comment has been minimized.

Show comment
Hide comment
@cake-pie

cake-pie Oct 7, 2018

Contributor

#25852 is the likely cause.

Contributor

cake-pie commented Oct 7, 2018

#25852 is the likely cause.

@anonym

This comment has been minimized.

Show comment
Hide comment
@anonym

anonym Oct 8, 2018

Contributor

#25852 is the likely cause.

Without that fix, moving 10000 rags is the same as moving 1 with Advanced Inventory, which also is nonsense. Note that the fix did not affect "normal" pickup/drop with, so the reported "with [d]/[,] keys, drop/pickup 100 rags = 9m" was like this before.

I think the comprehensive fix we need for item move cost should add consistency in cost between methods (e.g. Advanced Inventory vs drop/pickup actions) and make it simulate bulk item moving:

Consistency

The move costs should be consistent in the following cases (but we should probably keep that dropping is cheaper than picking up):

  • "in advanced [/] screen, from pos [5] to [i], with key [M] or [m], move 100 rags = 15m24s"
  • "with [d]/[,] keys, drop/pickup 100 rags = 9m"
  • "in advanced [/] screen, from pos [i] to [5], with key [M] or [m], move 100 rags = 6m18s"

Indeed, "normal" pickup/drop (, and d) are implemented with the activities ACT_PICKUP and ACT_DROP (there are a few others too, like ACT_STASH), but Advanced Inventory uses a different system that re-implements a similar logic for MOVE_VARIABLE_ITEM and MOVE_ITEM_STACK, but when you use "move all" then it is implemented with ACT_MOVE_ITEMS. All of these calculate move cost in their own way. No wonder there's inconsistency!

So as has already been suggested we should make MOVE_VARIABLE_ITEM and MOVE_ITEM_STACK also use the ACT_MOVE_ITEMS activity. Then we make the move costs consistent among ACT_MOVE_ITEMS, ACT_PICKUP and ACT_DROP (there is some work in this direction, see activity_item_handling.cpp's move_cost() and Pickup::cost_to_move_item()). I guess we want:

ACT_MOVE_ITEMS < ACT_PICKUP + ACT_DROP

since picking up actually includes organizing stuff into your inventory (it is not picking something up to your hands).

Bulk item moving

And now to actually solve this bug! Now that all item moving/picking/dropping is handled by activities, at the time the game processes them we have the full list of items to process, so we can optimize and simulate that the player does sensible bulk moving, even mixing different stacks of items. We can just go through this list and fill up "an armful" (5 liters? Larger items are dealt with one at a time) of items and calculated the move cost for that (I guess current Pickup::cost_to_move_item() actually might be good enough), and then repeat until the list is empty.


Thoughts? If this idea looks like the way to go, I could try to have a look (it looks pretty daunting though!).

Contributor

anonym commented Oct 8, 2018

#25852 is the likely cause.

Without that fix, moving 10000 rags is the same as moving 1 with Advanced Inventory, which also is nonsense. Note that the fix did not affect "normal" pickup/drop with, so the reported "with [d]/[,] keys, drop/pickup 100 rags = 9m" was like this before.

I think the comprehensive fix we need for item move cost should add consistency in cost between methods (e.g. Advanced Inventory vs drop/pickup actions) and make it simulate bulk item moving:

Consistency

The move costs should be consistent in the following cases (but we should probably keep that dropping is cheaper than picking up):

  • "in advanced [/] screen, from pos [5] to [i], with key [M] or [m], move 100 rags = 15m24s"
  • "with [d]/[,] keys, drop/pickup 100 rags = 9m"
  • "in advanced [/] screen, from pos [i] to [5], with key [M] or [m], move 100 rags = 6m18s"

Indeed, "normal" pickup/drop (, and d) are implemented with the activities ACT_PICKUP and ACT_DROP (there are a few others too, like ACT_STASH), but Advanced Inventory uses a different system that re-implements a similar logic for MOVE_VARIABLE_ITEM and MOVE_ITEM_STACK, but when you use "move all" then it is implemented with ACT_MOVE_ITEMS. All of these calculate move cost in their own way. No wonder there's inconsistency!

So as has already been suggested we should make MOVE_VARIABLE_ITEM and MOVE_ITEM_STACK also use the ACT_MOVE_ITEMS activity. Then we make the move costs consistent among ACT_MOVE_ITEMS, ACT_PICKUP and ACT_DROP (there is some work in this direction, see activity_item_handling.cpp's move_cost() and Pickup::cost_to_move_item()). I guess we want:

ACT_MOVE_ITEMS < ACT_PICKUP + ACT_DROP

since picking up actually includes organizing stuff into your inventory (it is not picking something up to your hands).

Bulk item moving

And now to actually solve this bug! Now that all item moving/picking/dropping is handled by activities, at the time the game processes them we have the full list of items to process, so we can optimize and simulate that the player does sensible bulk moving, even mixing different stacks of items. We can just go through this list and fill up "an armful" (5 liters? Larger items are dealt with one at a time) of items and calculated the move cost for that (I guess current Pickup::cost_to_move_item() actually might be good enough), and then repeat until the list is empty.


Thoughts? If this idea looks like the way to go, I could try to have a look (it looks pretty daunting though!).

@cake-pie

This comment has been minimized.

Show comment
Hide comment
@cake-pie

cake-pie Oct 8, 2018

Contributor

Thoughts?

SGTM. 👍
Having consistent, reasonable behavior throughout is key.

Contributor

cake-pie commented Oct 8, 2018

Thoughts?

SGTM. 👍
Having consistent, reasonable behavior throughout is key.

@Xelat84

This comment has been minimized.

Show comment
Hide comment
@Xelat84

Xelat84 Oct 9, 2018

Contributor

@anonym sounds like a good solution, need to try.

Contributor

Xelat84 commented Oct 9, 2018

@anonym sounds like a good solution, need to try.

@kevingranade kevingranade added this to the 0.D milestone Oct 11, 2018

@kevingranade kevingranade added this to Confirmed in 0.D Release Oct 11, 2018

@ZhilkinSerg ZhilkinSerg moved this from Confirmed to Fix Proposed in 0.D Release Oct 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment