Skip to content

Conversation

kashike
Copy link
Contributor

@kashike kashike commented Jul 13, 2017

Java 8 and Guava both have an Optional class - moving these annotations outside of the Optional class removes the need to import them (importing Optional.Method will conflict with java.lang.reflect.Method, as well) or use fully-qualified class names.

This pull request:

  • deprecates net.minecraftforge.fml.common.Optional, with no replacement
  • deprecates net.minecraftforge.fml.common.Optional$InterfaceList, replaced by net.minecraftforge.fml.common.OptionalInterface$OptionalInterfaceList (though repeating OptionalInterface should be preferred now)
  • deprecates net.minecraftforge.fml.common.Optional$Interface, replaced by net.minecraftforge.fml.common.OptionalInterface
  • deprecates net.minecraftforge.fml.common.Optional$Method, replaced bynet.minecraftforge.fml.common.OptionalMethod

Additionally, net.minecraftforge.fml.common.OptionalInterface is repeatable.

@mezz mezz added 1.12 Cleanup This request reorganizes messy code, or fixing it would require doing so. labels Jul 13, 2017
@LexManos
Copy link
Member

Actually no, this should all be deprecated and be removed in favor of the Capability system.
The only reason this exists at all is because modders designed bad APIs. It's a hack and we're not going to expand/endorse it more then we need to.

@Alpvax
Copy link

Alpvax commented Jul 13, 2017

@LexManos How do you implement a CapabilityProvider for an optional dependency?

@HenryLoenwind
Copy link

Alpvax, like this:

https://github.com/SleepyTrousers/EnderIO/blob/1.11-base/src/main/java/crazypants/enderio/integration/buildcraft/BuildcraftIntegration.java#L31-L37

https://github.com/SleepyTrousers/EnderIO/blob/1.11-base/src/main/java/crazypants/enderio/integration/buildcraft/BuildCraftFluidRegister.java

Or this:

https://github.com/SleepyTrousers/EnderIO/blob/1.11-base/src/main/java/crazypants/enderio/power/CapInjectHandler.java#L35-L42

General solution:

  1. you put all stuff that would crash into one class (as static methods or implementing an interface and assigning to a field elsewhere)
  2. you add a second class that tries to load the first one and remembers if that worked
  3. you call into the first class only if loading the class worked (e.g. using the second class as proxy)

Things to remember:

  1. @CapabilityInject will not load it's parameter, so it is safe to use
  2. Calling a static method will only load the class that method is in if it is actually executed
  3. You can catch the Exception from trying to load a missing class

@AlexIIL
Copy link
Contributor

AlexIIL commented Jul 13, 2017

@HenryLoenwind have you ever actually tested the bc compact code? I'm pretty sure it's broken, in part by using the wrong modid. I'm also not sure that it's necessary to reference the class by reflection, java seems to handle it fine if you just never call a method if it doesn't exist.

@Alpvax
Copy link

Alpvax commented Jul 13, 2017

I believe that as long as the file is there for compilation, it is not needed at runtime if it is never called (Hence all the ClassNotFoundExceptions at runtime when people try to use client-only classes on the server).

Adding an adapter which the check can be delegated to seems to be the way forwards, thanks.

@LexManos
Copy link
Member

https://github.com/SleepyTrousers/EnderIO/blob/1.11-base/src/main/java/crazypants/enderio/power/CapInjectHandler.java#L35-L42 is the recomended way, it shouldn't need the try around it because that function should only ever be called by the event. And if the event is fired the Cap class is there.

But having the try doesn't hurt anything.

That's WHY I made @CapInject work on methods.
Also, if you don't want to go that route, you can do something along the lines of:
if (CAP_HOLDER != null)
myHandler = getHandlerForCap()

THere are PLENTY of ways to do it without causing crashes, Capabilities were SPECIFICALLY designed to replace the need for @optional

@Alpvax
Copy link

Alpvax commented Jul 13, 2017

Yes, I completely forgot that CapabilityInject works on methods. Am I correct in thinking that you still need to use a wrapper around the capabilty handler, as if the class doesn't exist it will crash?

@stale
Copy link

stale bot commented Oct 4, 2017

This has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale This request hasn't had an update from the author in a long time. label Oct 4, 2017
@kashike kashike closed this Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.12 Cleanup This request reorganizes messy code, or fixing it would require doing so. Stale This request hasn't had an update from the author in a long time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants