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
Expand Events #330
Expand Events #330
Conversation
public interface PlayerBedEvent extends PlayerEvent { | ||
|
||
/** | ||
* Gets the location of the player in respect to the bed. |
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.
This should probably be Gets the location of the player *before entering the bed*
instead.
/** | ||
* Called when an {@link Entity} is killed or removed due to unload. | ||
*/ | ||
public interface EntityDeathEvent extends EntityEvent, CauseTracked, Cancellable { | ||
|
||
/** | ||
* Gets a copy of the {@link ItemStack}s this entity will drop on death. |
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.
Why a copy? Could this list be mutable?
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.
I think that's addressed by setDrops
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.
It is, but seems unnecessary since List
has methods to make it simpler
event.getDrops().add(event.getGame().getRegistry().getItemBuilder() ...);
Is neater than
List<ItemStack> drops = event.getDrops();
drops.add(event.getGame().getRegistry().getItemBuilder() ...);
event.setDrops(drops);
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.
I disagree with leaving lists mutable, I'd rather have copies handed around to prevent manipulating or holding references of live ItemStacks following death events.
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.
Is there not a way to use a mutable list, without leaking references to itemstacks afterwards? Like using an editable snapshot which doesn't get used again?
What about |
@ST-DDT I'm not sure how event causes are going to work but that sounds like something that would be described in the cause. |
What about them? Are you copying the events we had from Bukkit? |
@simon816 Maybe, i'm not sure about using @gabizou What? I only suggested them or similar things to be added. |
* | ||
* @param drops The list of drops the entity will drop on death | ||
*/ | ||
void setDrops(List<ItemStack> drops); |
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.
are set drops removed from the players inventory, or will removing an item from the drops not replace it in the players inventory?
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.
I think the player will lose everything on death, and this is what it drops. (Correct me if I am wrong)
* Gets the clicked position of this interact event. | ||
* | ||
* <p>This may not always be available, in which case | ||
* {@link Optional#absent()} is returned. Specifically, when a player |
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.
The clicked position is sent by the client for all entities. The only time it's not available is for blocks.
Is this event going to be subclassed to handle interacting with entities, or are those method going to be added to this class?
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.
The clicked position is sent by the client for all entities
No it is not. Again, refer to the protocol.
The only time it's not available is for blocks.
It's almost always available for blocks.
Is this event going to be subclassed to handle interacting with entities, or are those method going to be added to this class?
No. It isn't necessary since PlayerInteractEntity extends PlayerInteract.
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.
No it is not. Again, refer to the protocol.
I've looked at MCP - two packets get sent for all interactions, except for ArmorStand
: only INTERACT_AT
is sent. Even if it wasn't, there's no reason it couldn't be calculated server-side.
Additionally, I've seen thing while working on handling the Use Entity packet in Glowstone.
a4749d8
to
e3317ba
Compare
Just wondering should PlayerExpChangeEvent not have a setCurrent. It would avoid the whole getPlayer().setExperience() thing. And it would allow multiple plugins to change/modify the experience gain. |
ef45426
to
41875fe
Compare
int getTotalExperinece(); | ||
|
||
/** | ||
* Sets the experience accumulated towards the next level. |
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.
There should be a note about how much experience advances a player to the next level.
public interface HumanLeaveBedEvent extends HumanSleepEvent { | ||
|
||
/** | ||
* Gets whether the spawn location for the player was set. |
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.
There should be a note explaining why this could be false.
Some events I thought of: EntityAddStatusEffectEvent |
5a112fa
to
458d5f4
Compare
* Gets the clicked position of this interact event. | ||
* | ||
* <p>This may not always be available, in which case | ||
* {@link Optional#absent()} is returned. Specifically, when a player |
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.
Currently, the location is sent for all entities (although it's only used for AmorStand
s)
From what I can tell we are missing SignChangeEvent. Maybe make it extend BlockChangeEvent |
The problem with doing this is that the event is a "state". Changing the experience via
Will add.
Adding in a different way, but being added nonetheless.
Could you elaborate on what this does?
Already covered by
Perhaps a more generic way to do this?
Will add.
Perhaps can be handled via
Will add. |
@gabizou BlockHitEvent is essentially when a player starts breaking a block. Bukkit had a BlockDamageEvent that did the same thing and allowed you to get/setInstant(). You might say you could do the same with interact and then set the block to air, but if you want drops or other vanilla block breaking handling or something it's useful. |
c6c539c
to
97b3e76
Compare
MerchantTradeEvent? perhaps with TradeOffer getOffer(), ItemStack getFirstStack() and ItemStack getSecondStack()? |
import org.spongepowered.api.item.inventory.ItemStack; | ||
|
||
/** | ||
* An event when a {@link Furnace} |
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.
Missing docs?
23c3449
to
e4912e7
Compare
e4912e7
to
8fdacff
Compare
- Living - Human - Player - TileEntity
Adds the following events:
Modifies the following events:
More events are to follow as seen fit.