Sophisticated Storage

Sophisticated Storage

36M Downloads

EMI Recipe Tree Highlight not working

CodeMastr3 opened this issue ยท 8 comments

commented

When utilizing the recipe tree of EMI it normally highlights items in chests that are needed for the recipe, this works in at least vanilla chests but in SophisticatedStorage chests it doesn't highlight the items.

Someone, storm03._. in discord, did some checking and found that at least for 1.20.1 the last version that worked with this feature of EMI was 1.20.1-1.3.43.1154.

I'm running Craftoria 1.21.3 which has 1.21.1-1.4.38

Example Image of me starting Recipe Tree
Image

Example Image of me opening a SophisticatedStorage chest with same Recipe Tree but with a required item in the chest
Image

Example Image of me opening a vanilla chest with same Recipe Tree but with a required item in the chest
Image

commented

@P3pp3rF1y just letting you know, I'm currently looking into this issue and I already know what is going on.

Basically because the storages are potential crafting stations Emi does not highlight the slots.

More technical:
In Emi's EmiScreenManager the renderSlotOverlays function is excluding all slots that are in a StandardRecipeHandler regardless of if the RecipeHandler can or can not craft at this point in time.

Not returning any slots if the recipe handler can't craft anything (e.g. because the crafting upgrade is not there) will fix the issue.

Replacing getInputSources in EmiGridMenuInfos with this:

@Override
    public List<Slot> getInputSources(C handler) {
		Optional<? extends UpgradeContainerBase<?, ?>> craftingContainer = handler.getOpenOrFirstCraftingContainer(recipeType);
		return craftingContainer.map(craftingHandler -> {
			List<Slot> slots = new ArrayList<>(handler.realInventorySlots.stream().filter(s -> s.mayPickup(Minecraft.getInstance().player)).toList());
			slots.addAll(getCraftingSlots(handler));
			return slots;
		}).orElse(Collections.emptyList());
    }

will work.

If you want I can make a PR and add the workaround for the Settings bug there too.

Or I can just sent you the patch for both in discord.

commented

@Salandora just wanted to let you know that that fix has one small downside, if the Crafting Upgrade is present in the storage, but is not opened/used (so the right side Crafting option is collapsed), it still just thinks container is able to craft and thus the fix breaks. Ideally, opening/closing the crafting option in the chest should be what dynamically determines whether the storage is able to craft, otherwise a chest that has the upgrade but crafting isn't opened can't really be easily used with the recipe tree (no highlights) to just draw items from.

It seems to at least work dynamically without reopening the storage to enable/disable the highlighting, since it starts highlighting if you grab the crafting upgrade out of it, without reopening the storage, so it could hopefully be a function tied to the Crafting menu opened/closed on the right side to do the same, checking crafting capability of the chest.

I do understand that that solution might be more convoluted and harder to implement! Your current solution at least works great to make sure the highlighting works at all for the storages, as long as there's no crafting upgrade present at all.

commented

I haven't taken a closer look at this yet, but one thing I definitely want to work is still being able to autofill (and automatically open) closed crafting upgrade as having to open the upgrade for auto fill to work seems pretty awkward to me. So if it's possible to detect closed crafting upgrade enable highlighting in that case and still have EMI show auto fill button for crafting that's good, but I am guessing that it just has a single flag that says if the gui has crafting and either shows highlighting or enables auto fill and in that case I prefer auto fill.

commented

Could we compromise on having both highlighting and the possibility of autofilling?

commented

@CodeMastr3 how do you suggest to do that? What I am saying is that if EMI can't have both in the same gui I am going for autofill and by definition that means there's no way to have highlighting in that case.
However we may be able to make highlighting work if there's no crafting upgrade in the storage.

commented

Only way I can see it work hybrid (highlight works with a crafting upgrade in the storage), is if you ignore the autofill also auto opening the crafting upgrade (if present). This way you could forward info to EMI (about the storage having or not having a crafting grid present, to adjust its assumption whether to highlight or be in crafting mode) depending on if the user clicks the crafting upgrade to be open or not. This'd have the downside of it not just auto assuming a storage with crafting upgrade is always usable to craft with, so if the grid is not open it'd auto open when e.g. EMI tries to craft (this is I presume what you want so that crafting is seamless when a crafting upgrade is present whether opened or not)?

Otherwise I definitely would like for the info of "no there's no crafting grid present here" to be passed to EMI when there's no crafting upgrade applied to the storage, Salandora's quick fix already seems to do that, which'd definitely be a desired feature I think.

Whether it should

  1. work hybrid - pros: even with a crafting upgrade, when its closed highlighting could work, and if grid is opened, EMI would be in non-highlight craft mode, cons: for better or worse, the user would need to manually toggle crafting mode by opening the craft upgrade, thus can't just seamlessly autofill craft in a storage with the upgrade regardless of grid opened or not

or

  1. highlight strictly only working in storage without crafting upgrade present - pros: storage with the upgrade has seamless crafting autofill whether the grid is open or not, making crafting feel nicer, cons: if a player for any reason applies crafting upgrades to more chests, it breaks highlighting with those entirely, so this'd depend on the player strictly only ever placing the upgrade on chests they do not wish the highlight to function in and seamlessly craft in, this might run into issues with some who might play as "eh might as well put it in a lot of chests for convenience" who'd care more about highlighting still working over the craft autofill auto opening the upgrade (they might think the highlight vs craft mode should have a step of user action in between, the manual clicking the crafting upgrade open/close is the user deciding whether the chest should be in craft vs highlight mode, and care more for this over the autofill auto opening craft function)

depends on what a player would like more, so it's hard to say. Ideally it could be a config option that the user can configure, e.g. "treat storages with crafting upgrade as always in crafting mode" with a note that this auto opens the grid to craft, but stops highlighting from working in said storage. Downside is that means implementing both functions which costs more work for something already somewhat niche (despite imo EMI being pretty much better in every way to JEI, it's still not as widely adopted in popular packs so lower userbase), the config option itself could look complicated and out of place if a user doesn't have EMI (so then the config's presence would need to depend whether EMI is detected to be also present or not, making the whole thing even more complicated).

I'm personally partial to the hybrid solution, because to me storage first and foremost is still a place items are stored and retrieved from, them being able to on demand craft without closing them (by manually opening the crafting upgrade) is nice but shouldn't compromise their primary function (of storing and retrieving items, the latter being made worse if highlight on what to retrieve stops working if the upgrade is present, even if closed). The crafting upgrade is convenient when one might do a quick craft still in a chest (and because of this, what I observed everyone do on our server including me is to just place crafting upgrades in pretty much every storage, so that the option to do so is there, but it'd feel bad if this meant highlighting stops working with all those), but I'd still primarily craft with dedicated crafting options (like any crafting table/station, or crafting on a stick wherever with a button press) after collecting items needed from various storage. It's unlikely that one chest somehow contains all items I need for a craft, so I'd never assume by default that I'd do the crafting in a chest, I'd say the chance is a lot bigger that I open a chest to place or retrieve items compared to using the craft function in it.

commented

I'll come back to this once I finished my little rewrite of the fabric port which took longer then I would like to admin.

My current Idea is to override the render function and highlight the necessary items this way IF and only if I can retrieve the list of items that needs highlighting somehow without having to rely on non API functions.
This way it would work without a crafting upgrade, a closed upgrade and an open upgrade.
But I haven't fully looked into it yet due to aforementioned rewrite.

commented

That'd be beyond excellent of course, best of all worlds.

In the meanwhile my suggestion/wishlist would be as the relatively smallest effort interim solution is to just have it rely on crafting upgrade being present or not as per your earlier workaround. Just so that users can keep highlight working as before, as long as they don't install crafting upgrades, shipped as a temporary solution in whenever next (unrelated) mod update happens, so that players don't need to manually downgrade these mods by a couple versions for it to work at all (especially in modpacks that usually bundle latest version of its mods on pack update).

Thank you both for all your hard work on this!