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

Master 1.19 lts #1441

Closed
Closed

Conversation

kirjorjos
Copy link
Contributor

Closes issue #1424

Copy link
Member

@rubensworks rubensworks left a 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.

@kirjorjos
Copy link
Contributor Author

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.

@rubensworks
Copy link
Member

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?

So if I understand correctly, you suggest keeping the current approach (losing the block)? If so, I can live with that :-)

@kirjorjos
Copy link
Contributor Author

kirjorjos commented Dec 20, 2024

I was suggesting 2 recipes, a convenience that is lossy and what we have now, and a squeezer recipe that isn't

@rubensworks
Copy link
Member

Sure, two recipes can work. But how shall we distinguish them so they have different inputs?

@kirjorjos
Copy link
Contributor Author

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.

@rubensworks
Copy link
Member

Ah yes, that's a good idea!

@kirjorjos kirjorjos closed this by deleting the head repository Dec 21, 2024
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

Successfully merging this pull request may close these issues.

2 participants