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

suggestion: augment to disable pedestal sends of specific things #204

Open
Mowmaster opened this issue Apr 28, 2023 · 5 comments
Open

suggestion: augment to disable pedestal sends of specific things #204

Mowmaster opened this issue Apr 28, 2023 · 5 comments
Labels
1.19 Fix Released A Build for this version has been published. [1.19]

Comments

@Mowmaster
Copy link
Owner

this way a pedestal wouldnt send items, but would still send energy (in the case of an fe generator)
or wouldnt send fluids but would send xp (fluid converter)

@mfrankov
Copy link
Collaborator

mfrankov commented May 3, 2023

An alternative suggestion I was about to make when I saw this was to update BasePedestalBlockEntity.canAcceptEnergy to return false if the pedestal contains a generator upgrade (presumably would require exposing canAccept{Items,Fluids,Energy,XP,Dust} in IPedestalUpgrade and implementing those as appropriate in all the respective upgrades so they can be looked up in BasePedestalBlockEntity where needed).

That potentially simplifies the approach as it doesn't require more items/complexity for players to manage, and (at least to me) is more inline with my intuition/expectations of these blocks. At least most modded generators don't let you insert energy, and processing machines (even a vanilla furnace) don't let you put items into the output slot.

@Mowmaster
Copy link
Owner Author

If you've ever taken a look at the configs, 'energy' is a fairly dynamic concept in pedestals, this could be fluid, Fe, xp, or dust (from effect Scrolls mod)
Having to make conditions for each of these depending on the case where it's being used as an 'energy' would be a ton of work on my end. Potentially if I decide to rewrite this system I could do as you've suggested, but for now using filters in combination with an additional augment would be incredibly easy to implement.

If you decide you want to PR all the code to make this work for the dynamic energy types, im down for that. But I personally won't be spending any time to do this,especially with a 1.20 port on the horizon.

@mfrankov
Copy link
Collaborator

mfrankov commented May 4, 2023

I'm not sure I fully grasp the specific rewrite you'd think required (or would accept).

The issue I'm seeing here is that there seems to be a distinct difference in how pedestals versus upgrades handle the 5 types of transferrable things (items, fluid, energy [fe], experience, dust).

BasePedestalBlockEntity has 6 handlers (one for each of those 5 transferrable things, and privateHandler for the "coin”/glowstone/filter/redstone/augments/tool/work card). Those respective handlers control the overall ability for the pedestal to extract/receive each of their respective types (based on the filter NBT if one is present as implemented in MowLibBaseFilter and augmented by the Storage/Handler state [e.g. if the fe buffer is already full it won't accept more energy]). They don't currently take the Upgrade into account at all.

IPedestalUpgrade also has their own canTransfer* methods that are used by some upgrades to determine if they can “transfer” (which I’d more say “process”) those respective types, e.g. ItemUpgradeVoid seems to only void each of the types of things that can be contained within a pedestal if the void upgrade itself hasn’t been toggled to disable that type (which, like with the filters, is also stored in NBT — this time in ItemUpgradeBase).

I think to support what I was suggesting, the change would require adding something like pedestalCanReceive{Items,Fluids,Energy,XP,Dust} and pedestalCanExtract{Items,Fluids,Energy,XP,Dust} to IPedestalUpgrade (with a default implementation of true, and an @Override in the respective Upgrades that want to disable which specific type and direction should be excluded like ItemUpgradeGenerator_FurnaceFuels which would set pedestalCanReceiveEnergy to false, or ItemUpgradeFluidConverter which would set pedestalCanSendFluid to false based on your initial comment in this issue). Once those exist, it would require updating BasePedestalBlockEntity to have the various methods like canAcceptEnergy on L1576 to call getCoinOnPedestal and look at the appropriate pedestalCan{Receive,Extract}* assuming an upgrade is present in addition to looking at the filter (directly in the cases of items/fluids, or via the I*Storage for energy/xp/dust) and Handler/Storage state (again, e.g. if the buffer is full).

If what I describe above seems amenable to you — adding 10 new methods that control a single direction of each of the 5 types of transferrable things to IPedestalUpgrade and using them within BasePedestalBlockEntity — I can certainly take a stab at making a PR. I have plenty of Java/Scala development, just no experience with modded Minecraft development (so it’ll take me some time to get a dev & testing setup figured out).

@Mowmaster
Copy link
Owner Author

if your new to forge stuff, basically there is handlers setup(capabilities) for managing how TYPES are managed across all mods, adding custom code to these handlers is extremely risky, yet the proper way to go about this.(ive done this with filters already)[i also did this a lot in 1.16 and it created way more dupes and bugs so im not super keen to do this anymore, hence my reluctance to implementing your suggestion myself]

If you only changed my custom send methods it only prevents the pedestal from sending TYPES it wouldnt keep other mods from pulling them out unnecessarily(without also configuring the other mod not to do so)

my other hesitation is that by implementing upgrades that are hardcoded to restrict transport, users cannot control that themselves, i can guarantee ill see comments from users in the future complaining about this if its implemented. (similar complaints happened in 1.16 as i did something like this back then too [the anvil upgrade in 1.16 was the main offender])
my augment solution give the user complete control at the cost of some minor added complexity. (its also mostly copy paste code on my part so its incredibly easy to implement)

@mfrankov
Copy link
Collaborator

mfrankov commented May 5, 2023

Ah yes, the places I suggested above would only work on pedestal-pedestal transfer (but as you point out, I could add this into the actual handlers setup that are exposed via getCapabilities [i.e. where you've already done this with filters]).

my other hesitation is that by implementing upgrades that are hardcoded to restrict transport, users cannot control that themselves, i can guarantee ill see comments from users in the future complaining about this if its implemented.

That's more the reason why I was asking if you were okay with this approach as opposed to going out and just making a PR. While I should be able to throw together the implementation, I wouldn't have to deal with the long-term support impact of fielding questions (or impacting your creative direction around how you'd prefer users interact with your mod).

Happy to wait on having it be done via augments/filters. And in the meantime I'm still able to essentially handle this via pedestal indirection (i.e. using a pedestal between the generator and furnace to blacklist coal, and between the furnace and generator to blacklist power, so as to not create the unwanted transfers / infinite loop).

@Mowmaster Mowmaster added 'Back Burnered' Known issues, but not important enough to take away from pushing updates [1.19] labels May 16, 2023
@Mowmaster Mowmaster added 1.19 Fix Released A Build for this version has been published. and removed 'Back Burnered' Known issues, but not important enough to take away from pushing updates labels Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.19 Fix Released A Build for this version has been published. [1.19]
Projects
None yet
Development

No branches or pull requests

2 participants