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

Add BlockBreakEffectsCallback #1023

Open
wants to merge 17 commits into
base: 1.16
Choose a base branch
from

Conversation

TheBrokenRail
Copy link

@TheBrokenRail TheBrokenRail commented Aug 18, 2020

Sometimes you might not want the block break particles and sounds to appear for a block. However, the only good way to do this (if you want access to the breaking entity) is a @Redirect in World#breakBlock. This has already caused a conflict between Magna and EnergonRelics.

		BlockBreakEffectsCallback.EVENT.register((world, breakingEntity, pos) -> {
			BlockState state = world.getBlockState(pos);
			return state.getBlock() == Blocks.BEDROCK ? ActionResult.FAIL : ActionResult.PASS;
		});

@Geometrically
Copy link
Contributor

Should probably add a testmod too

@TheBrokenRail
Copy link
Author

TheBrokenRail commented Aug 18, 2020

I did add a testmod though?

@i509VCB i509VCB added enhancement New feature or request reviews needed This PR needs more reviews labels Aug 20, 2020
@i509VCB i509VCB requested review from liach and a team August 20, 2020 20:51
@i509VCB i509VCB added the test label Aug 20, 2020
Co-authored-by: liach <7806504+liach@users.noreply.github.com>
@liach liach requested a review from i509VCB August 21, 2020 18:27
Copy link

@Vaerian Vaerian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good code wise, but I guess I just wonder if maybe the two events ought to be separated. Because both methods are using the same event signature it means we're limited to the common information between the two methods and we're missing out on some extra information that could be useful in certain situations. I guess it really comes down to the comment I made at the BlockMixin, I don't specifically know what each of these methods have in terms of parameters, so I don't know what kind of useful information they might be able to relay to the event. You know these methods better than I do, so if you think someone might benefit from having those additional variables passed in then I might recommend the two events get separated for greater control.

@TheBrokenRail
Copy link
Author

What do you mean? There isn't any information that isn't common between the two mixins. They both have the entity that broke the block, the position of the block, the block state, and the world. The Block.onBreak mxin only has two world and two players because of how @Redirects work, they are literally the same object. player == player2

@TheBrokenRail
Copy link
Author

Is there anything else I should do?

Copy link
Contributor

@i509VCB i509VCB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further consideration, I believe the block mixin will suffice.

@TheBrokenRail
Copy link
Author

Any update on this? Should I rebaee it on 1.17?

@i509VCB
Copy link
Contributor

i509VCB commented Dec 14, 2020

Keep this on 1.16, we can cherry pick at merge time

@i509VCB i509VCB requested a review from a team January 19, 2021 20:40
@i509VCB i509VCB added the priority:low Low priority PRs that don't immediately need to be resolved label Jan 19, 2021
@LambdAurora LambdAurora requested a review from a team May 20, 2021 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority:low Low priority PRs that don't immediately need to be resolved reviews needed This PR needs more reviews test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants