Optimization for PLReflect.java and a few other style conventions and fixes.

[cire3wastaken]

I was scrolling through the source when I saw how you guys implemented the getClassById and getClassByName functions, which I think could be optimized better. Right now, you guys loop over the entire array of classes, and don't cache the result. I think there are 2 main ways to fix this. 1) Cache the result or 2) Instead of using an array, use a HashMap for amortized constant time retrieval, which is better than the current linear time. You could also optimize this further by creating the HashMap with the correct amount of elements, like you do with the array. Lax's idea was, in my opinion, unfairly rejected (this is pretty much the same optimization). You can avoid the brought up issue of the same function/object name in two ways: 1) If it's a function, you can simply refactor the code to make functions get grabbed from the respective Class<?> or 2) synthesize member classes and anonymous classes (the java compiler does this under the hood, a member class of class X called class Y becomes X$Y.class)

I was also looking at the code, and there are a few style issues I feel like that should be fixed to match with Sun/Oracle coding standards. For one, I think you should 1) actually get the eaglerforge.net domain or 2) refactor the package to somewhere else, possibly package io.github.eaglerforge or something. There's also a few classes (such as this) that need to be refactored to fit with java style conventions.

Another issue I noticed is the usage of TeaVM in the main source set. You should keep TeaVM related code in the TeaVM source set, and have a shadow class in the lwjgl source set, so desktop runtime won't be broken, and it's also a good practice. The main source set for eagler is pretty much a common/main source set.

Another major issue I noticed is how you interact with the Minecraft source, and the lack of documentation on the changes. Documentation isn't required, however adding a // EaglerForge start and a // EaglerForge end comment to wherever you modified the source code is required for long term scalability. Another thing, is how you guys are not very compliant with a Minimal Diff policy in modifying the source, which weakens the long term maintainability of this project. Even better, if you have code that needs to called on an event, listen to the event in a separate class in the eaglerforge package for maintainability!! Try your best to not add code within the minecraft source, unless it is absolutely needed!!

(the worst offender of all of the style conventions and code conventions is probably this)
This class could probably be refactored and fixed, and there's no need to make a static field in the Minecraft class, you can get all the data you need via Minecraft.getMinecraft() and its functions.

This following part is copied from craft bukkit: https://hub.spigotmc.org/stash/projects/spigot/repos/craftbukkit/browse (thank you craftbukkit for already having this amazing explanation)

Minimal Diff Policy

The Minimal Diff Policy is key to any changes made within Minecraft classes. When people think of the phrase "minimal diffs", they often take it to the extreme - they go completely out of their way to abstract the changes they are trying to make away from editing Minecraft's classes as much as possible. However, this is not what is meant by "minimal diffs". Instead, when trying to understand this policy, it helps to keep in mind its goal: to reduce the impact of changes we make to Minecraft's internals have on our update process.

To put it simply, the Minimal Diffs Policy simply means to make the smallest change in a Minecraft class possible without duplicating logic.

Here are a few tips you should keep in mind, or common areas you should focus on:

Try to avoid duplicating logic or code when making changes.
Try to keep your changes easily discernible - don't nest or group several unrelated changes together.
All changes must be surrounded by EaglerForge comments.
If you only use an import once within a class, don't import it and use a fully qualified name instead.
Try to employ "short-circuiting" of logic if at all possible. This means you should force a conditional to be the value needed to side step the code block to achieve your desired effect.
For example, to short circuit this:

if (!this.world.isClientSide && !this.isDead && (d0*d0) + d1 + (d2*d2) > 0.0D) {
    this.die();
    this.h();
}

You would do this:

if (false && !this.world.isClientSide && !this.isDead && (d0*d0) + d1 + (d2*d2) > 0.0D) {
    this.die();
    this.h();
}

For checks related to API where an exception needs to be thrown in a specific case we recommend use the package Preconditions
For checking arguments we recommend using Preconditions#checkArgument where a failing check should throw a IllegalArgumentException
We recommend using this to ensure the behaviour of @NotNull in the EaglerForge API
For checking arguments we recommend using Preconditions#checkState where a failing check should throw a IllegalStateException
For example, you should use:

private Object messenger;

public void sendMessage(Sender sender, String message) {
    Preconditions.checkArgument(sender != null, "sender cannot be null");
    Preconditions.checkState(this.messenger != null, "The messenger instance cannot be used")
}

Instead of:

private Object messenger;

public void sendMessage(Sender sender, String message) {
    if (sender == null) {
        throw new IllegalArgumentException("Sender cannot be null");
    }

    if (this.messenger == null) {
        throw new IllegalStateException("The messenger instance cannot be used");
    }
}

When the change you are trying to make involves removing code, or delegating it somewhere else, instead of removing it, you should comment it out.
For example:

// EaglerForge start - special case dropping so we can get info from the tile entity
public void dropNaturally(World world, int i, int j, int k, int l, float f, int i1) {
    if (world.random.nextFloat() < f) {
        ItemStack itemstack = new ItemStack(Item.SKULL, 1, this.getDropData(world, i, j, k));
        TileEntitySkull tileentityskull = (TileEntitySkull) world.getTileEntity(i, j, k);

        if (tileentityskull.getSkullType() == 3 && tileentityskull.getExtraType() != null && tileentityskull.getExtraType().length() > 0) {
            itemstack.setTag(new NBTTagCompound());
            itemstack.getTag().setString("SkullOwner", tileentityskull.getExtraType());
        }

        this.b(world, i, j, k, itemstack);
    }
}
// EaglerForge end

public void remove(World world, int i, int j, int k, int l, int i1) {
    if (!world.isStatic) {
        /* EaglerForge start - drop item in code above, not here
        if ((i1 & 8) == 0) {
            ItemStack itemstack = new ItemStack(Item.SKULL, 1, this.getDropData(world, i, j, k));
            TileEntitySkull tileentityskull = (TileEntitySkull) world.getTileEntity(i, j, k);

            if (tileentityskull.getSkullType() == 3 && tileentityskull.getExtraType() != null && tileentityskull.getExtraType().length() > 0) {
                itemstack.setTag(new NBTTagCompound());
                itemstack.getTag().setString("SkullOwner", tileentityskull.getExtraType());
            }

            this.b(world, i, j, k, itemstack);
        }
        // EaglerForge end */

        super.remove(world, i, j, k, l, i1);
    }
}

EaglerForge Comments

Changes to a Minecraft class should be clearly marked using EaglerForge comments.

All EaglerForge comments should be capitalised appropriately. (i.e. EaglerForge, not EF/eaglerForge, etc.)
If the change only affects one line of code, use an end of line EaglerForge comment
Examples:

If the change is obvious, then you need a simple end of line comment.

if (true || minecraftserver.getAllowNether()) { // EaglerForge

// EaglerForge
if (!isEffect && !world.isStatic && world.difficulty >= 2 && world.areChunksLoaded(MathHelper.floor(d0), MathHelper.floor(d1), MathHelper.floor(d2), 10)) {

If the change affects more than one line, you should use a multi-line EaglerForge comment.
Example:

The majority of the time, multi-line changes should be accompanied by a reason since they're usually much more complicated than a single line change. If the change is something important to note or difficult to discern, you should include a reason at the end of line EaglerForge comment.

// EaglerForge start - special case dropping so we can get info from the tile entity
public void dropNaturally(World world, int i, int j, int k, int l, float f, int i1) {
    if (world.random.nextFloat() < f) {
        ItemStack itemstack = new ItemStack(Item.SKULL, 1, this.getDropData(world, i, j, k));
        TileEntitySkull tileentityskull = (TileEntitySkull) world.getTileEntity(i, j, k);

        if (tileentityskull.getSkullType() == 3 && tileentityskull.getExtraType() != null && tileentityskull.getExtraType().length() > 0) {
            itemstack.setTag(new NBTTagCompound());
            itemstack.getTag().setString("SkullOwner", tileentityskull.getExtraType());
        }

        this.b(world, i, j, k, itemstack);
    }
}
// EaglerForge end

Otherwise, if the change is obvious, such as firing an event, then you can simply use a multi-line comment.

    // EaglerForge start
    BlockIgniteEvent event = new BlockIgniteEvent(this.cworld.getBlockAt(i, j, k), BlockIgniteEvent.IgniteCause.LIGHTNING, null);
    world.getServer().getPluginManager().callEvent(event);

    if (!event.isCancelled()) {
        world.setTypeIdUpdate(i, j, k, Block.FIRE);
    }
    // EaglerForge end

All EaglerForge comments should be on the same indentation level the code block it is in.
Imports in Minecraft Classes

Do not remove unused imports if they are not marked by EaglerForge comments.

[cire3wastaken]

@OtterCodes101 opinions?

[ZXMushroom63]

I was scrolling through the source when I saw how you guys implemented the getClassById and getClassByName functions, which I think could be optimized better. Right now, you guys loop over the entire array of classes, and don't cache the result. I think there are 2 main ways to fix this. 1) Cache the result or 2) Instead of using an array, use a HashMap for amortized constant time retrieval, which is better than the current linear time. You could also optimize this further by creating the HashMap with the correct amount of elements, like you do with the array. Lax's idea was, in my opinion, unfairly rejected (this is pretty much the same optimization). You can avoid the brought up issue of the same function/object name in two ways: 1) If it's a function, you can simply refactor the code to make functions get grabbed from the respective Class<?> or 2) synthesize member classes and anonymous classes (the java compiler does this under the hood, a member class of class X called class Y becomes X$Y.class)

There isn't a reason to optimize getClassById or getClassByName, this is intended to be cached on the mod-side. Example

Currently, member classes are ignored, and by multiple functions having the same name, I'm referring due this situation:

public class ExampleClass {
     public static calculateSum(int a, int b) {
          return a + b;
     }
     public static calculateSum(float a, float b) {
          return a + b;
     }
     public static calculateSum(double a, double b) {
          return a + b;
     }
}

Additionally, which optimization did lax suggest? I'm having trouble finding it.

I was also looking at the code, and there are a few style issues I feel like that should be fixed to match with Sun/Oracle coding standards. For one, I think you should 1) actually get the eaglerforge.net domain or 2) refactor the package to somewhere else, possibly package io.github.eaglerforge or something. There's also a few classes (such as this) that need to be refactored to fit with java style conventions.

Fair point, but it is worth noting that those coding guidelines have not been updated since 1999 and are kept purely for archival purposes. While this doesn't necessarily disqualify them from relevance, changing the package name is a mostly pointless infra-change. Refactoring server.java is an entirely different issue, but something like ServerAPI.java would be more suitable.

Another issue I noticed is the usage of TeaVM in the main source set. You should keep TeaVM related code in the TeaVM source set, and have a shadow class in the lwjgl source set, so desktop runtime won't be broken, and it's also a good practice. The main source set for eagler is pretty much a common/main source set.

I'm not familiar with the Eaglercraft desktop runtime, so I can't comment on this.

Another major issue I noticed is how you interact with the Minecraft source, and the lack of documentation on the changes. Documentation isn't required, however adding a // EaglerForge start and a // EaglerForge end comment to wherever you modified the source code is required for long term scalability. Another thing, is how you guys are not very compliant with a Minimal Diff policy in modifying the source, which weakens the long term maintainability of this project. Even better, if you have code that needs to called on an event, listen to the event in a separate class in the eaglerforge package for maintainability!! Try your best to not add code within the minecraft source, unless it is absolutely needed!!

I agree fully on this point, it is wrong to pollute the source code outside of the the net.eaglerforge.api package, and it is actively causing problems when trying to update to newer upstream changes.

(the worst offender of all of the style conventions and code conventions is probably this) This class could probably be refactored and fixed, and there's no need to make a static field in the Minecraft class, you can get all the data you need via Minecraft.getMinecraft() and its functions.

[radmanplays]

@ZXMushroom63 Eaglercraft desktop runtime is just eaglercraft running in java runtime instead of javascript runtime. the "teavm" folder is used by the javascript runtime and the "lwjgl" folder is used by the java runtime and the "main" folder is used by both so @cire3wastaken is suggesting that we should move the eaglerforge folder from the main folder to teavm folder since there are teavm libraries are used in eaglerforge classes and teavm libraries dont exist in java runtime

[cire3wastaken]

Currently, member classes are ignored, and by multiple functions having the same name, I'm referring due this situation:

public class ExampleClass {
     public static int calculateSum(int a, int b) {
          return a + b;
     }
     public static float calculateSum(float a, float b) {
          return a + b;
     }
     public static double calculateSum(double a, double b) {
          return a + b;
     }
}

You can fix this problem by using the bytecode descriptors, and possibly add another function getMethodByDescriptor. For example, the first method would be Lnet/whateverpackage/ExampleClass#calculateSum(II)I, etc.

Fabric does this with mixins, you have to specify the full bytecode descriptor to be able to mixin

[cire3wastaken]

There isn't a reason to optimize getClassById or getClassByName, this is intended to be cached on the mod-side. Example

The only issue I see with forcing the mod to cache the result is in a worse case scenario, when the mod needs 100s or maybe more classes, or maybe more realistically, hundreds of mods with 10 to 100 classes each, but the classes they want are all located at the end of array. Linear time quickly adds up, and this will block the main thread. I using a HashMap is still better for amortized constant time. The constant time is really useful, and you don't need to check if the value is in the Map, since it'll return null if it isn't.

[ZXMushroom63]

There isn't a reason to optimize getClassById or getClassByName, this is intended to be cached on the mod-side. Example

The only issue I see with forcing the mod to cache the result is in a worse case scenario, when the mod needs 100s or maybe more classes, or maybe more realistically, hundreds of mods with 10 to 100 classes each, but the classes they want are all located at the end of array. Linear time quickly adds up, and this will block the main thread. I using a HashMap is still better for amortized constant time. The constant time is really useful, and you don't need to check if the value is in the Map, since it'll return null if it isn't.

Quite a bit late, and the architecture for EaglerForge is almost entirely different now, but the way TeaVM converts HashMaps to JavaScript is by storing two arrays, one for keys, the other for values. They both used linear time.

[radmanplays]

blud wrote a whole essay complaining about the code 💀

[radmanplays]

btw will you add eaglerforge api to resent if we fix our code?

[OtterCodes101]

it'll prob make resent even bigger so i'm guessing no