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

Fix mob always drop exp when killed #823

Merged
merged 15 commits into from Feb 4, 2018

Conversation

Projects
None yet
4 participants
@XuZhen86
Contributor

XuZhen86 commented Jan 25, 2018

This pull request tries to fix the bug that mob always drop exp when killed.

Observed behavior:
Mobs always drop exp when killed, even drown in water without player touching them.

Expected behavior:

A mob will not drop experience unless it dies within five seconds (100 game ticks) of an attack registered as a player hit (including tamed wolves and TNT).
Experience - Official Minecraft Wiki

Solution:

  1. Add a countdown to track the 100 game ticks.
  2. Enable countdown when a valid source hit the mob.
  3. Set the killer when it kills the mob.
  4. Add AnimalTamer to GlowPlayer . Set the killer to the owner of the wolf when wolf kills the mob.
  5. Mob only drops exp when player is in survival mode.

Possible issues:

  1. The bow and arrow is still under development, the killer setting of arrow is not implemented.
  2. It gives me java.lang.IllegalArgumentException: Compound does not contain ByteTag "CollarColor" whenever I place a wolf using wolf egg?
  3. The check style seems to crashed it self, I had to disable it to continue the compile.

XuZhen86 added some commits Jan 25, 2018

*/
@Getter
@Setter
private int hitByPlayerCountdown;

This comment has been minimized.

@momothereal

momothereal Jan 25, 2018

Member

Wouldn't it be simpler to have a field like playerDamageTick that is set to the current ticksLived when the entity is hit, then check if ticksLived - playerDamageTick <= 100 when killed?

// or damaged by a tamed wolf
if (
(source instanceof GlowPlayer
&& ((GlowPlayer)source).getGameMode() == GameMode.SURVIVAL)

This comment has been minimized.

@momothereal

momothereal Jan 25, 2018

Member

Should adventure mode also be checked?

@@ -24,6 +24,10 @@
private int fuseTicks;
private Entity source;
@Getter
@Setter
private GlowPlayer player;

This comment has been minimized.

@momothereal

momothereal Jan 25, 2018

Member

Add Javadoc for this

XuZhen86 added some commits Jan 25, 2018

Small fix
1. Simplified player damage tick determine process
2. Extracted chunks of code to isPlayerHit() and determinePlayer()
3. Improved Javadoc
@@ -730,7 +739,8 @@ public void setHealth(double health) {
if (world.getGameRuleMap().getBoolean("doMobLoot")) {
LootData data = LootingManager.generate(this);
deathEvent.getDrops().addAll(data.getItems());
if (data.getExperience() > 0) {
// Only drop experience when hit by a player within 5 seconds (100 game ticks)
if (ticksLived - playerDamageTick <= 100 && data.getExperience() > 0) {

This comment has been minimized.

@momothereal

momothereal Jan 25, 2018

Member

You'll probably want to have playerDamageTick default to -1 (in order to know if the entity wasn't hit yet), and add a check here for it being positive.

if (playerDamageTick >= 0 && ticksLived - playerDamageTick <= 100 && data.getExperience() > 0)

This comment has been minimized.

@XuZhen86

XuZhen86 Jan 26, 2018

Contributor

I made it private int playerDamageTick = -101;, it solves the problem without extra code.

@@ -14,20 +14,33 @@
public class BlockTnt extends BlockType {
/**
* Convert a TNT block into a primed TNT entity.
* Convert a TNT block into a primed TNT entity with the player who ignited the tnt.

This comment has been minimized.

@momothereal

momothereal Jan 25, 2018

Member

tnt -> TNT (in the @param as well)

playerDamageTick = ticksLived;
}
if (health - amount <= 0) {

This comment has been minimized.

@momothereal

momothereal Jan 25, 2018

Member

Should this if-statement be inside of the isPlayerHit if-block above?

This comment has been minimized.

@XuZhen86

XuZhen86 Jan 25, 2018

Contributor

Strictly saying it doesn't matter, they both determine the source. But put it inside does make more sense.

This comment has been minimized.

@Pr0methean

Pr0methean Jan 26, 2018

Contributor

Put it inside. It'll save a few instructions when !isPlayerHit(source)

@@ -830,6 +849,47 @@ public void damage(double amount, Entity source, DamageCause cause) {
setLastDamager(source);
}
/**
* Checks if the incoming source of damage is a "Player Hit".
* @param source The incoming source of damage

This comment has been minimized.

@momothereal

momothereal Jan 25, 2018

Member
/**
 * Checks if the source of damage was caused by a player.
 *
 * @param source The source of damage
 * @return true if the source of damage was caused by a player, false otherwise.
 */
// If damaged by a TNT ignited by a player
} else if (source instanceof GlowTntPrimed) {
return
((GlowTntPrimed)source).getPlayer() != null

This comment has been minimized.

@momothereal

momothereal Jan 25, 2018

Member

Casts should have a space after the closing parentheses. There's a couple to fix here.

Ex.: ((GlowTntPrimed) source).getPlayer() != null

Also you can store the GlowTntPrimed instance before returning because you are casting it multiple times:

GlowTntPrimed tnt = (GlowTntPrimed) source;
return tnt.getPlayer() != null [...]
return (Player)((GlowTntPrimed)source).getPlayer();
}
// If been killed by a player
else if (source instanceof GlowPlayer) {

This comment has been minimized.

@momothereal

momothereal Jan 25, 2018

Member

Since you are returning inside the if-blocks, the elses are redundant and can be omitted.

This comment has been minimized.

@Pr0methean

Pr0methean Jan 26, 2018

Contributor

Should we maybe use a VicariousKillerEntity or MeansOfKilling interface to simplify all of this?

This comment has been minimized.

@XuZhen86

XuZhen86 Jan 26, 2018

Contributor

I don't think that's necessary. There are only several cases that require checking vicarious killing.

*/
@Getter
@Setter
private GlowPlayer player;

This comment has been minimized.

@momothereal

momothereal Jan 25, 2018

Member

Why are this variable and getter/setter necessary? The getSource() method should return the player who ignited the TNT (if any), according to docs.

This comment has been minimized.

@Pr0methean

Pr0methean Jan 26, 2018

Contributor

IOW:

public GlowPlayer getPlayer() {
    Entity source = getSource();
    return (source instanceof GlowPlayer) ? (GlowPlayer) source : null;
}
public void setPlayer(Player player) {
    source = player;
}
if (source instanceof GlowPlayer) {
return true;
// If damaged by a TNT ignited by a player
} else if (source instanceof GlowTntPrimed) {

This comment has been minimized.

@momothereal

momothereal Jan 25, 2018

Member

The elses are redundant and can be omitted (return statement inside of the if-blocks).

@momothereal

This comment has been minimized.

Member

momothereal commented Jan 25, 2018

The exception caused by spawning a Wolf using an egg is now fixed in 12df4a7.

return null;
} else {
return source;
}

This comment has been minimized.

@Pr0methean

Pr0methean Jan 26, 2018

Contributor

return (source != null && source.isValid()) ? source : null;

XuZhen86 added some commits Jan 26, 2018

}

This comment has been minimized.

@momothereal

momothereal Jan 27, 2018

Member

Remove whitespace (keep only 1 line between methods)

@@ -195,7 +196,7 @@
* @author Graham Edgecombe
*/
@DelegateDeserialization(GlowOfflinePlayer.class)
public class GlowPlayer extends GlowHumanEntity implements Player {
public class GlowPlayer extends GlowHumanEntity implements Player, AnimalTamer {

This comment has been minimized.

@momothereal

momothereal Jan 27, 2018

Member

I think this is redundant because GlowPlayer extends GlowHumanEntity, which implements HumanEntity, which extends AnimalTamer. (alternatively, GlowPlayer -> Player -> HumanEntity -> AnimalTamer)

*
* @param player Player that ignited the TNT
*/
public void setPlayer(Player player) {

This comment has been minimized.

@momothereal

momothereal Jan 29, 2018

Member

Can you remove this setter and just use setSource with the player? I don't think having a separate method here is necessary.

*/
public static void igniteBlock(Block tntBlock, boolean ignitedByExplosion) {
public static void igniteBlock(
GlowBlock tntBlock, boolean ignitedByExplosion, GlowPlayer player) {

This comment has been minimized.

@Pr0methean

Pr0methean Feb 1, 2018

Contributor

Why do you have to narrow tntBlock from Block to GlowBlock? Things like that make unit testing more complicated.

This comment has been minimized.

@XuZhen86

XuZhen86 Feb 1, 2018

Contributor

It's more consistent within the class. All other methods use Glowblock.

This comment has been minimized.

@mastercoms

mastercoms Feb 1, 2018

Member

I agree with Pr0methean, it should be kept like Block, unless there is a specific reason for doing it otherwise. The other usages of GlowBlock as a method parameter are not reasons for this one to use it.

@mastercoms mastercoms merged commit 89a3784 into GlowstoneMC:dev Feb 4, 2018

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment