Alvinn8 / paper-nms-maven-plugin

A maven plugin for using NMS on paper with Mojang mappings.
MIT License
115 stars 8 forks source link

`FluidState` gets remapped by mistake when running maven `build` #15

Closed LoneDev6 closed 1 year ago

LoneDev6 commented 1 year ago

Steps to reproduce

1

Compile this code:

import net.minecraft.core.BlockPos;
import net.minecraft.server.level.ServerLevel;
import net.minecraft.world.level.material.Fluid;
import net.minecraft.world.level.material.FluidState;
import org.bukkit.block.Block;
import org.bukkit.craftbukkit.v1_19_R3.CraftWorld;

public class Test1
{
    public void test(Block block)
    {
        ServerLevel level = ((CraftWorld) block.getWorld()).getHandle();
        BlockPos blockPos = new BlockPos(block.getX(), block.getY(), block.getZ());
        FluidState fluidState = level.getFluidState(blockPos);
        if (!fluidState.isEmpty())
        {
            Fluid fluidType = fluidState.getType();
            level.scheduleTick(blockPos, fluidType, fluidType.getTickDelay(level));
        }
    }
}

2

Run the maven command clean build. See the output remapped code is this working and correct code.

import net.minecraft.core.BlockPosition;
import net.minecraft.server.level.WorldServer;
import net.minecraft.world.level.IWorldReader;
import net.minecraft.world.level.material.Fluid;
import net.minecraft.world.level.material.FluidType;
import org.bukkit.block.Block;
import org.bukkit.craftbukkit.v1_19_R3.CraftWorld;

public class Test1 {
  public void test(Block block) {
    WorldServer level = ((CraftWorld)block.getWorld()).getHandle();
    BlockPosition blockPos = new BlockPosition(block.getX(), block.getY(), block.getZ());
    Fluid fluidState = level.b_(blockPos);
    if (!fluidState.c()) {
      FluidType fluidType = fluidState.a();
      level.a(blockPos, fluidType, fluidType.a((IWorldReader)level));
    } 
  }
}

3

Run the maven command build instead of clean build

See the remapper mistakenly remapping FluidState to FluidType.

import net.minecraft.core.BlockPosition;
import net.minecraft.server.level.WorldServer;
import net.minecraft.world.level.IWorldReader;
import net.minecraft.world.level.material.FluidType;
import org.bukkit.block.Block;
import org.bukkit.craftbukkit.v1_19_R3.CraftWorld;

public class Test1 {
  public void test(Block block) {
    WorldServer level = ((CraftWorld)block.getWorld()).getHandle();
    BlockPosition blockPos = new BlockPosition(block.getX(), block.getY(), block.getZ());
    FluidType fluidState = level.b_(blockPos);
    if (!fluidState.c()) {
      FluidType fluidType = fluidState.a();
      level.a(blockPos, fluidType, fluidType.a((IWorldReader)level));
    } 
  }
}

4

Now add some random code to the class which SHOULD make maven remove the cached class from the target dir, to regenerate it (which seems not to happen)

import net.minecraft.core.BlockPos;
import net.minecraft.server.level.ServerLevel;
import net.minecraft.world.level.material.Fluid;
import net.minecraft.world.level.material.FluidState;
import org.bukkit.block.Block;
import org.bukkit.craftbukkit.v1_19_R3.CraftWorld;

public class Test1
{
    public void test(Block block)
    {
        ServerLevel level = ((CraftWorld) block.getWorld()).getHandle();
        BlockPos blockPos = new BlockPos(block.getX(), block.getY(), block.getZ());
        FluidState fluidState = level.getFluidState(blockPos);
        if (!fluidState.isEmpty())
        {
            Fluid fluidType = fluidState.getType();
            level.scheduleTick(blockPos, fluidType, fluidType.getTickDelay(level));
            int test = 0;
            if(test == 0)
            {
                System.out.println("test");
            }
        }
    }
}

And see again how the Fluid class is mistakenly renamed.


import net.minecraft.core.BlockPosition;
import net.minecraft.server.level.WorldServer;
import net.minecraft.world.level.IWorldReader;
import net.minecraft.world.level.material.Fluid;
import net.minecraft.world.level.material.FluidType;
import org.bukkit.block.Block;
import org.bukkit.craftbukkit.v1_19_R3.CraftWorld;

public class Test1 {
  public void test(Block block) {
    WorldServer level = ((CraftWorld)block.getWorld()).getHandle();
    BlockPosition blockPos = new BlockPosition(block.getX(), block.getY(), block.getZ());
    Fluid fluidState = level.b_(blockPos);
    if (!fluidState.c()) {
      FluidType fluidType = fluidState.a();
      level.a(blockPos, fluidType, fluidType.a((IWorldReader)level));
      int test = 0;
      if (test == 0)
        System.out.println("test"); 
    } 
  }
}

This happens because the maven plugin is remapping the code a second time and remaps Fluid by mistake because of the remapping map:

c   net/minecraft/world/level/material/FluidState   net/minecraft/world/level/material/Fluid

c   net/minecraft/world/level/material/Fluid    net/minecraft/world/level/material/FluidType

Temporary solution 1

My solution is to basically run clean build to avoid this from happening but it makes my build time longer.

Temporary solution 2

Separate your plugin into modules and add this plugin into the maven module containing the NMS code. It will force clean up the previous built code even if you run the package command without the clean argument.

 <plugin>
   <artifactId>maven-clean-plugin</artifactId>
   <version>3.2.0</version>
   <executions>
       <execution>
           <id>auto-clean</id>
           <phase>initialize</phase>
           <goals>
               <goal>clean</goal>
           </goals>
       </execution>
   </executions>
</plugin>

Possible solution

Somehow implement a way to know if a file was already remapped, delete it from the maven build cache folder and compile then remap it again.

Final notes

I'd like to know if you think there is a solution for this. Thanks a lot for your hard work.

Alvinn8 commented 1 year ago

Thanks for the well-made bug report! Yep, it does seem like it is remapping twice as you said. I'm rather pressed for time at the moment, so I will have to take a look at solving this at a later date. In the meantime, thanks for sharing the temporary solutions in case other people encounter the same problem. PRs are welcome in case anyone wants to give it a stab.

LoneDev6 commented 1 year ago

Bonus

You can use an alternative cache plugin to make the build faster even after using one of the 2 workarounds I provided: https://github.com/apache/maven-build-cache-extension

This plugin works fine with the paper-nms-maven-plugin. I tested it with my project which used to take 35 seconds to build, now it takes 4 seconds. Build time increases only if a module is changed since it compiles only the changed module.

Note: this works only if you have correctly structured your project to be a multi module project. It surely won't do any difference if you have a single module project.

Alvinn8 commented 1 year ago

@LoneDev6 Hello! I have pushed a fix for this as version 1.4-SNAPSHOT, so give that a try if you can. It appears to fix the issue in the simple reproduction examples you provided in the PR description. I'll make some other changes tomorrow and release it as version 1.4.

LoneDev6 commented 1 year ago

You're a chad. Do you have Discord? I want to make a donation for your efforts

Alvinn8 commented 1 year ago

That's very kind of you, thanks for your support, but you really don't need to! I'm alvinn8 on discord.