hopper reimplementation #3980
No reviewers
Labels
No Label
#P1 CRITICAL
#P2: HIGH
#P3: elevated
#P4 priority: medium
#P6: low
#Review
annoying
API
bug
code quality
combat
commands
compatibility
configurability
contribution inside
controls
core feature
creative mode
delayed for engine release
documentation
duplicate
enhancement
environment
gameplay
graphics
ground content conflict
GUI/HUD
help wanted
incomplete feature
invalid / won't fix
items
looking for contributor
mapgen
meta
mineclone2+
Minecraft >= 1.13
Minecraft >= 1.17
missing feature
mobile
mobs
mod support
model needed
multiplayer
Needs adoption
needs discussion
needs engine change
needs more information
needs research
nodes
non-Minecraft feature
non-mob entities
performance
player
possible close
redstone
release notes
schematics
Skyblock
sounds
Testing / Retest
tools
translation
unconfirmed
mcl5
mcla
Media missing
No Milestone
No project
No Assignees
6 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: VoxeLibre/VoxeLibre#3980
Loading…
Reference in New Issue
No description provided.
Delete Branch "Morik666/MineClone2:hopper"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
In response to issues 2013 and 1780
Reimplemented hoppers and all (blast_furnace, furnace, smoker, composters, double chests, shulker_boxes and droppers) connected nodes.
Had to add internal inventory to composters. After merge all old composters have to be replaced so inventoris can be created.
Have tested insertion and extraction from and into all (blast_furnace, furnace, smoker, composters, double chests, shulker_boxes and droppers) connected nodes by hoppers.
Also tested insertion into chests by droppers.
Probably, should be tested by another pair of eyes.
In case of any criticism would be glad to make changes.
Composter is malfunctioning when trying to pull items from it. It sometimes worked, and sometimes not. May be worth looking into that.
@ -314,0 +292,4 @@
local dst_list = 'main'
local stack_id
if dst_def._mcl_hoppers_on_try_push ~= nil then
This is not a boolean value, so no need for the
~= nil
– justif dst_def._mcl_hoppers_on_try_push then
will suffice.@ -438,0 +325,4 @@
local src_list = 'main'
local src_inv, stack_id
if src_def._mcl_hoppers_on_try_pull ~= nil then
Same thing as with push.
@ -574,6 +600,29 @@ minetest.register_node("mcl_blast_furnace:blast_furnace_active", {
_mcl_hardness = 3.5,
on_rotate = on_rotate,
after_rotate = after_rotate_active,
_mcl_hoppers_on_try_pull = function(pos, hop_pos, hop_inv, hop_list)
See smoker.
@ -577,0 +614,4 @@
end
return nil, nil, nil
end,
_mcl_hoppers_on_try_push = function(pos, hop_pos, hop_inv, hop_list)
See smoker.
@ -913,6 +941,34 @@ local function register_chest(basename, desc, longdesc, usagehelp, tt_help, tile
end,
mesecons = mesecons,
on_rotate = no_rotate,
_mcl_hoppers_on_try_pull = function(pos, hop_pos, hop_inv, hop_list)
Same issue as furnaces. Make a function and reuse it.
@ -916,0 +955,4 @@
stack_id = mcl_util.select_stack(inv, "main", hop_inv, hop_list)
return inv, "main", stack_id
end,
_mcl_hoppers_on_try_push = function(pos, hop_pos, hop_inv, hop_list)
...this too.
@ -1487,0 +1548,4 @@
--- Returns false if itemstack is a shulker box
---@param itemstack ItemStack
---@return boolean
function is_not_shulker_box(stack)
Should be local or namespaced (i.e. in mcl_chests).
@ -593,2 +620,4 @@
on_rotate = on_rotate,
after_rotate = after_rotate_active,
_mcl_hoppers_on_try_pull = function(pos, hop_pos, hop_inv, hop_list)
See smoker.
@ -595,0 +634,4 @@
end
return nil, nil, nil
end,
_mcl_hoppers_on_try_push = function(pos, hop_pos, hop_inv, hop_list)
See smoker.
@ -579,6 +605,29 @@ minetest.register_node("mcl_smoker:smoker_active", {
_mcl_hardness = 3.5,
on_rotate = on_rotate,
after_rotate = after_rotate_active,
_mcl_hoppers_on_try_pull = function(pos, hop_pos, hop_inv, hop_list)
Code duplicated (multiplicated considering furnance and blast furnance). Make a reusable function outside the definition and add it to both definitions. You can define that function in mcl_furnaces, since mcl_smoker depends on that anyway (and mcl_blast_furnace does too).
@ -582,0 +619,4 @@
end
return nil, nil, nil
end,
_mcl_hoppers_on_try_push = function(pos, hop_pos, hop_inv, hop_list)
Code duplicated. See above.
Could not find code references between furnaces, smokers, and blast_furnaces. So made a pair of functions in each.
Made
mcl_chests
namespace. Functionmcl_chests.is_not_shulker_box
cannot be local. If it is it doesn't get passed tomcl_util.select_stack
. Had to debug it for an hour.Methods
_mcl_hoppers_on_try_push
and_mcl_hoppers_on_try_pull
for big chest are different (for left part they try moving items from itself and only then go for "right" part, in case of right it's going for the left first and only then check itself). Are you sure you want it generalized and reused? IMHO code will be bloated and won't be used for anything else anyway.Dropped
~= nil
in function checks.Checked composters. Strange. Could not replicate. Was working fine for me for hopper pulling and right-clicking... What was your testing set-up?
You can make the function a part of the mcl_furnace global.
Also mark parts of the review which you implemented as solved.
@ -326,0 +305,4 @@
local stack_id
if dst_def._mcl_hoppers_on_try_push then
dst_inv, dst_list, stack_id = dst_def._mcl_hoppers_on_try_push(dst_pos, pos, hop_inv, hop_list)
dst_inv
is not declared as local@ -326,0 +307,4 @@
if dst_def._mcl_hoppers_on_try_push then
dst_inv, dst_list, stack_id = dst_def._mcl_hoppers_on_try_push(dst_pos, pos, hop_inv, hop_list)
else
dst_meta = minetest.get_meta(dst_pos)
...and neither is
dst_meta
In terms of chests, you're right.
In terms of composters, looks like a problem with ABMs and not with your code. There are more problems with the ABM, including some other problems with hoppers that I suspect are due to ABMs.
Also, do an interactive rebase and remove the merge commits if that was just updating your branch to master.
157fb3ff66
to1bbe0464d0
1bbe0464d0
to93dd0e5dd8
93dd0e5dd8
tod82621624d
Made requested changes, rebase and squash
It looks like the line endings changed (CR/LF). Take a look at those files, because there are too many lines changed with no actual changes.
a5621d85f8
toca1dc0d938
ca1dc0d938
tob822e0b596
sorry. linux endings only should be now
It turns out something is indeed broken with composters. They work arbitrarily many cycles, and then stop working at some point in the hopper setups. In the attached screenshot, there are two hoppers putting saplings into the composter (ignore the hopper on the left, it's unrelated) and the hopper on the bottom trying to take an item from it. It fails though, as is evident from bone meal being on top of the composter, ready to be harvested (and indeed, it can be harvested by hand, which unlocks the composter for another few cycles).
@ -281,0 +358,4 @@
local meta = minetest.get_meta(pos)
local inv = meta:get_inventory()
local stack = inv:get_stack("dst", 1)
if not stack:is_empty() and hop_inv:room_for_item(hop_list, stack) then
Putting right before this
if
line a line likeminetest.log(dump(stack:get_name()))
, I am getting what is in the second screenshot attached to the review. The empty string means there is nothing in the stack, it is empty, and it is sent every second for every attempt by ABM to pull it out into hopper... until you remove the bone meal by hand. Then it starts working again. The string with the name appears during a successful hopper pull, and there are a few successful cycles as it is evident from the screenshot... after which it breaks again and it returns to logging empty strings.Strange issue. Spent an evening and couldn't figure out where the roots of the problem were.
Rewriten to generate item on request.
Run a test until composter accepted 15 stacks of seeds with no issues. After item transfer internal inventory was empty every time.
b822e0b596
to3aba1347c9
3aba1347c9
toe91b2af607
Everything aside of those 3 lines looks fine.
Two hoppers pointing at each other (a form of a redstone clock, clocks built from more hoppers – at least 4 – work properly) basically freeze their state, as shown in the screenshot. Basically, ABM always triggers them at the same order, so first one gives away the item and the other one, in the same tick, gives it back. Do you want to seek a way to fix it? This is not a regression though, so if you call it a day, we can merge this (unless someone else finds a problem/regression).
@ -281,0 +367,4 @@
_mcl_hoppers_on_after_pull = function(pos)
local meta = minetest.get_meta(pos)
local inv = meta:get_inventory()
local stack = inv:get_stack("dst", 1)
These variables have no use here. Seems misplaced (mispasted?).
Ok, found one more missing feature: Brewing stands should work with hoppers. From the top you feed the ingredients, and from the side fuel and bottles.
e91b2af607
to455ddbe016
@ -107,0 +87,4 @@
end
--- Math and node swap during compost progression
---@param pos Vecotr Position of the node
Typo Vecotr -> Vector
There should be a noted delay of at least a "redstone tick" between receiving and returning.
A simple-ish fix would be to set a flag on the receiving node if it is a hopper, and changing the abm to look for the flag first, and unsetting the flag and then exit, if it is set. That will grant a single "tick" delay. (Obv. if a longer delay is needed, then instead of a flag, use an int, and when the int is at a specific count, set it to zero. same basic idea though.)
Offering this, as I use hopper clocks often in MC, and this will eventually have to be implemented.
Rejecting the ABM means a delay of one second I think.
Anyway, it's a good idea to use the new API to add special handling for hopper pushing into hopper.
@Morik666 are you already on that clocks thing? If no, then leave it alone and implement the rest, I'll handle the clocks later.
Didn't have time for it yet. Got covid again. Will try to find some time on this weekend.
Have some ideas on clock implementation.
PS. Do you have discord or any other chat group? Have implemented backpacks, and wanted to know if you are interested in PR. There is a lot of space for discussion about inventory size and crafting cost. Open an issue for that?
Here's the discord:
https://discord.gg/NAVZU9wD
37393e0c84
to99e5f6e964
Brewing was tough. Had to add new group to potions to distinguish them from ingredients. Implemented pushing into stand. Did not implement pulling from it. Only condition that could think of is the last stage of potion. But how correct that assumption is?
Fixed typo.
Should I also implement protection (minetest.is_protected)? Not sure if existing protections can be circumvented by my hoppers. If yes, how can I test it?
Didn't touch ABM locking for a clock yet.
99e5f6e964
tocd66761b50
cd66761b50
to3e2dfd9595
3e2dfd9595
to34fb689159
Removed leftover lines and implemented redstone clock.
https://gyazo.com/86f3aaf04f85ce0d8c067fd8592f72ed
Splash and lingering potions don't have a
bottle
group, so hoppers don't push them into stands.Regarding pulling from brewing stands, you can check if it is not brewing, so it would only let a hopper pull potions when it is not brewing. Hoppers would probably only be used in multi-stand setups anyway, so eg. one hopper pushes in netherwarts into a stand receiving water bottles from the side, then the result is pulled and pushed from the side into another stand, which is receiving another ingredient from the top.
To support protection, you would have to save an owner (probably placer) in the hopper block metadata, and then check through
minetest.is_protected()
whether the owner of the hopper has the access to the block from which the hopper is trying to pull or to which the hopper is trying to push. I don't think the current master has anything like that implemented. Personally, I'd leave implementing protection checks for another PR, even if you want to implement those. I mean, if you're placing containers on the edge of your protected area, somebody could push into or pull from them, but other than that you're safe anyway.Protection implementation is left for mods, so you would have to install a separate protection mod to test that and work on that.
@ -379,0 +391,4 @@
function(stack) return minetest.get_item_group(stack:get_name(), "bottle") == 1 end)
end
end
end
Look at
on_put()
above. Hopper putting in potions doesn't update the block (i.e. the potion is invisible when looking at the stand, only visible in its inventory). May also be doneafter_push
.I also did some tests and I can see theses use cases doesn't work. If it's intended, please let me know
Put a book in a hopper on top of a bookshelf, the book doesn't go in the bookshelf. Same, book doesn't go out if a hopper is below.
Same with disc for jukebox
Impossible to move item from hopper to an ender chest and out from it too. (same in minecraft)
a blocked hopper (redstone power) will receive item from another hopper. It should not, a locker hopper should block item both in and out
Can't put coal in a furnace minecart (same in minecraft)
![image](/VoxeLibre/VoxeLibre/attachments/c0c91746-86ee-431d-9d77-6d2870759c0f)
Can't load a chest boat, or unload it with a hopper below
![image](/VoxeLibre/VoxeLibre/attachments/33421457-10c6-411e-a565-ee1a3f115e78)
34fb689159
to81fe823914
81fe823914
to8392311536
8392311536
to2dfdd6c8b4
Fixed splash and lingering potions. Added pulling from stand.
Thanks for extensive testing, @Araca
Extended support for bookshelvs.
Regarding jukebox, music for playing requires connection to player. The weren't working with hoppers before and fix requires rewriting jukebox itself. Keeping it for later.
As I understand ender chests aren't supported for balance and chunk loading reasons.
Made blocked hoppers (redstone powered) to prevent item transfer.
Furnace minecart I'll leave for later. It wasn't working before and I'm not comfortable enough to fix it now.
Regarding chest boat, I didn't even know it existed. Added ability to push into it. Does pulling from it should work? If yes, then how?
2dfdd6c8b4
to25a447ab3f
I broke a block below the boat and put a hopper. I don't know if it's relevant to have it pulling item from a chest boat, but if it's an easy one why not. Some creative mind may find a use. I think it's up to you, or another opinion to decide.
In your last code version, item are pull from a upper hopper into an ender chest , but not from a side hopper and from ender chest to the hopper. I don't know if it's intended so.
I found another issue. An hopper can't be unloaded at the same time it is loaded. So if I stack two hoppers in a chest and I put a stack in the upper hopper, first the full stack go to the middle hopper and THEN this middle hopper is unloaded to the chest.
![image](/VoxeLibre/VoxeLibre/attachments/c8b075d8-3173-422c-bcf2-42df98971c30)
I found this in the minecratf wiki, that I think it's not the case "While a locked hopper does not push or pull/collect items, it may still receive items from dispensers, droppers and other hoppers, and may have its items pulled out by another hopper beneath it"
In your MR I tested at least this case "A locked hopper should receive hopper from other hopper". I don't know why there is this specific behaviour in MC, but it makes the etho's hopper clock work in MC and not in your MR. If it starts to be too much for you, we can create another ticket I guess.
Add pulling from a chestboat identical to pulling from a chest-minecart.
Also, hopper-minecart doesn't work at all. You can try hooking those minecarts up to your hopper API. They should only interact with blocks and not with other entities, so should be simple enough.
If it turns out to require more work, don't do it now. It's time for final touch-ups and we're merging once it's done.
Rest looks fine: ender chests don't work with hoppers at all, and shouldn't. Locked hoppers seem to work properly (i.e. not work). etc.
Couldn't reproduce. Hoppers don't work with enderchests at all, and shouldn't, and for a good reason.
Couldn't reproduce.
So, the locked hopper should only be different from the normal hopper in the fact that it:
but still has the API functions (callbacks used by other hoppers/hopper minecarts) defined like a normal hopper... or a chest?
You said yourself though that
now you're requesting it the other way? Which is it?
@the-real-herowl I couldn't reproduce either ender chest and hopper unload/load in the same time, sorry for that.
Regarding the locked hopper, I was a too vague in the comment last week.
You translate the functional behavior to technical. The wiki talks only about hopper (no chest/hopper minecart).
Wiki says locked hopper will
25a447ab3f
to7c169b6846
7c169b6846
toab313fc238
Using the Etho hopper clock as an example (because it is better for server lag to have this available) it uses pistons to move a redstone block between two hoppers that face each other. The unlocked hopper pushes a stack of items into the locked hopper, and when the unlocked hopper is empty, the piston shifts to the other hopper (now filled up).
Locked hoppers should receive items from the listed item pushers (Hoppers, dispensers, droppers) but nothing else. And, they do not pull items out - the items have to be pushed in.
Having this functionality working is part of getting the redstone into shape for the game.
ab313fc238
to44af2997e8
It looks like hoppers aren't the problem with the etho's clock, pistons are (and ABMs).
Do you have some queries regarding the hopper minecart implementation, @Morik666 ?
44af2997e8
to1da7bb0bdc
Unlike minecart, boats swim 0.2 below the block it sits on top of. While one can swim directly under hopper, it's impossible to climb onto hopper from water. So instead of hopper pulling through 0.8 of water block height one would need to park next to a hopper.
Minecart is not so straightforward, thou. Will need to alter mcl_util.hopper_push and mcl_util.hopper_pull to accept both nodes and entities as either parameter. Not sure how to handle it yet. Let's close this PR and I'll work on item transfer for entities in next PR.