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

InvRef: Add convenience method add_list for bulk transfer of items #8466

Closed
wants to merge 1 commit into from

Conversation

ClobberXD
Copy link
Contributor

@ClobberXD ClobberXD commented Apr 11, 2019

  • New Lua API method InvRef:add_list(dst_listname, src_listname) for bulk transfer of items from one inventory list to another.
  • Returns the leftover items as an inventory list.
  • New internal function push_inventory_list_raw (declared/defined in c_content.{h|cpp}) to directly push an InventoryList * object to the Lua stack. Existing function push_inventory_list now makes use of push_inventory_list_raw internally.

To test:

  • Install and enable luacmd mod.
  • Populate at least 10 slots with unique items in the main inventory list.
  • Type the following into the console:
    /lua inv = me:get_inventory()
    /lua inv:set_list("main", inv:add_list("craft", "main"))

The above chat-command transfers all items in main to the crafting grid and sets the leftover back to main. Since there were more than 9 unique items in main, there's bound to be at least one item as leftover.


Closes #7663. Tested, works as expected.

@Desour
Copy link
Member

Desour commented Apr 11, 2019

I think, this would be more useful if it worked between different inventories, too.

@ClobberXD ClobberXD force-pushed the InvRef_add_list branch 2 times, most recently from 5508653 to 3842ae7 Compare April 12, 2019 06:41
@ClobberXD
Copy link
Contributor Author

Fixed src list being destroyed after the bulk transfer. Apparently, I was trying to push a non-existent list to the Lua stack, because I was using push_inventory_list in spite of using an independent InventoryList * object. See the updated first post for more details.

I think, this would be more useful if it worked between different inventories, too.

I agree. I'll see what I can do.

InventoryList *src_list = inv->getList(src_listname);

// Create new list to store leftovers
InventoryList *leftover_list = new InventoryList("leftover",
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need it as pointer?
InventoryList leftover_list("leftover", src_list->getSize(), getServer(L)->getItemDefManager());
EDIT: and push_inventory_list_raw(L, &leftover_list);

const char *src_listname = luaL_checkstring(L, 3);

InventoryList *dst_list = inv->getList(dst_listname);
InventoryList *src_list = inv->getList(src_listname);
Copy link
Member

Choose a reason for hiding this comment

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

getList may return nullptr is that list does not exist. Check for 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.

Will do.

@SmallJoker
Copy link
Member

SmallJoker commented Apr 13, 2019

The above chat-command transfers all items in main to the crafting grid

So they're moved? The code does indicate that the list is copied,.
Here's how transferring item lists would work entirely in Lua:

local src = inv:get_list("main")
for i = 1, #src do
	src[i] = dst:add_item("craft", src[i])
end
inv:set_list("main", src)

@ClobberXD
Copy link
Contributor Author

@SmallJoker Correct - they're moved. After all, this is a convenience method to achieve exactly what the Lua code you posted does - bulk transfer of items from one list to another.

The name might cause some confusion, though. add_list sounds like a method that creates a new inv. list. Should I rename it to something like move_list?

@SmallJoker
Copy link
Member

@ClobberXD move_list would indeed fit better. Although I really doubt this function is generic enough to be helpful for most mods. It makes the list item transfer easier, but a five lines of Lua code can do the same - which is a very little amount of code.
I'm not entirely against this PR, but would like to hand over reviewing & approving to the other devs.

@ClobberXD
Copy link
Contributor Author

I too am starting to doubt the necessity of this convenience function, given the implementation effort. But that got me thinking, would it make sense to implement this in Lua and add it to builtin?

@ClobberXD
Copy link
Contributor Author

Closing due to lack of solid use-cases and lack of core dev interest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ✨ PRs that add or enhance a feature @ Script API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lua API: Provide convenience function InvRef:add_list
4 participants