terrarium-earth / Heracles

A tree style questing mod allowing creators to set completable quests for their users
MIT License
29 stars 16 forks source link

[Feature Request]: Toggle block/item registration via the config #235

Closed sisby-folk closed 3 months ago

sisby-folk commented 3 months ago

Is your feature request related to a problem?

<- people who publish like the only heracles pack with no modded blocks or items in it

Solution(s)

Two booleans in the config file, one for the quest book, the other for utility blocks/items - when disabled, said things are not registered at all on init, and appropriate checks are added to avoid crashes in this case.

Describe alternatives you've considered

"just hide them from recipe viewers" yeah but that's what we did for FTB - heracles should be able to do better.

CodexAdrian commented 3 months ago

We'd need to test out if adding a config to disable items would still work with vanilla clients joining a modded server with Heracles. Heracles sends out custom packets to players, and it seems like sending custom packets that arent received by the client properly on 1.21 will kick the user from the server. Introducing this feature introduces a ton of other headaches that come from non-syncable configs (like registry condition configs), so we need to explore whether or not itd even be beneficial. If your only goal is just for the sake of disabling the item because you dont want to use it, then I would say that disabling it via the recipe viewers is the best option given how many instability issues can come from having config based registries.

sisby-folk commented 3 months ago

this is exclusively a modpack use case - I control both sides, so there's no concern. simply do not touch the config in other cases.

CodexAdrian commented 3 months ago

Oh.. then I would say that disabling the item would be a better solution for this problem via other means. Allowing the disabling of registries introduces alot of other issues that we were willing to overlook if it enabled the use of vanilla clients on servers with heracles for optional quests. But if we're strictly talking about a modpack exclusive use case, disabling via JEI is the most reliable solution

sisby-folk commented 3 months ago

Issue is not completed, and I'm going to be PRing this and using the fork regardless of whether it's merged

CodexAdrian commented 3 months ago

I meant to close the issue as not planned. Hard forking the project feels like a disproportional response to what we felt was a valid concern. Introducing this feature for the mod as a whole would introduce other issues when it comes to config mismatch. I dont really understand why using JEI to disable the item from view isnt enough of a valid solution. And I also dont understand what would be the point of introducing an unstable feature to the base mod so that your use case is fixed only for you to continue to use your fork anyways. Please don't take the time to make a PR for this feature as its not something we think would be good for the mod. If you still feel disabling the items registration is the only viable solution and you're going to use a fork anyways, then itd be better to just make your fork not have the items at all.

sisby-folk commented 3 months ago

it's not a disproportionate response intended to offend or anything - we just want this version of the mod so it's faithful to our modpack, so if it's not master, it's a forked jar - nothing personal.

sisby-folk commented 3 months ago

A configurable version will be a smaller diff as we won't actually have to remove the relevant classes etc, so, easier to maintain.

I don't particularly see why that use case would be particularly offensive or unstable - just close the PR when you get it if that's a genuinely unanimous choice.