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

Implements Enderpearls & Fixes a bug with water/lava buckets in creative mode #602

Closed
wants to merge 8 commits into from

Conversation

@Mimerme
Copy link

@Mimerme Mimerme commented Dec 11, 2017

  • Water & lava buckets are no longer emptied in creative mode
  • Enderpearls have been implemented
@CLAassistant
Copy link

@CLAassistant CLAassistant commented Dec 11, 2017

CLA assistant check
All committers have signed the CLA.

@@ -15,7 +15,8 @@
public static final int ITEM_FRAME = 71;
public static final int FIREWORK = 76;
public static final int LEASH_HITCH = 77;

public static final int THROWN_ENDERPEARL = 65;

This comment has been minimized.

@mastercoms

mastercoms Dec 11, 2017
Member

Could you put this after ENDER_CRYSTAL but before ITEM_FRAME?

holding.setType(Material.BUCKET);
}
}
}


//Empties the user's inventory when the item is used up

This comment has been minimized.

@mastercoms

mastercoms Dec 11, 2017
Member

Add a space between the // and the comment.

This comment has been minimized.

@mastercoms

mastercoms Dec 12, 2017
Member

Add a space between // and the comment.

if (holding.getType() == Material.WATER_BUCKET
|| holding.getType() == Material.LAVA_BUCKET) {
if ((holding.getType() == Material.WATER_BUCKET
|| holding.getType() == Material.LAVA_BUCKET) && player.getGameMode() != GameMode.CREATIVE) {

This comment has been minimized.

@mastercoms

mastercoms Dec 11, 2017
Member

Put && player.getGameMode() != GameMode.CREATIVE) on a new line.



x = location.getX();
//Add 1.5m to account for eye level

This comment has been minimized.

@mastercoms

mastercoms Dec 11, 2017
Member

Add a space between the // and the comment.


setRawLocation(velLoc);

//If the EnderPearl collides with anything except air/fluids

This comment has been minimized.

@mastercoms

mastercoms Dec 11, 2017
Member

Add a space between the // and the comment.


@Override
public boolean doesBounce() {
// TODO Auto-generated method stub

This comment has been minimized.

@mastercoms

mastercoms Dec 11, 2017
Member

Is there a reason for these comments? The methods don't look auto-generated anymore.


public class ItemEnderPearl extends ItemType {

@Override

This comment has been minimized.

@mastercoms

mastercoms Dec 11, 2017
Member

Please use 4 spaces for indentations.


private void throwEnderPearl(GlowPlayer player) {
Location throwLoc = player.getLocation().clone();
//throwLoc.setY(player.getEyeHeight());

This comment has been minimized.

@mastercoms

mastercoms Dec 11, 2017
Member

Why is this commented out?

@mastercoms
Copy link
Member

@mastercoms mastercoms commented Dec 11, 2017

Thank you for your contribution to Glowstone! I've reviewed your PR and it looks good overall, just a few tweaks here and there and we should be good to merge. 👍

@Mimerme
Copy link
Author

@Mimerme Mimerme commented Dec 12, 2017

Everything should be alright now I hope :)

Copy link
Member

@mastercoms mastercoms left a comment

Thanks for the changes, there's some more small issues that I found though. Sorry about not catching them earlier.

import net.glowstone.entity.objects.GlowEnderPearl;

public class ItemEnderPearl extends ItemType {

This comment has been minimized.

@mastercoms

mastercoms Dec 12, 2017
Member

Remove whitespace.

public void rightClickAir(GlowPlayer player, ItemStack holding) {
throwEnderPearl(player);
}

This comment has been minimized.

@mastercoms

mastercoms Dec 12, 2017
Member

Remove whitespace.

public Context getContext() {
return Context.ANY;
}

This comment has been minimized.

@mastercoms

mastercoms Dec 12, 2017
Member

Remove whitespace.

@@ -0,0 +1,29 @@
package net.glowstone.block.itemtype;
import org.bukkit.Location;

This comment has been minimized.

@mastercoms

mastercoms Dec 12, 2017
Member

Add a blank line between package and import.


public class GlowEnderPearl extends GlowEntity implements EnderPearl{
private ProjectileSource shooter;

This comment has been minimized.

@mastercoms

mastercoms Dec 12, 2017
Member

Remove whitespace.

setRawLocation(velLoc);

//If the EnderPearl collides with anything except air/fluids
if(!isTouchingMaterial(Material.AIR) &&

This comment has been minimized.

@mastercoms

mastercoms Dec 12, 2017
Member

Since the ender pearl is only a block in size, I believe you can just check

location.getBlock().isLiquid() || location.getBlock().isEmpty()
x = location.getX();

//Add 1.5m to account for eye level
y = location.getY() + 1.75;

This comment has been minimized.

@mastercoms

mastercoms Dec 12, 2017
Member

Do we need to spoof the location in the packet or can we just create the ender pearl at the correct location server side (by adding 1.75 to throwLoc)?

z = location.getZ();
yaw = location.getYaw();
pitch = location.getPitch();
Vector vel = location.getDirection().multiply(2);

This comment has been minimized.

@mastercoms

mastercoms Dec 12, 2017
Member

I think this would be better set in ItemEnderPearl#throwEnderPearl.

This comment has been minimized.

@Mimerme

Mimerme Dec 13, 2017
Author

What makes you say that? I think that it should stay mainly because it would make the constructor for GlowEnderPearl verbose.

//Add 1.5m to account for eye level
y = location.getY() + 1.75;
z = location.getZ();
yaw = location.getYaw();

This comment has been minimized.

@mastercoms

mastercoms Dec 12, 2017
Member

Use Position#getIntYaw.

y = location.getY() + 1.75;
z = location.getZ();
yaw = location.getYaw();
pitch = location.getPitch();

This comment has been minimized.

@mastercoms

mastercoms Dec 12, 2017
Member

Use Position#getIntPitch.

!isTouchingMaterial(Material.STATIONARY_WATER) &&
!isTouchingMaterial(Material.STATIONARY_LAVA)) {
System.out.println(world.getBlockTypeIdAt(location));
((GlowPlayer) shooter).teleport(location);

This comment has been minimized.

@kamcio96

kamcio96 Dec 12, 2017
Member

Debug + are you sure it's Player?

public void setShooter(ProjectileSource source) {
// TODO Auto-generated method stub
this.shooter = source;
((GlowPlayer) source).playSound(location, Sound.ENTITY_ENDERPEARL_THROW, 3, 1);

This comment has been minimized.

@kamcio96

kamcio96 Dec 12, 2017
Member

Why it's here? Could you move it to ItemEnderPearl?

…unused imports, fixed some verbosity, and made syntax corections.
Copy link
Member

@mastercoms mastercoms left a comment

Nice work, just a few more fixes :)


private void throwEnderPearl(GlowPlayer player) {
Location throwLoc = player.getLocation().clone();
throwLoc.setY(throwLoc.getY() + 1.5);

This comment has been minimized.

@mastercoms

mastercoms Dec 13, 2017
Member

Set the velocity here, instead of in the spawn message.

new GlowEnderPearl(throwLoc, 2).setShooter(player);
player.playSound(player.getLocation(), Sound.ENTITY_ENDERPEARL_THROW, 3, 1);
}
}

This comment has been minimized.

@mastercoms

mastercoms Dec 13, 2017
Member

Add a new line at the end of the file.

@Override
public void setBounce(boolean arg0) {
// TODO Auto-generated method stub

This comment has been minimized.

@mastercoms

mastercoms Dec 13, 2017
Member

Remove the extra line here.


setRawLocation(velLoc);

//If the EnderPearl collides with anything except air/fluids

This comment has been minimized.

@mastercoms

mastercoms Dec 13, 2017
Member

Write the comment like this:

// If the EnderPearl collides with anything except air/fluids
public static final int ITEM_FRAME = 71;
public static final int FIREWORK = 76;
public static final int LEASH_HITCH = 77;

This comment has been minimized.

@mastercoms

mastercoms Dec 13, 2017
Member

Remove whitespace.

holding.setType(Material.BUCKET);
}
}
}

//Empties the user's inventory when the item is used up

This comment has been minimized.

@mastercoms

mastercoms Dec 13, 2017
Member

Write the comment like this:

// Empties the user's inventory when the item is used up
@Pr0methean
Copy link
Contributor

@Pr0methean Pr0methean commented Dec 20, 2017

Beware of possible conflicts with #614

}

private void throwEnderPearl(GlowPlayer player) {
Location throwLoc = player.getLocation().clone();

This comment has been minimized.

@Pr0methean

Pr0methean Dec 21, 2017
Contributor

clone() is unnecessary here; GlowEntity.getLocation() already calls it.

mastercoms added a commit that referenced this pull request Dec 28, 2017
* Also fixes buckets emptying in creative mode
@mastercoms
Copy link
Member

@mastercoms mastercoms commented Dec 28, 2017

Merged in dc58d03 (with fixes).

@mastercoms mastercoms closed this Dec 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants