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

Furnace Cart resupplying works incompatible with lithium #718

Closed
skymen opened this issue May 20, 2024 · 14 comments
Closed

Furnace Cart resupplying works incompatible with lithium #718

skymen opened this issue May 20, 2024 · 14 comments

Comments

@skymen
Copy link

skymen commented May 20, 2024

Hi, thanks for fixing my issue yesterday!

I'm trying out the new release, but I can't figure out how furnace cart resupplying is supposed to work. I have a hopper pointed at a furnace cart with fuel inside of it, and the cart doesn't get filled up.

Reading the code, I don't understand what the trigger for refueling is.

I am on latest released version, and I have the new feature enabled in the config.

@SFort
Copy link
Collaborator

SFort commented May 20, 2024

out.mp4

i have no idea maybe a mod conflict? (also this made me notice the feature crashes on forge, which i may have forgotten to test)

@SFort
Copy link
Collaborator

SFort commented May 20, 2024

also ye sorry the code is jank.
i thought of two ways to implement this.

a.) make the furnace minecart an actuall inventory
this would be the best since it would ensure copatability for basicly any mod which does inventory transfer as well.
however it means i gotta add new methods to the FurnaceMinecart class which means implementing a vanilla interface.

Fabrication currently has no way to do this is a safe manner and tbh probably? wont get one.

b) make the hopper check for furnace carts and give it a fake entity with an inventory to act as an intermediary.

i pulled a bit of a hack to make it not have to check for entities twice, which is better for performance.
i think i have an idea to make this less hacky.

@SFort
Copy link
Collaborator

SFort commented May 20, 2024

try 3.4.17

@skymen
Copy link
Author

skymen commented May 21, 2024

Ok, I found the culprit. The mod isn't compatible with lithium's default config. Specifically the rule mixin.block.hopper that seems to be doing a bunch of underlying changes to the hopper mechanic
https://github.com/CaffeineMC/lithium-fabric/blob/44f4330b9ab1cef568e80336d949c61386f747b9/src/main/resources/lithium.mixins.json#L51

I'm not too well versed with how Minecraft works internally, but reading over the general purpose of each rule, this mixin might be the culprit?
https://github.com/CaffeineMC/lithium-fabric/blob/44f4330b9ab1cef568e80336d949c61386f747b9/src/main/java/me/jellysquid/mods/lithium/mixin/block/hopper/AbstractMinecartEntityMixin.java#L20

I'm guessing it's trying to optimise the minecart-hopper interaction by preventing minecarts with no inventory attached from triggering events that it deems unnecessary.

In case you can't have it fixed in fabrication, I can try reporting it as a bug to lithium so they can take it into account, or at least provide a config rule that disables that specific mixin instead of every hopper optimisations at once. Worst case scenario, I just add one line of config to disable the whole thing and I should be good to go 👍

Still, since this rule is enabled by default with lithium, and it's installed in many modpacks it might cause confusion in the future.

@SFort
Copy link
Collaborator

SFort commented May 21, 2024

I'll add it to the feature description. there's not much else that can be resonably done about this.
(reminds me of 376, which should also have it added to desc)

@SFort SFort closed this as completed May 21, 2024
@SFort
Copy link
Collaborator

SFort commented May 21, 2024

actually no wait i think 376 was fixed, so this should be do-able

@SFort SFort reopened this May 21, 2024
@SFort SFort changed the title Unsure how Furnace Cart resupplying works Furnace Cart resupplying works incompatible with lithium May 21, 2024
@SFort
Copy link
Collaborator

SFort commented May 21, 2024

Thanks for debuging it.
I got half way through making a compatability fix but it's waay too hacky and requires messing with lithium in ways which break from one version to the next.
Really wish i could just implement inventory...

Try asking the lithium devs, they seem to have added handling for the wierd wool hopper mechanic so maybe thei've got an approach for this. (if they think compat code is required on both sides message me)

Here's the relavent code:
https://github.com/FalsehoodMC/Fabrication/blob/3.0/1.18/src/main/java/com/unascribed/fabrication/mixin/d_minor_mechanics/furnace_minecart_resupplying/MixinHopperBlockEntity.java#L31

(FabInject is just mixins Inject but with a few extra checks)

@2No2Name
Copy link

2No2Name commented May 22, 2024

Ok, I found the culprit. The mod isn't compatible with lithium's default config. Specifically the rule mixin.block.hopper ...

... this mixin might be the culprit? https://github.com/CaffeineMC/lithium-fabric/blob/44f4330b9ab1cef568e80336d949c61386f747b9/src/main/java/me/jellysquid/mods/lithium/mixin/block/hopper/AbstractMinecartEntityMixin.java#L20

Lithium developer here. The mixin only influences lithium's internal movement listening system, which is supposed to tell hoppers to check for inventory minecarts after a nearby inventory minecart moved.

Lithium has another feature that makes block entities (including hoppers) sleep under certain conditions, with certain events waking them up again (e.g. an inventory entity moving nearby). This might be incompatible with your additional hopper feature, because the sleeping and waking conditions are not implemented. I suggest testing with the line mixin.world.block_entity_ticking.sleeping.hopper=false added to the lithium.properties file in the config folder in the .minecraft folder. If that fixes the issue:

Here is an instruction how you can disable that lithium setting automatically when your mod is installed:
https://github.com/CaffeineMC/lithium-fabric/wiki/Disabling-Lithium's-Mixins-using-your-mod's-fabric-mod.json

For example: fabric-mod.json :

"custom": {
  "lithium:options": {
    "mixin.world.block_entity_ticking.sleeping.hopper": false
  }
}```

@SFort
Copy link
Collaborator

SFort commented May 22, 2024

@2No2Name (mentioning in case gh didn't follow the issue)

i probably should have repeated what was mentioned in 376

Fabrications features can generally be toggled while in game. so anything todo with fabric-mod.json is not an option.

the way 376 ended up being resolved is this
https://github.com/FalsehoodMC/Fabrication/blob/3.0/1.18/src/main/java/com/unascribed/fabrication/support/MixinConfigPlugin.java#L517
(inserting a FabConf.isEnabled check into lithiums mixin)
this has worked suprisingly well, considering that was is 2021 and i haven't heard a peep about it since.

i tried doing something simmilar in this case, that being mostly just going arount and changing "instanceof Inventory" to "instance of Inventory | instanceof FabricationFurnaceMinecart" inside lithium.
this mostly works(with additional tweaks), unlike with 376 it also maintains both mods changes, however it is far more unsafe and lithium has refactored this code across versions meaning it'd be an absolute pain to maintain.

ideally we could get something that works without loosing the optimization and i think it's possible, although it probably requres changes in both mods?

otherwise i'll just have to add a discraimer to the feature description to go manually disable the lithium optimization.

@2No2Name
Copy link

2No2Name commented May 23, 2024

I suggest to change your implementation to actually make your custom furnace minecart a vehicle inventory entity (similar to chest boat) that immediately acts when it receives an item, similarly to a composter that is getting filled up. Vanilla furnaces and similar blocks also already implement the possibility of disallowing item transfers under certain conditions.
Alternatively, fabric api also allows creating inventories at certain locations, but I am not sure if this is suitable for entities

@SFort
Copy link
Collaborator

SFort commented May 24, 2024

That would be the best solution.
it just requires fabrication to implement a vanilla interface on a vanilla class.
which gets really messy as soon as another mod has that idea, and since fabrication has houndrets of features i don't think it can affort the risk of using those kinds of injections.

"immediately acts when it receives an item", makes some mods have dupe bugs, sadly an inventory has to be reversable at least in the same tick. (but that's not really relavent)

Thanks for the advice, i immagine you deal with a fair bit of this.

@SFort
Copy link
Collaborator

SFort commented May 24, 2024

mixin.world.block_entity_ticking.sleeping.hopper=false Isn't the only conflict

SFort added a commit that referenced this issue May 25, 2024
@2No2Name
Copy link

"immediately acts when it receives an item", makes some mods have dupe bugs, sadly an inventory has to be reversable at least in the same tick. (but that's not really relavent)

Of course you could also implement the updating during the furnace minecart tick, checking whether any items were inserted until then

@SFort
Copy link
Collaborator

SFort commented May 29, 2024

yup, just pointing it out in case you have any code which needs to be reconsidered. (it's not the important part, just a side note)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants