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

150 bones inventory slots and register_transfer_inventory_to_bones_on_player_death callback #3030

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

Conversation

imre84
Copy link
Contributor

@imre84 imre84 commented May 5, 2023

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
Copy link
Contributor

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.

Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

  • 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).

mods/bones/init.lua Outdated Show resolved Hide resolved
game_api.txt Outdated Show resolved Hide resolved
game_api.txt Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
@appgurueu
Copy link
Contributor

  • 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
Copy link
Contributor Author

imre84 commented May 5, 2023

* 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 imre84 force-pushed the master branch 2 times, most recently from 77168cb to 5a28778 Compare May 6, 2023 09:44
@imre84
Copy link
Contributor Author

imre84 commented May 6, 2023

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

can I run luachecks somehow?

@imre84 imre84 requested a review from appgurueu May 10, 2023 07:04
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

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?

mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
@imre84
Copy link
Contributor Author

imre84 commented May 12, 2023

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?

@imre84 imre84 requested a review from appgurueu May 23, 2023 14:57
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

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.

end
return
end
for _, fun in ipairs(dead_player_callbacks) do
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Refactored.

@fluxionary
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

imre84 commented Feb 18, 2024

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

@fluxionary
Copy link
Contributor

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.

mods/bones/init.lua Outdated Show resolved Hide resolved
@SmallJoker
Copy link
Member

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.

@SmallJoker SmallJoker self-requested a review February 24, 2024 12:21
@imre84
Copy link
Contributor Author

imre84 commented Feb 24, 2024

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
Copy link
Contributor Author

imre84 commented Mar 28, 2024

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

@SmallJoker
Copy link
Member

SmallJoker commented Mar 30, 2024

@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)

scroll Y max

@imre84
Copy link
Contributor Author

imre84 commented Mar 30, 2024

Képernyőkép ekkor: 2024-03-30 18-14-03
this is how it looks like for me

mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
@imre84
Copy link
Contributor Author

imre84 commented Mar 30, 2024

is it any better now?

@imre84
Copy link
Contributor Author

imre84 commented Mar 30, 2024

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
Copy link
Contributor Author

imre84 commented Mar 30, 2024

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

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

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.

mods/bones/init.lua Show resolved Hide resolved
mods/bones/init.lua Outdated Show resolved Hide resolved
mods/bones/init.lua Show resolved Hide resolved
@imre84
Copy link
Contributor Author

imre84 commented Mar 31, 2024

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

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Works.

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

Successfully merging this pull request may close these issues.

5 participants