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

Modded Hybrid Bukkit/Forge Servers - Enum vs Static Classes #239

Open
CryptoMorin opened this issue Jan 3, 2024 · 0 comments
Open

Modded Hybrid Bukkit/Forge Servers - Enum vs Static Classes #239

CryptoMorin opened this issue Jan 3, 2024 · 0 comments
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request opinions Extra attention is needed performance Performance improvements special case Rare cases that need to be handled in the code manually by hardcoding a part of the system

Comments

@CryptoMorin
Copy link
Owner

CryptoMorin commented Jan 3, 2024

The Problem

So as most of you know, there are server software out there that basically combine Bukkit and Forge together to make both plugins and mods function properly together. These include Magma, Mohist, Ketting, etc.

They work kind of well for the most part, but of course they fail in some rares cases when plugins use advanced features. One thing that they do is that they inject enums into the org.bukkit.Material class in order to register materials used by other mods.
This fails if someone uses XMaterial, since this class is mostly shadowed for each plugin and not a part of the official Bukkit source code. So when a new material is encountered, XMaterial cannot match it and throws an error (which can be just ignored and return null, but that wouldn't work practically.)

Important

As of early v1.19, this is not mainly an issue with modded servers anymore, as Minecraft is beginning to introduce registries (Spigot, Forge, Fabric) where most enums, materials, sounds, banner types, mob variants and basically everything is being re-coded in a form of static fields. These registries might allow plugins or datapacks to add custom elements in future releases.

Related Issues:

Related Enum Bytecode Manipulation Classes:

Throughout this entire post, I'll be focusing on XMaterial specifically, but this issue is not limited to this class and can include XPotion, XEnchantment, XSound and XBiome. XSound has recently gained the ability to play namespaced sounds, but the enum injection issue will still remain.

The Solution

Now the only way to fix this issue to make it actually support forge servers, is to turn the enum classes into static members like so:

public enum XMaterial {
  GRASS(1, "GRASS_BLOCK"), PAPER, ...
}

// ->

public class XMaterial {
  public static final XMaterial GRASS = new XMaterial(1, "GRASS_BLOCK");
  public static final XMaterial PAPER = new XMaterial();

  public String name() { /* Some cached reflection stuff */ }
  public static XMaterial[] values() { /* Some cached reflection stuff */ }

  public XMaterial matchXMaterial(Material mat) {
    if (unrecognized material in cache) {
      addToCache(new XMaterial(mat));
    }
  }
}

You might ask is it not possible to just a single enum and change its properties?

public enum XMaterial {
  GRASS, PAPER, CUSTOM;
}

public static XMaterial matchXMaterial(Material material) {
  CUSTOM.setName("Blah");
  CUSTOM.setMaterial(material);
  return CUSTOM;
}

No, this is not thread-safe. If multiple threads use this same method, it will fail. But most of the Bukkit's API is already thread-unsafe, so why not? We could probably synchronize it, but it's still not going to be viable when using the object for prolonged periods.

public synchronized static XMaterial matchXMaterial(Material material) {}

// Or this which is more reliable:
public static XMaterial matchXMaterial(Material material) {
  synchronized (CUSTOM) {}
}

Now, back to our previous solution. There are advantages and disadvantages to the static member approach.

Disadvantages

Conversion

Converting these enum classes might sound extremely hard to rewrite all these data, but a simple Java script code can generate the code for us since all the data is there.

Plugins that have Proguard configured to not remove enums (which you should definitely do for XSeries if you're using them for configs/databases too) might have to readjust their settings to not shrink these classes (if they obfuscate with shaded libs).

Java Bytecode

Doing this would mean that the underlying JVM bytecode is going to be changed. The solution is to simply recompile the plugin with the new XSeries version and redistribute it, but for people who somehow remotely implement XSeries, either by a utility plugin or downloading them, they'll encounter weird issues like the NoSuchFieldException. So they'd have to recompile too.

Source Code Format

As you can see, this is very messy and bloated compared to enums.

public class XMaterial {
  public static final XMaterial 
      GRASS = new XMaterial(1, "GRASS_BLOCK"),
      PAPER = new XMaterial()
      ;
}

This is nice and all, but not as concise as enums. Surprisingly, using Kotlin won't make this shorter. In fact, it will be longer!

class XMaterial {
  companion object {
    @JvmField val GRASS = XMaterial(1, "GRASS_BLOCK")
    @JvmField val PAPER = XMaterial()
  }
}

Scala and Java interoperability is just out of the way.

class XMaterial(val name: String, val data: Byte, ...):
  def name(): String =
    /* Some cached reflection stuff */
end XMaterial

object XMaterial {
    val GRASS = XMaterial(1, "GRASS_BLOCK")
    val PAPER = XMaterial()
}

from Java:

XMaterial mat = XMaterial$.MODULE$.GRASS();

Also, let's not forget that this will probably need Kotlin/Scala Runtime libs and that's a big no for most projects.

Switch Statements

// With enums you can do:
switch (material) {
  case GRASS:
    return something;
  case PAPER:
    return somethingElse;
}

// With class members switch statements are useless,
// unless you use Java 17 pattern matching which is unlikely:
import static XMaterial;
return switch (material) {
  case GRASS -> something;
  case PAPER -> somethingElse;
}

Annotations

You'll no longer be able to use the standard enum values inside annotations. You might think that putting materials or sounds inside of annotations might sound weird, but it's actually useful in some cases.

public enum SpecialItems {
  @Materials({XMaterial.PAPER, XMaterial.BOOK})
  TICKET;
}

Object Comparison & Hash

  • The == operator will continue to work correctly unless someone initializes another instance.
  • ordinal() cannot be used anymore.
  • With enums you can use wonderful optimized classes like EnumMap & EnumSet

One way to solve the singleton issue is to simply make equals() and hashcode() methods depend on the underlying Material object, but this will not work for unsupported materials in older versions correctly.

Advantages

Removal of valueOf()

This might sound like a disadvantage, but it's actually one of the common sources of errors for XSeries newcomers. This method should not be used in any circumstances.

Memory Space

Enums often require more than twice as much memory as static constants and will consume 10x more file size compared to static constants. (StackOverFlow)
Considering that we will be caching our own name() this won't be entirely true.

Abstraction

If we, at some point in time wanted to extend another class, we're free to do so.

Modded Support

For people who look to make their plugin compatible as mods and not just inside hybrid servers, this can be a huge step and will gain more attraction when your plugin/mod supports more platforms.

Forward Compatibility

Right now, if you for example, try to use a newer material name inside a config and XSeries isn't updated for that new Minecraft version yet, and try to use XMaterial.matchXMaterial(String) you will get back null because the method must return an XMaterial and no enum entries are defined for the new material yet. Even if you use a direct XMaterial.matchXMaterial(Material) you'll get a Unsupported material error.

With this solution, XMaterial can fix these issues on the fly, even though you'll not be able to use a predefined XMaterial field for the newer material until XSeries is updated for that version.
This would also help abandoned plugins that use XSeries, allowing them to continue functioning if no major changes are introduced.

Interfaces

There's a 3rd solution, and that's using interfaces that supports both enums and abstract classes which solves most of the problems listed here. However it's not confirmed if this is a firm solution yet.

One issue is that if we decided to have a custom hashcode/equals strategy, it wouldn't be possible for enums as those methods are final for them. Enums themselves implement a hashcode based on memory position which is pretty good.

interface Material {
    String name();

    boolean isSupported();

    default Material or(Material other) {
        return this.isSupported() ? this : other;
    }

    boolean equals(Object other);

    int hashCode();
}

Conclusion

It would seem that the disadvantages outweigh the advantages, but the biggest advantage is obviously that we can support modded servers that way and forward compatibility. Please let me know if I missed something or I provided wrong information.

Personally, I was completely against this at first, but now I'm unsure.

Edit: It appears that there's a 3rd solution, and that's using interfaces read above for more info.

If you want this whole change to happen, react with 👍 if you agree and 👎 if you disagree or react with 😕 if you're unsure. Feel free to comment additional information if you like. I'd appreciate it if you only vote once you've actually read the entire post.

@CryptoMorin CryptoMorin added bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request opinions Extra attention is needed special case Rare cases that need to be handled in the code manually by hardcoding a part of the system performance Performance improvements labels Jan 3, 2024
@CryptoMorin CryptoMorin self-assigned this Jan 3, 2024
@CryptoMorin CryptoMorin pinned this issue Jan 3, 2024
@CryptoMorin CryptoMorin unpinned this issue May 11, 2024
CryptoMorin added a commit that referenced this issue Nov 18, 2024
# Partially Updated for v1.21.3
* Fixes #306
* Fixes #310

> [!IMPORTANT]
> XSeries will no longer follow independent class principle due to a lot of changes caused by previous versions.
> This version implements the abstraction concept from #239 as an experiment. This change was forced because
> Minecraft now uses registries for most enum-like.

* Added XAttribute (For #307)
* ParticleDisplay clone() method now properly copies the "only visible to" setting. (Fixes #309)
CryptoMorin added a commit that referenced this issue Nov 18, 2024
# Partially Updated for v1.21.3
* Fixes #306
* Fixes #310

> [!IMPORTANT]
> XSeries will no longer follow independent class principle due to a lot of changes caused by previous versions.
> This version implements the abstraction concept from #239 as an experiment. This change was forced because
> Minecraft now uses registries for most enum-like.

* Added XAttribute (For #307)
* ParticleDisplay clone() method now properly copies the "only visible to" setting. (Fixes #309)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request opinions Extra attention is needed performance Performance improvements special case Rare cases that need to be handled in the code manually by hardcoding a part of the system
Projects
None yet
Development

No branches or pull requests

1 participant