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 API for explosions to damage the explosion cause #11180

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EsotericEnderman
Copy link

This PR is to add an API feature which expands the functionality of World#createExplosion by adding the option to deal damage to the source entity (this is not the default behaviour). This deals with this issue (not technically a bug, but still something that I think should be addressed).

The API is expanded by overloading existing World#createExplosion methods with a new boolean shouldDamageSource parameter. When true, the entity specified as the source will be damaged by the explosion. Existing behaviour is not changed.

@EsotericEnderman EsotericEnderman requested a review from a team as a code owner July 27, 2024 19:04
@EsotericEnderman EsotericEnderman force-pushed the feature/expand-explosions-api branch 4 times, most recently from b9f018f to 03717d4 Compare August 6, 2024 21:52
Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay on the initial review, this PR seems to have fallen completely under my radar.

In general, when expanding a heavily overloaded API like this, we'd only want to add one more method.

In this case public boolean createExplosion(@Nullable Entity source, @NotNull Location loc, float power, boolean setFire, boolean breakBlocks, boolean damageSource);.
Callers of as niche of an API as the damageSource flag will be, can be expected to fully define all other parameters. The amount of methods added by this patch is way too many.

Beyond that, generally LGTM, but yea, please reduce this to a single added method on World, preferably just the method signature mentioned above. Your changes will also end up in the Expand-Explosions-API patch, so if you want to merge your changes into that patch directly, that would be great. Our contribution doc has a tutorial explaining how to edit patches.

@EsotericEnderman
Copy link
Author

Thanks for reviewing. I'll make sure to change what you requested ASAP.

I'm currently on holidays, but I'll get to it as soon as I'm back.

@EsotericEnderman EsotericEnderman force-pushed the feature/expand-explosions-api branch 2 times, most recently from 4d9afd8 to ae12cce Compare August 23, 2024 17:59
@EsotericEnderman
Copy link
Author

Sorry, didn't realise that would close the PR.

So far, I've reduced the PR to add a single method, as well as merge the changes into the Expand-Explosions-API patch, as requested.

However, the build is currently failing at the javadoc task, not sure why.

@EsotericEnderman
Copy link
Author

I believe I have fixed the issue just now.

@EsotericEnderman EsotericEnderman force-pushed the feature/expand-explosions-api branch 3 times, most recently from 0b0f86d to 707c91a Compare August 23, 2024 22:14
@EsotericEnderman
Copy link
Author

I just tested this with the test plugin, and it does damage the source entity as intended, but there's an issue with the explosion sounds and particles, which I'll look into tomorrow.

@EsotericEnderman EsotericEnderman force-pushed the feature/expand-explosions-api branch 2 times, most recently from 109d8ff to 30d66e4 Compare August 24, 2024 15:05
@EsotericEnderman
Copy link
Author

Just fixed the issue with particles and sound.

@EsotericEnderman
Copy link
Author

Just tested this using the test plugin and everything works perfectly.

@EsotericEnderman EsotericEnderman force-pushed the feature/expand-explosions-api branch 3 times, most recently from e9df61b to eaf2f59 Compare September 14, 2024 15:23
@lynxplay
Copy link
Contributor

lynxplay commented Sep 20, 2024

The more I look at the diff the less happy I am with it.
The explosion invocation code is already ugly in vanilla with the large amounts of overload.
Having to add a boolean param to that many method seems annoying.

Not that we can really get around overloading internal methods here, but maybe a Consumer would be more suited to avoid further diff if we expand the API even more?

E.g. something like these two
https://pastebin.com/4c1ZhtL0
https://pastebin.com/J6QwPZ4Q

@EsotericEnderman EsotericEnderman force-pushed the feature/expand-explosions-api branch 2 times, most recently from 0d86679 to 5e73eed Compare September 26, 2024 19:13
This intends to give plugin developers more control over explosions created using the World#createExplosion method, specifically by adding the option for explosions to damage the explosion cause (not the default behavior, and previously impossible to do, as far as I know). This is done by overloading existing methods with an extra `excludeSourceFromDamage` parameter.
@EsotericEnderman
Copy link
Author

The more I look at the diff the less happy I am with it. The explosion invocation code is already ugly in vanilla with the large amounts of overload. Having to add a boolean param to that many method seems annoying.

Not that we can really get around overloading internal methods here, but maybe a Consumer would be more suited to avoid further diff if we expand the API even more?

E.g. something like these two https://pastebin.com/4c1ZhtL0 https://pastebin.com/J6QwPZ4Q

I implemented these changes and everything seems to work.

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

Successfully merging this pull request may close these issues.

2 participants