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 off hand support for Armor stands #520

Merged
merged 10 commits into from
Jul 8, 2017

Conversation

Postremus
Copy link
Contributor

@Postremus Postremus commented Jul 6, 2017

Add equipment sounds for armorstands. They are played to the player who equipped the armor stand. Currently no sound is played because of #519.

Implement EntityEquipment. This allows the armorstand to use the equipmentmonitor.

Save the off hand item of the livingentity. Use the tags HandItems, ArmorItems, HandDropChances, ArmorDropChances for this. Keep the old Equipment and DropChances tags for backwards compatibility.

Add off hand support for armor stands.

closes #471

@Postremus Postremus changed the title #471 Add off hand support for Armor stands Jul 6, 2017
@aramperes aramperes self-requested a review July 6, 2017 19:07
@@ -1,13 +1,20 @@
package net.glowstone.entity.objects;


Copy link
Member

Choose a reason for hiding this comment

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

Whitespace! ;)

// Deprecated since 15w31a, left here for compatibilty for now
if (compound.isList("Equipment", TagType.COMPOUND)) {
List<CompoundTag> list = compound.getCompoundList("Equipment");
if (list.size() == 5) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to allow for a variable size of this list? For example, having a list with only 1 element would only set the "item in hand" slot, and so forth.

Copy link
Contributor Author

@Postremus Postremus Jul 7, 2017

Choose a reason for hiding this comment

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

I can't see a Situation where:

  • A world is saved with an incomplete list of "Equipment", "HandItems" or "ArmorItems"
  • and the client still working

Just tried this situation out. Client is just black, and I can't move.

Since it is not safe to have incomplete lists in vanilla, I don't see how useful handling loading of them is going to be for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, more testing revealed, that paper has no problem with incomplete *Items lists.

Copy link
Member

Choose a reason for hiding this comment

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

The only situation I could see this happening is when using data-tags in commands (/summon, for example): it would be possible for a command to not have all the items in the list, and the Entity Store would have to handle that.

don't call equipmentmonitor.reset() twice
we already do that properly in the livingentity

Also don't save the *Items and *Chances Tag for a player, since he can never have these tags
order of the lists musts still be correct, e.g. feet must be before leg

If no Item or dropChance is available, load an empty Itemstack or 1f respectively
@aramperes
Copy link
Member

@Postremus I don't have time to test this on my own system right now. Can you confirm that your changes work without major breakage? If not, I'll have a look myself tonight or during the weekend.

@Postremus
Copy link
Contributor Author

Postremus commented Jul 8, 2017

It seems to work without breaking anything.

Executing:
/summon armor_stand ~ ~ ~ {ShowArms:1,Invisible:1,HandItems:[{Count:1b,id:276s},{Count:1b,id:442s}], ArmorItems:[{Count:1b,id:313s}, {Count:1b,id:312s}, {Count:1b,id:311s}]}
Spawns the armorstand with 3 pieces of armor (foot, leg, chest), and 2 handitems, correctly.

No other entity uses an EntityEquipment right now in Glowstone.

@aramperes
Copy link
Member

@Postremus Alright, thank you for confirming this works :)

Perhaps adding EntityEquipment support for other entities can be done in a future PR. The scope of your PR is mostly about Armor Stands anyways.

@aramperes aramperes merged commit ccd47f2 into GlowstoneMC:master Jul 8, 2017
@aramperes
Copy link
Member

Thank you again for your contribution!

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

Successfully merging this pull request may close these issues.

2 participants