hopper reimplementation #3980

Merged
the-real-herowl merged 1 commits from Morik666/MineClone2:hopper into master 2023-11-28 00:34:52 +01:00
Member

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.

In response to issues [2013](https://git.minetest.land/MineClone2/MineClone2/issues/2013) and [1780](https://git.minetest.land/MineClone2/MineClone2/issues/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.
the-real-herowl added this to the 0.85.0 - Fire and Stone milestone 2023-10-27 01:03:22 +02:00
the-real-herowl requested changes 2023-10-27 06:04:33 +02:00
the-real-herowl left a comment
Owner

Composter is malfunctioning when trying to pull items from it. It sometimes worked, and sometimes not. May be worth looking into that.

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 – just if dst_def._mcl_hoppers_on_try_push then will suffice.

This is not a boolean value, so no need for the `~= nil` – just `if dst_def._mcl_hoppers_on_try_push then` will suffice.
Morik666 marked this conversation as resolved
@ -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.

Same thing as with push.
Morik666 marked this conversation as resolved
@ -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.

See smoker.
Morik666 marked this conversation as resolved
@ -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.

See smoker.
Morik666 marked this conversation as resolved
@ -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.

Same issue as furnaces. Make a function and reuse it.
the-real-herowl marked this conversation as resolved
@ -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.

...this too.
the-real-herowl marked this conversation as resolved
@ -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).

Should be local or namespaced (i.e. in mcl_chests).
Morik666 marked this conversation as resolved
@ -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.

See smoker.
Morik666 marked this conversation as resolved
@ -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.

See smoker.
Morik666 marked this conversation as resolved
@ -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).

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).
Morik666 marked this conversation as resolved
@ -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.

Code duplicated. See above.
Morik666 marked this conversation as resolved
Author
Member

Could not find code references between furnaces, smokers, and blast_furnaces. So made a pair of functions in each.

Made mcl_chests namespace. Function mcl_chests.is_not_shulker_box cannot be local. If it is it doesn't get passed to mcl_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?

Could not find code references between furnaces, smokers, and blast_furnaces. So made a pair of functions in each. Made `mcl_chests` namespace. Function `mcl_chests.is_not_shulker_box` cannot be local. If it is it doesn't get passed to `mcl_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?
Morik666 requested review from the-real-herowl 2023-10-27 20:27:03 +02:00

Could not find code references between furnaces, smokers, and blast_furnaces. So made a pair of functions in each.

You can make the function a part of the mcl_furnace global.

> Could not find code references between furnaces, smokers, and blast_furnaces. So made a pair of functions in each. > You can make the function a part of the mcl_furnace global.

Also mark parts of the review which you implemented as solved.

Also mark parts of the review which you implemented as solved.
the-real-herowl requested changes 2023-10-28 03:02:58 +02:00
@ -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

`dst_inv` is not declared as local
Morik666 marked this conversation as resolved
@ -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

...and neither is `dst_meta`
Morik666 marked this conversation as resolved

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.

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.

Also, do an interactive rebase and remove the merge commits if that was just updating your branch to master.
Morik666 force-pushed hopper from 157fb3ff66 to 1bbe0464d0 2023-10-28 17:45:20 +02:00 Compare
Morik666 force-pushed hopper from 1bbe0464d0 to 93dd0e5dd8 2023-10-28 17:52:31 +02:00 Compare
Morik666 force-pushed hopper from 93dd0e5dd8 to d82621624d 2023-10-28 18:02:42 +02:00 Compare
Author
Member

Made requested changes, rebase and squash

Made requested changes, rebase and squash
Morik666 requested review from the-real-herowl 2023-10-28 18:07:15 +02:00
the-real-herowl requested changes 2023-10-28 18:52:16 +02:00
the-real-herowl left a comment
Owner

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.

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.
Morik666 force-pushed hopper from a5621d85f8 to ca1dc0d938 2023-10-28 19:14:17 +02:00 Compare
Morik666 force-pushed hopper from ca1dc0d938 to b822e0b596 2023-10-28 19:16:01 +02:00 Compare
Author
Member

sorry. linux endings only should be now

sorry. linux endings only should be now
Morik666 requested review from the-real-herowl 2023-10-28 19:18:16 +02:00
the-real-herowl requested changes 2023-10-29 02:46:54 +02:00
the-real-herowl left a comment
Owner

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

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 like minetest.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.

Putting right before this `if` line a line like `minetest.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.
Author
Member

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.

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.
the-real-herowl marked this conversation as resolved
Morik666 force-pushed hopper from b822e0b596 to 3aba1347c9 2023-10-30 18:33:06 +01:00 Compare
Morik666 force-pushed hopper from 3aba1347c9 to e91b2af607 2023-10-30 18:47:45 +01:00 Compare
Morik666 requested review from the-real-herowl 2023-10-30 19:00:36 +01:00
the-real-herowl requested changes 2023-10-31 03:13:32 +01:00
the-real-herowl left a comment
Owner

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

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

These variables have no use here. Seems misplaced (mispasted?).
Morik666 marked this conversation as resolved
the-real-herowl requested changes 2023-10-31 03:45:22 +01:00
the-real-herowl left a comment
Owner

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.

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.
the-real-herowl force-pushed hopper from e91b2af607 to 455ddbe016 2023-10-31 14:15:29 +01:00 Compare
AFCMS reviewed 2023-10-31 18:15:05 +01:00
@ -107,0 +87,4 @@
end
--- Math and node swap during compost progression
---@param pos Vecotr Position of the node
Member

Typo Vecotr -> Vector

Typo Vecotr -> Vector
Morik666 marked this conversation as resolved
Member

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.

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.

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

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.

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.

> > 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. > > 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.
Author
Member

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?

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?

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

> 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
Morik666 force-pushed hopper from 37393e0c84 to 99e5f6e964 2023-11-07 21:02:15 +01:00 Compare
Author
Member

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.

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.
Morik666 requested review from the-real-herowl 2023-11-07 21:15:58 +01:00
Morik666 force-pushed hopper from 99e5f6e964 to cd66761b50 2023-11-11 02:36:56 +01:00 Compare
Morik666 force-pushed hopper from cd66761b50 to 3e2dfd9595 2023-11-11 02:43:04 +01:00 Compare
Morik666 force-pushed hopper from 3e2dfd9595 to 34fb689159 2023-11-11 02:45:37 +01:00 Compare
Author
Member

Removed leftover lines and implemented redstone clock.
https://gyazo.com/86f3aaf04f85ce0d8c067fd8592f72ed

Removed leftover lines and implemented redstone clock. https://gyazo.com/86f3aaf04f85ce0d8c067fd8592f72ed
the-real-herowl requested changes 2023-11-13 00:10:19 +01:00
the-real-herowl left a comment
Owner

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.

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 done after_push.

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 done `after_push`.
Morik666 marked this conversation as resolved
Contributor

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

  • Can't load a chest boat, or unload it with a hopper below
    image

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](/attachments/c0c91746-86ee-431d-9d77-6d2870759c0f) * Can't load a chest boat, or unload it with a hopper below ![image](/attachments/33421457-10c6-411e-a565-ee1a3f115e78)
193 KiB
118 KiB
Morik666 force-pushed hopper from 34fb689159 to 81fe823914 2023-11-21 20:32:52 +01:00 Compare
Morik666 force-pushed hopper from 81fe823914 to 8392311536 2023-11-21 20:42:55 +01:00 Compare
Morik666 force-pushed hopper from 8392311536 to 2dfdd6c8b4 2023-11-21 21:04:00 +01:00 Compare
Author
Member

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?

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?
Morik666 force-pushed hopper from 2dfdd6c8b4 to 25a447ab3f 2023-11-21 21:20:19 +01:00 Compare
Morik666 requested review from the-real-herowl 2023-11-21 21:20:24 +01:00
Contributor

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?

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.

image

As I understand ender chests aren't supported for balance and chunk loading reasons.

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

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.

> 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? 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. ![image](/attachments/19c95a3a-cc91-4eec-a407-f5cda8d6be83) > As I understand ender chests aren't supported for balance and chunk loading reasons. 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](/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.
the-real-herowl requested changes 2023-11-23 02:09:05 +01:00
the-real-herowl left a comment
Owner

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.

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.

As I understand ender chests aren't supported for balance and chunk loading reasons.

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.

Couldn't reproduce. Hoppers don't work with enderchests at all, and shouldn't, and for a good reason.

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.

Couldn't reproduce.

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.

So, the locked hopper should only be different from the normal hopper in the fact that it:

  • doesn't run the push/pull against other nodes
  • doesn't collect dropped items

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

a blocked hopper (redstone power) will receive item from another hopper. It should not, a locker hopper should block item both in and out

now you're requesting it the other way? Which is it?

> > As I understand ender chests aren't supported for balance and chunk loading reasons. > > 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. Couldn't reproduce. Hoppers don't work with enderchests at all, and shouldn't, and for a good reason. > 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. Couldn't reproduce. > 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. So, the locked hopper should only be different from the normal hopper in the fact that it: - doesn't run the push/pull against other nodes - doesn't collect dropped items 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 > a blocked hopper (redstone power) will receive item from another hopper. It should not, a locker hopper should block item both in and out now you're requesting it the other way? Which is it?
Contributor

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

but still has the API functions (callbacks used by other hoppers/hopper minecarts) defined like a normal hopper... or a chest?

You translate the functional behavior to technical. The wiki talks only about hopper (no chest/hopper minecart).
Wiki says locked hopper will

  • receive item from hopper, dispenser, dropper
  • push item to a below hopper
@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. > but still has the API functions (callbacks used by other hoppers/hopper minecarts) defined like a normal hopper... or a chest? You translate the functional behavior to technical. The wiki talks only about hopper (no chest/hopper minecart). Wiki says locked hopper will - receive item from hopper, dispenser, dropper - push item to a **below** hopper
Morik666 force-pushed hopper from 25a447ab3f to 7c169b6846 2023-11-24 01:00:59 +01:00 Compare
Morik666 force-pushed hopper from 7c169b6846 to ab313fc238 2023-11-24 01:01:36 +01:00 Compare
Member

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.

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.
the-real-herowl force-pushed hopper from ab313fc238 to 44af2997e8 2023-11-26 02:23:35 +01:00 Compare

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 ?

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 ?
Morik666 force-pushed hopper from 44af2997e8 to 1da7bb0bdc 2023-11-27 13:41:14 +01:00 Compare
Author
Member

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.

image

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.

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. ![image](/attachments/df54f5f7-583a-4907-b78b-eba67bf245b3) 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.
Morik666 requested review from the-real-herowl 2023-11-27 13:47:54 +01:00
the-real-herowl approved these changes 2023-11-28 00:34:26 +01:00
the-real-herowl merged commit 19728c5a19 into master 2023-11-28 00:34:52 +01:00
the-real-herowl removed the
Testing / Retest
label 2023-11-28 04:39:43 +01:00
Sign in to join this conversation.
No reviewers
No project
No Assignees
6 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: VoxeLibre/VoxeLibre#3980
No description provided.