Doubts about creating Sword and Armor

[Niinnnnnn]

Do I need to use ItemSword to create a sword or create an ItemArmor armor and use ToolMaterial and ArmorMaterial?
This was made for ease, right? it is not mandatory to use

[Cadiboo]

What do you mean? It’s certainly easier & allows vanilla to better handle your item, and allows you to override vanilla’s items but no it’s not exactly necessary - you are able to replicate the vanilla behaviour without replicating it, but for example the tooltips that vanilla puts on swords won’t appear and you’ll have to implement them yourself. For code maintainability you should probably extend vanilla because

a) if vanilla changes something you will know about and can change it

b) knowing something _is_ (extends) something else is better than knowing something _should be_ (replicates the functionality of) something else 

[Animefan8888]

  On 9/27/2018 at 6:18 AM, Niinnnnnn said:

This was made for ease, right? it is not mandatory to use

Expand  

It was made for compatibility, if mod one wants all swords to be used in there machine, then it will check if an Item is an instance of ItemSword.

[jabelar]

Minecraft code is pretty inconsistent, and sometimes uses direct instance checking and sometimes uses instanceof. However it DOES use instanceof sometimes and you'll probably want to take advantage of it. For example, the getAttackDamage() method only gets called from EntityLiving if the item is an instanceof ItemSword.

 

As an example of inconsistency (or not the best programming practice). the Item#isShield() method is implemented as:

    public boolean isShield(ItemStack stack, @Nullable EntityLivingBase entity)
    {
        return stack.getItem() == Items.SHIELD;
    }

That is frankly a "dumb" way to do it. Better programming (from type hierarchy point of view) would be simply return false and let ItemShield return true. Alternatively, it should check for instanceof ItemShield. However, this implementation means that for your custom shield you need to override this method directly. It would be better programming practice to be able to extend ItemShield and automatically it would know that isShield() is true...

 

So you'll need to do a mix of extending the class, overriding things that shouldn't be necessary, and working around places where the instance is hard-coded in vanilla.

 

There are times when you don't want to extend though -- if you don't want your item to be always treated as a sword. I have a tutorial about deciding whether to extend or copy a vanilla class: https://jabelarminecraft.blogspot.com/p/minecraft-forge-172-know-when-to-copy.html?showComment=1538061795434

[V0idWa1k3r]

  On 9/27/2018 at 3:25 PM, jabelar said:

As an example of inconsistency (or not the best programming practice). the Item#isShield() method is implemented as:

Expand  

The reason it's implemented like is is because it's a forge patch. Vanilla simply did item == Items.SHIELD when checking for a shield in various places. Forge removed those direct checks and instead did this patch to the item class. It's functionally the same, preserves vanilla's functionality and allows anyone to override this when they want to make a custom shield themselves.

Or at least I assume it's a forge patch because it's together with the rest of them and has a javadoc.

[jabelar]

  On 9/27/2018 at 3:45 PM, V0idWa1k3r said:

The reason it's implemented like is is because it's a forge patch. Vanilla simply did item == Items.SHIELD when checking for a shield in various places. Forge removed those direct checks and instead did this patch to the item class. It's functionally the same, preserves vanilla's functionality and allows anyone to override this when they want to make a custom shield themselves.

Or at least I assume it's a forge patch because it's together with the rest of them and has a javadoc.

Expand  

 

Good point -- so it is bad Forge practice! Still doesn't change the criticism. They should have done like they did and replace all the checks with the isShield() call but the default implementation of isShield() in the Item class should have just returned false and should have been overridden to return true in ItemShield.

[V0idWa1k3r]

  On 9/27/2018 at 3:49 PM, jabelar said:

Good point -- so it is bad Forge practice! 

Expand  

I think it is done this way to make as little patches as possible and have the patches be as small as possible. 1 patch to the Item class < 1 patch to the Item class and 1 patch to the ItemShield class.

[jabelar]

  On 9/27/2018 at 3:52 PM, V0idWa1k3r said:

I think it is done this way to make as little patches as possible and have the patches be as small as possible. 1 patch to the Item class < 1 patch to the Item class and 1 patch to the ItemShield class.

Expand  

Yes, I understand that goal but the point of Forge intervening is to make it amenable to modders.  And in any case I offered a way to do it without patch to ItemShield -- they could test instanceof ItemShield instead of = Items.SHIELD. 

 

The way it is currently, a person might extend ItemShield and then wonder why it isn't working. The onus is on the modder to understand there is a hard-coded method in the parent class that needs to be overwritten. It would be nice to simply extend ItemShield and have it immediately work as a shield.