150 bones inventory slots and register_transfer_inventory_to_bones_on_player_death callback

[imre84]

in this PR there are two changes

first, the size of the bones inventory were changed to 15*10

rationale of these changes:

there might be a scenario when a bones inventory would need to hold up to 4*8+6+4*3*8+4+3*3 distinct items:

• 4*8 for the main inventory
• 6 for the 3d_armor
• (at most) 4*3*8 for 4 backpack worth of items (unified inventory)
• 4 more for the actual backpacks
• 3*3 more for the crafting grid

that adds up to 147, so 150 slots would be sufficient

secondly, I made a public interface in bones to provide a way for other mods to register callbacks to transfer items to bones on player death.

rationale of these changes:

3d armor for instance tries to search for a bones block in the vicinity of the death:
https://github.com/minetest-mods/3d_armor/blob/master/3d_armor/init.lua#L371
given all the precautions there might be an ambiguity (the same player dying on the same spot in the past 20 minutes)
also a scenario is possible when the main inventory and the crafting grid of the player was empty, in this case 3d armor will just drop its items disregarding the bones_mode configuration

also, please consider unified_inventory where backpack related items are stored in detached inventories, making the pre-existing api unable to handle these inventories

[appgurueu]

As I already wrote on IRC, I don't think a fixed "upper bound" of 150 slots is a decent solution.

First of all, a few more items and 150 slots are too few again; this needs to be variable.

Second, most of the time most of these slots will be unused, especially in vanilla MTG, worsening UX.
I think the size of the bones should depend on how many items they contain, and 4*8 should be a lower bound to not break hacks such as 3d_armor's.

Third, all items making their way to the bones might break game mechanics: Currently, 3d_armor will just drop the armor if it doesn't fit in the bones (correct me if I'm wrong). With this change, they'd always be in the bones, thus there'd be no point in quick retrieval of them, and no point in keeping slots free in your inventory; really, if you think of it, the size limitation of the bones may be considered a gameplay feature. Not sure what backpacks etc. do.

I definitely support the addition of an API. That way, mods which want everything to always go in the bones should be able to make it happen. Here's my proposal: Allow adding items to the bones via a callback which is called on player death and returns the items to be added. If the items exceed 4*8, expand the inventory accordingly. Otherwise, leave it untouched.

[appgurueu]

• You don't need parenthesis around conditions.
• You don't need the entire private/public stuff. This isn't Java.
• Get rid of the global variables applying to the current bones. They should instead be local variables (local to the function placing the bones, not local to the file).

[appgurueu]

• Fix the Luacheck warnings. This includes eliminating global variables.
• Your code style still doesn't match the rest of the code. You use excessive spacing around parenthesis and parenthesis where none are needed.
• My point about a variable size was not that there should be a configurable max, but rather that after the callbacks have been run, the size should be determined based on how many items were added; the minimum size should be 4*8 to support 3d_armor's hack.
• What happened to the logging?
• Your usage of translations is wrong (see review comments).

[imre84]

* My point about a variable size was not that there should be a configurable max, but rather that after the callbacks have been run, the size should be determined based on how many items were added; the minimum size should be 4*8 to support 3d_armor's hack.

also, please take a look at https://github.com/imre84/minetest_game/blob/master/mods/bones/init.lua#L33
the displayed size of the grid depends on the actual number of items inside, and has nothing to do with the configurable max (other than serving as an upper bound)

[imre84]

• Fix the Luacheck warnings. This includes eliminating global variables.

can I run luachecks somehow?

[appgurueu]

Some variable names and some minor formatting could use improvement.

Can you try to clean up the code and in particular the highly imperative loop where you lazily add bones if there is an item to be placed a bit?

[imre84]

where you lazily add bones

the original functionality also drops a bone if the bone block cannot be placed, I wanted to retain this functionality. Is there a better way to do this?

[appgurueu]

Sorry for the late review. The logic checks out, but is harder to follow than it needs to be. Consider refactoring as suggested in my comment.

There are also some minor typos here and there that I'll fix in one go before merge when this gets a second approval.

[appgurueu]

I feel it should be possible to refactor this loop by splitting it up into functions:

• A function to collect the items to be added to the inventory in a list. This function would run the callbacks and append all the items to a list.
• A function to find a suitable position for a bones node.
• A function to set and populate the inventory of set node.

Perhaps the latter two functions should be only one. You might also want to inline these, but the important thing is that I'd like to have this as a clear-cut three stage process:

1. Collect items to be placed in the inventory
2. Find a position to place the block
3. Place the block and populate the inventory

in order to get rid of the very stateful loop with high cyclomatic complexity

[appgurueu]

Refactored.

[fluxionary]

not sure if this is super relevant, but someone suggested that i link my bones fork here:

https://content.minetest.net/packages/rheo/bones/

it's got an API for adding additional inventories, and comes w/ support for 3d_armor, unified_inventory, and i3. it also has a lot of other additional features, and due to #2710, i didn't think it was appropriate to try to get them integrated here.

[appgurueu]

It's difficult to draw the line between features and maintenance. I see dehardcoding and adding APIs as maintenance rather than "adding features"; it is necessary in this case such that mods like 3d_armor don't have to rely on unreliable hacks anymore. Effectively there will be bugs (or at least very hacky code) in 3d armor and many other mods which add player inventories which are induced by us not offering a proper API.

Note however that these dependencies have go the other way around: MTG will not make itself dependant on 3d armor, i3, unified inventory, etc. These mods (and any mods that may come in the future) have to use the game APIs we offer to interact with MTG.

[imre84]

hello, have anything happened to this PR? is it incorporated into mtg5.8.0?

[fluxionary]

hello, have anything happened to this PR? is it incorporated into mtg5.8.0?

if the core devs don't respond, feel free to PR any fixes to https://github.com/fluxionary/minetest-bones_redo. it's already in use on a couple popular servers, and i'm happy to maintain it.

[SmallJoker]

Rebase needed. I support this minor feature and will review it. The "maintenance-only" state that MTG currently has should not be the reason for requiring mods to provide helpful functionality of MTG mods.

[imre84]

can somebody kindly please point me towards an RTFM regarding how to solve this conflict? I cannot think of an easy way for git mergetool to work given the fact that the conflict is between repositories and not inside a single repository

[imre84]

all done, formspec vs scrollbars was a bit fidgety, I hope you like it

[SmallJoker]

@imre84 Thank you for the scroll container. This should scale much better on mobile platforms with limited screen space. Does the scroll container fit well on your side? Asking because I get the following situation when inserting about 9 additional stacks: (using the "craft" fields)

[imre84]

this is how it looks like for me

[imre84]

is it any better now?

[imre84]

also current behaviour is to display the first 4*8 slots even if they're empty, which is a nightmare from UX perspective, some form of slot reordering would be beneficial

[imre84]

ok, I'm not sure if bones inventory reordering is over the top, maybe it should be nerfed somehow, idk

[SmallJoker]

The scrollbar does fit much better now. Perhaps the hacky scrollbar size calculation can be fixed in the future directly within Minetest. We'll see.
Your calculation does work well on my end. Thanks.

I also tested the partially dropped inventory case, which also works as expected.
See comments for the last remaining issues.

[imre84]

ok, hopefully all done, let me know if I missed something

[SmallJoker]

Works.