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

[Feature Request] API: Alternatively use callbacks as pipeworks #4

Open
lnjX opened this issue Apr 6, 2017 · 7 comments
Open

[Feature Request] API: Alternatively use callbacks as pipeworks #4

lnjX opened this issue Apr 6, 2017 · 7 comments

Comments

@lnjX
Copy link

lnjX commented Apr 6, 2017

The hoppers API is nice, but in my case I don't have an inventory with lists, I use meta fields to store this data, because I only have one item. (FYI I'm talking abot the drawers mod; https://github.com/minetest-mods/drawers)

It would be nice if the API could alternatively try to call a callback of the nodedef if the node is in a "hopper_receiver" group. It could be even the same callback as in pipeworks ("on_insert_object(pos, node, stack, direction)").

@FaceDeer
Copy link
Member

FaceDeer commented Apr 7, 2017

The subject of drawers compatibility came up a few days back on the hopper forum thread, but I got a bit sidetracked when I discovered that hoppers had a major unrelated bug. Thanks for reminding me. :)

When I started thinking about this initially, my first idea was to see if I could add a real inventory to drawers that the metadata-based storage could use as a "buffer" to interact with other code. I didn't get very far into trying to actually code it up, so I don't know how well it'll work, but if I could pull that off it would allow drawers to present a standard inventory API for any mod to make use of (not just hoppers). I still feel like it might be a good idea to try that, since pipeworks' API support for inventory transfer is only partial (it doesn't have on_take_object, for example). But if that proves difficult or fragile I'd certainly be amenable to adding a "backup" API to hoppers to allow for this sort of situation to work.

I'm going to be busy for the next few days but my weekend's largely free, I'll do some exploratory coding and see if my ideas can translate into something vaguely practical.

@lnjX
Copy link
Author

lnjX commented Apr 7, 2017

I'd also be happy to see a PR by you for the drawers mod ofc! I also think that your solution might be better, if it's not too complicated. 😃

@FaceDeer
Copy link
Member

FaceDeer commented Apr 7, 2017

One way or another, we shall interconnect all the things! :)

@FaceDeer
Copy link
Member

FaceDeer commented Apr 9, 2017

I think I'm going to be able to pull off the "dummy inventory" idea in drawers (though of course it remains to be seen whether you'll take it), but in the process I've just identified a significant bug in hopper that I need to fix (until now it's never tried working with a node that cares about which specific inventory slot an item is being added to).

It's turning out that the trickiest part may be fixing the hopper bug in a suitably generic way. I think Minetest's api didn't really expect inventories to care about which specific slot things went into either, even though it's got a parameter for checking that. Some of the functions I was relying on just check if there's room for an item anywhere in an inventory, which is convenient for the usual inventories but not for these four-drawer ones where the index indicates what drawer stuff is in.

So it may well turn out to be much simpler to add those fallback functions after all. :(

@lnjX
Copy link
Author

lnjX commented Apr 10, 2017

I also first thought about using normal inventories, but it was just easier to directly use meta fields. And I currently have more important to-dos on the drawers mod (automatic sorting system, drawer upgrades, etc.).
I personally don't think that anyone will really directly connect a drawer with a hopper, when the sorting system is finished. I'll use a normal inventory for the drawer controllers, so then hoppers will be connectable to the controllers.

@FaceDeer
Copy link
Member

That may be for the best. There's nothing wrong with drawers' basic approach, it works quite well. But it is rather different from how "standard" inventories work, so it would take some contortions to make it work generically. I'm actually pleasantly surprised by how far I got yesterday making it work - I got the dummy inventories set up for drawers, I just couldn't make hoppers hook up to them easily thanks to some omissions in the minetest API itself (if your curious to see what I was doing, here's the branch I was working in for that)

Speaking of branches, I'm still somewhat new to this Git thing so I'm not habituated to all these forking and branching shenannigans everyone gets up to. :) The changes I sent the pull request for were just the "get it working on my machine at all" steps I had to do before starting into the stuff that might or might not be worth sending so I didn't think to make a branch for it. Sorry about that.

Anyway. I will still look into adding those fallback function hooks to hopper when I do the work to fix the bug I uncovered, at this point I'm not feeling so bad about doing that because I've discovered enough flaws in the Minetest inventory API to make doing that not so bad in comparison. :)

@lnjX
Copy link
Author

lnjX commented Apr 10, 2017

(if your curious to see what I was doing, here's the branch I was working in for that)

You can also open a work-in-progress PR on the drawers repo, so everyone can already see that you're working on this. I had a look at it, 👍.

Speaking of branches, I'm still somewhat new to this Git thing so I'm not habituated to all these forking and branching shenannigans everyone gets up to. :) The changes I sent the pull request for were just the "get it working on my machine at all" steps I had to do before starting into the stuff that might or might not be worth sending so I didn't think to make a branch for it. Sorry about that.

No problem, this: minetest-mods/drawers#1 was my fault, but it is generally better for you if you don't mess up your master branch, if you want to do a PR just do all of your work into a new branch. FYI it's no problem (before and after you have commited) to move your changes into a new branch.
For example you've made your changes in the master branch:

# See your current changes
git status
# fork the current branch (master) into a new and enter it
git checkout -b new-feature-branch-name
# commit your changes
git add .
git commit
# publish them onto a new branch (set upstream is only needed for the first time)
git push --set-upstream origin new-feature-branch-name

And an example after it (so you already have made the commit on the master branch):

# publish your new commits onto a new branch
git checkout -b new-feature-branch-name
git push --set-upstream origin new-feature-branch-name
# reenter master branch
git checkout master
# you can take a look at the commit history now
git log
# revert all new commits
git revert <commit id of the last official commit>
# if you've reverted too many commits, pull them from the remote server
git pull

I personally don't have a local clone of my private fork (I think the most people have not). I clone the official repo and create new local branches for the new features and push them to a second remote server (my fork). You can add new remotes using git remote add <name> <url> and then you can set a different remote repo in git push --set-upstream remote-name branch-name.

Oops .. I didn't want to write a new git tutorial.

My first pull request: BlockMen/minetest_next#23 (I also used the master branch and BlockMen told me not to do so 😂)

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

No branches or pull requests

2 participants