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

ItemAxe subclasses are impossible. #2646

Closed
williewillus opened this issue Mar 24, 2016 · 10 comments
Closed

ItemAxe subclasses are impossible. #2646

williewillus opened this issue Mar 24, 2016 · 10 comments

Comments

@williewillus
Copy link
Contributor

In the ItemAxe constructor, the attack damage+speed are looked up in a hardcoded array indexed by the input material ordinal. Which means any EnumHelper added materials will immediately crash the game with AIOOBE since their indices are larger than the array -.-

Easy fix would be to just set them to 0 if the range is greater than the ordinal for diamond (or whichever vanilla one is last) and let the mod set it in their ctor

@williewillus
Copy link
Contributor Author

the proper fix as told to me is to extend ItemTool directly and set the tool class to axe

@hea3ven
Copy link
Contributor

hea3ven commented Mar 27, 2016

The problem with this approach is that you can't set the toolClass field, meaning you have to implement both getHarvestLevel and getToolClasses. Maybe the toolClass field could be protected in stead of private.

@williewillus
Copy link
Contributor Author

Yes, there are further problems with being unable to do this, reopening.

  1. What's been mentioned above
  2. This is more critical. Not subclassing ItemAxe means that the unique mechanic they have on shields cannot be inherited (it uses instanceof ItemAxe) without copy pasting the vanilla code into 3 separate event handlers. We need to look at finding a solution for this

@williewillus williewillus reopened this Mar 31, 2016
@williewillus
Copy link
Contributor Author

I may PR a fix for this shortly

@diesieben07
Copy link
Contributor

  1. setHarvestLevel.
  2. That should be patched to use the tool classes instead then.

@williewillus
Copy link
Contributor Author

it is checked in over 4 separate places

@williewillus
Copy link
Contributor Author

I'd rather just patch this one ctor than in 4 other places, but I get patching the check as well
@LexManos your thoughts on this?

@diesieben07
Copy link
Contributor

3 places, one of the 4 is the ItemTool constructor, which doesn't count. And even if you patch the constructor here, you still have to patch the 3 other places, because forge's axe tool-class should behave like actual axes, otherwise it's pointless to have the tool class.

@williewillus
Copy link
Contributor Author

yea I'll probably patch the instanceof checks into tool classes then

@LexManos
Copy link
Member

I'm not sure why mojang special cased these particular values.
The acceptable way to fix this is to just add another constructor to ItemAxe that takes in these values.
Its a dirty dirty hack but whatever. How many locations explicitly check the instanceof? Looks like 4. It may actually be better to patch those into .isToolType(Types.AXE) {In theory we should move twards using a enum as it's a faster compare == vs .equals}

Ya.. I think that'd be best. Add a Enum with a getOrCreate {similar to what we have for Biome type in the BiomeDictionary} And switch all the places where we use String toolType to Enum.
As I said, this would increase performance. Which would make the isToolType(AXE) replacements of instanceof ItemAxe better.

So two options: New constructor to bypass the array herpa
Or switch to a new system and patch those 4 instanceof checks.

CruzBishop added a commit to Techern-archived/MinecraftForge that referenced this issue Apr 11, 2016
As discussed in MinecraftForge#2646, this is the easiest way to bypass the hardcoded arrays

I wasn't confident enough to go through and completely replace said arrays, so I'll provide this for now. One day I'll come back and bypass the arrays completely (unless someone else beats me to it)
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

No branches or pull requests

4 participants