Direct instantiation of Box Protection Shape does not enforce min/max coordinate validity

[williambl]

The Issue

When creating a BoxShape through its constructor, the min BlockPos parameter must be lesser than the max BlockPos parameter in all three axes, or an invalid BoxFilter is created which matches nothing.

For example, creating a BoxShape through new BoxShape(server.getOverworld().getRegistryKey(), new BlockPos(10, 10, 10), new BlockPos(-10, -10, -10)) creates a shape which does not accept any events.

Creating the shape through ProtectionShape#box does not have this issue. However, doing this is not always an option. I ran into this issue because part of a project involved creating Authorities in a data-driven way from JSON files, using Codecs. Since the BoxShape codec creates shapes using BoxShape's constructor, it falls victim to this issue, which was difficult to pin down (as it seemed to simply be 'the authorities are broken').

Possible fixes

Rejection of invalid arguments in BoxShape constructor

The following lines could be added to the start of BoxShape's constructor:

    public BoxShape(RegistryKey<World> dimension, BlockPos min, BlockPos max) {
+        if (min.getX() > max.getX() || min.getY() > max.getY() || min.getZ() > max.getZ()) {
+            throw new IllegalArgumentException("min (%s) is not less than max (%s)".format(min, max));
+        }
        this.dimension = dimension;

This would be useful in that it would prevent invalid boxes from being created at all, however this may not be wanted in all cases (the exception would have to be caught and handled, which may be annoying)

Automatically re-arranging coordinates

The lines

        BlockPos min = new BlockPos(Math.min(a.getX(), b.getX()), Math.min(a.getY(), b.getY()), Math.min(a.getZ(), b.getZ()));
        BlockPos max = new BlockPos(Math.max(a.getX(), b.getX()), Math.max(a.getY(), b.getY()), Math.max(a.getZ(), b.getZ()));

from ProtectionShape#box could be moved into the BoxShape constructor. However, this could be unwanted as it would mean that the values passed in for min and max would not necessarily be the same values given out (when (de)serialising through the codec).

Changing constructor used by codec

In the BoxShape codec:

            return scope.max;
-        })).apply(instance, BoxShape::new);
+        })).apply(instance, ProtectionShape::box);
    });

This would make creating a box shape through the codec work the same way as creating it through the /protect shape add command - coordinates are re-arranged so that min and max are valid. However, this has the same issue as the above solution where the values passed in for min and max would not necessarily be the same values given out when (de)serialising through the codec.

Do nothing

Perhaps the current situation is acceptable, and what I'm doing is recommended against. However, in that case, some warning comments/javadoc wouldn't hurt :P