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 block explode event & fix exploded blocks change method #3990

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

AyhamAl-Ali
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali commented May 12, 2021

Description

  • This PR adds BlockExplodeEvent that is also triggered by create explosion effect (Added support for it to Explosion Yield Expr and Exploded Blocks Expr)
  • Also, this PR fixes changing method of exploded blocks to allow all kind of changes to it such as:
on block explode:
	add blocks in radius 2 around event-location to exploded blocks
	set exploded blocks to blocks in radius 2 around event-location
	remove blocks in radius 2 around event-location from exploded blocks
	remove chest from exploded blocks # Removes 1 block that matches a chest block type from exploded blocks
	remove all sand from exploded blocks # Removes all blocks that matches a sand block type
	loop exploded blocks:
		loop-block is sand
		remove loop-block from exploded blocks

Target Minecraft Versions: All
Requirements: None
Related Issues: #3759

Copy link
Member

@FranKusmiruk FranKusmiruk left a comment

Choose a reason for hiding this comment

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

What do you think about merging the two events into a single SkriptEvent like on [(entity|block)] explode? This event would trigger for both entity and block explosions if not especified in the pattern. You'd also need to create an expression like (explosion cause|detonator) which would return either an Entity or a Block depending on the event.

Also, make sure to implement the Expression#iterator method on ExprExplodedBlocks, this way users can be allowed to delete the looped value in loops.

@TPGamesNL TPGamesNL added 2.6 feature Pull request adding a new feature. labels May 12, 2021
@AyhamAl-Ali
Copy link
Member Author

AyhamAl-Ali commented May 12, 2021

Sure, I will do all that tonight (or maybe next month 🤔 my bad got so busy lately)

- Merging entity and block explosions  
- Added `explosion cause` that will return a string value ("block" or "entity")  
- Improved exploded blocks expression  
- Added a method suggested by Snow-Pyon for later use
src/main/java/ch/njol/skript/events/EvtExplode.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/util/Utils.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/util/Utils.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtExplode.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtExplode.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/util/Utils.java Outdated Show resolved Hide resolved
NOTE: getReturnType() of explosion cause is Object.class, if that could cause an issue and get() method is called before getReturnType() then I might be able to set isEntity in get() method and use it in getReturnType() method
src/main/java/ch/njol/skript/events/EvtExplode.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtExplode.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtExplode.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/util/Utils.java Outdated Show resolved Hide resolved
@AyhamAl-Ali AyhamAl-Ali mentioned this pull request Apr 12, 2022
1 task
src/main/java/ch/njol/skript/events/EvtExplode.java Outdated Show resolved Hide resolved

@Override
@Nullable
public Iterator<? extends Block> iterator(Event e) {
Copy link
Member

Choose a reason for hiding this comment

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

Is overriding iterator necessary here?

}

@Override
public @Nullable <R> Expression<? extends R> getConvertedExpression(Class<R>... to) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason behind this method? If there is one, write a comment in the code for, otherwise remove it

}

@Override
public void change(Event e, final @Nullable Object[] delta, final ChangeMode mode) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public void change(Event e, final @Nullable Object[] delta, final ChangeMode mode) {
public void change(Event e, @Nullable Object[] delta, ChangeMode mode) {

break;
} else if (amount > 0)
for (Block b : new ArrayList<>(blocks)) {
if (amountRemoved >= amount)
Copy link
Member

Choose a reason for hiding this comment

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

What would happen when you use remove 5 dirt and 2 stone from exploded blocks? Looks like it'll not remove any stone, but could be wrong

if (!getParser().isCurrentEvent(EntityExplodeEvent.class)) {
Skript.error("The 'explosion block yield' is only usable in an explosion event", ErrorQuality.SEMANTIC_ERROR);
if (!getParser().isCurrentEvent(BlockExplodeEvent.class, EntityExplodeEvent.class)) {
Skript.error("Explosion block yield can only be retrieved from an explode event.");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Skript.error("Explosion block yield can only be retrieved from an explode event.");
Skript.error("Explosion block yield can only be retrieved from an explode event");

Comment on lines +74 to +76
@Override
@SuppressWarnings({"unchecked", "rawtypes"})
public @Nullable <R> Expression<? extends R> getConvertedExpression(Class<R>... to) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Override
@SuppressWarnings({"unchecked", "rawtypes"})
public @Nullable <R> Expression<? extends R> getConvertedExpression(Class<R>... to) {
@Override
@Nullable
@SuppressWarnings({"unchecked", "rawtypes"})
public <R> Expression<? extends R> getConvertedExpression(Class<R>... to) {

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment to this method for why it's here?

* @param filter the {@link Predicate} / filter that will be checked to remove the elements
* @return amount of elements that matched the filter and have been removed.
*/
public <E> int removeFirstIf(Collection<E> coll, int n, Predicate<E> filter) {
Copy link
Member

Choose a reason for hiding this comment

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

This method is not used with the current version of this PR. If you think it can still be useful in the future, it's fine to keep it, but otherwise change the first arg from a Collection to a List, as a collection isn't ordered, and therefore the first n elements doesn't make sense for all collections.

image


for (; each.hasNext() && n > 0; n--) {
if (filter.test(each.next())) {
each.remove();
Copy link
Member

Choose a reason for hiding this comment

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

Iterator#remove is an optional operation, it is not guaranteed to be available. You should probably switch to removing from the collection directly.

Co-authored-by: TPGamesNL <29547183+TPGamesNL@users.noreply.github.com>
@AyhamAl-Ali AyhamAl-Ali mentioned this pull request Feb 18, 2023
1 task
@AyhamAl-Ali AyhamAl-Ali marked this pull request as draft April 22, 2023 00:04
@AyhamAl-Ali AyhamAl-Ali added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Apr 22, 2023
@AyhamAl-Ali
Copy link
Member Author

Coming Back Soon 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants