-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
Master 1.19 lts #1441
Master 1.19 lts #1441
Conversation
considered string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts on this PR:
- The nbt-clear approach is very straightforward, and seems like a good option. However, this will cause the original block the facade was bound to, to be lost. We could introduce a custom recipe handler that returns the original block as container item (similar to recipes returning empty buckets). The main problem of that approach, is that clearing facades in bulk won't be possible anymore due to the container item being placed in the crafting grid. So bulk crafting would become very painful in that case. Not sure that we best approach here is, as both have their pros and cons.
- This PR seems to contain two changes (recipe + operator change). Could these two be split into separate PRs? This keeps the commit history (and changelog) a bit cleaner.
- I noticed that you are re-merging your previously merged master branch, which will create a weird commit history. It's better to fetch from upstream and checkout the remote master branch, create a new feature branch from that, and PRing that feature branch. Then, only your new commits will be added to the PR.
I can definitely make sure to fix my branch setup and make a new branch for each PR, as for obtaining the block back, we might be able to allow the user to squeeze the block out of the facade and keep losing the block as a convenience recipe? Failing that, I'm fine implementing either method you described above. |
So if I understand correctly, you suggest keeping the current approach (losing the block)? If so, I can live with that :-) |
I was suggesting 2 recipes, a convenience that is lossy and what we have now, and a squeezer recipe that isn't |
Sure, two recipes can work. But how shall we distinguish them so they have different inputs? |
I was thinking the convenience one being in the normal 2x2 crafting grid, and the one that returns the block being a mechanical squeezer recipe, and if it supports multiple outputs (I don't remember without looking), a non-mechanical squeezer recipe as well. |
Ah yes, that's a good idea! |
Closes issue #1424