Afforess / Factorio-Stdlib

Factorio Standard Library Project
ISC License
162 stars 45 forks source link

copy_inventory is limited and should use set_stack #66

Closed NiftyManiac closed 7 years ago

NiftyManiac commented 7 years ago

Right now Inventory.copy_inventory uses inventory.insert, which is very limited. It won't preserve the positions of items, the contents of blueprints, or the inventories of armor.

Instead, you should do something like this:

  for i=1,#src do
    dest[i].set_stack(src[i])
  end
Afforess commented 7 years ago

Honestly I am not convinced of it's utility overall - given the correct solution is what you just posted above, and fairly concise already. I'm deprecating the Inventory module and will just remove the code later.

Nexela commented 7 years ago

When I put in the PR I deliberately avoided set_stack due to a factorio bug that will be fixed in .15

Also copy_inventory has its benifits copy_inventory may be the wrong name for it but the function is usefull

for i=1,#src do dest[i].set_stack(src[i]) end This has no slot checking and will error if #dest is < #src_inv Also if there is something in dest[i] it will just overwrite

For that reason I propose inventory module stays and gets expanded :)

Also the meta .valid isn't needed in copy_inventory() as simple_item_stacks are unreferenced tables anyway

Afforess commented 7 years ago

I'll re-evaluate when 0.15 drops. For now, the plan is to remove the existing implementation for the 0.15 release and if you want to contribute a fresh Inventory module with new functions, I'm all for that. I'm just not sure that it's worth tangling with right now, unless you really really want to.