Lithium (Fabric)

Lithium (Fabric)

22M Downloads

Offset must be one of {0f, 0.5f, 1f}. Crash with Supplementaies.

MehVahdJukaar opened this issue ยท 13 comments

commented

Version Information

Version is new one just gotten from CF on 1.20. Also happens on the forge ports.

Reproduction Steps

Install Supplementaries new update and click on a Sliding Block.

Sliding block has a block entity that extends MovingPistonBlockEntity

Lithum will make the game crash due to that offset check it added.

Here's the relevant log bit

image

latest.log

commented

This targeted check exists for good reason, specifically because there would be an NPE if the invariant assumed by lithium's optimization is violated, which is an objectively worse error experience. A more productive response to this instead of blocklisting lithium entirely is to use lithium's API to disable this specific lithium optimization with which your code conflicts. It's very easy and we even have a wiki page that describes how to do it and a list of all the possible optimizations. You just need to add a custom property to your metadata file (fabric mod json or mod toml on forge). The identifier for this lithium mixin is simply its package suffix block.moving_block_shapes. It's just as much effort as adding lithium to your custom (??) mod blocklist, doesn't unnecessarily degrade performance, and avoids breaking users' setups.

commented

The assertion is not at fault here. Lithium expects for a certain block shape and offset to be used by Pistons and it enables a fast path which relies on that. Fixing the problem is just a matter of having Lithium skip the optimizations specific to that block shape when mods are replacing it.

commented

@MehVahdJukaar The solution applied now will just throw you back to the vanilla/non-optimized code, if the block offset is not exactly -1, -.5, 0, .5 or 1, as the optimization is specifically made to handle these vanilla cases.

commented

I didn't know there was a way to disable certain features from a mod mod JSON. That's nice to know . Obviously wouldn't be an ideal solution so I'm glad a proper solution was taken with it falling back to vanilla logic. Will this be applied to 1.20 and 1.21?

commented

Probably there won't be a backport of this change. I suggest to use the mod json way to disable the optimization in case you still support the older versions

commented

alright will do. For both 1.21 and 1.20 or just 1.20?

on btw i reply to @douira I dont have a block list, merely a warn list as old 1.20 forge does not allow to specify mod conflicts in mods.toml. Merely use it to show a warning message warning users about a possible so I dont get the reports of that.

commented

Not sure yet about 1.21

commented

What about this?

if (offset < 0f || offset > 1f) {
throw new IllegalArgumentException("offset must be between 0f and 1f}");
}

I mean, offset is casted to an int either way and there are no index problems.
But if you, @CaffeineMC , want to round correctly, you can just write that:

44 return (int) (2 * offset + 0.5f) + 2 * direction.getId();

commented

I feel that too isnt enough. Vanilla wouldnt crash with an invalid offset (less than 0 or greater than 1), so, even if in this case it wouldnt matter it should still not implode when an arbitrry number is there, matching the vanilla behavior as it should since its modifying a vanilla class which mods can use and extend.
Adding forced crashes based off random arbitrary assumptions is bad