Darkhax-Minecraft / Bookshelf

A library mod which adds additional code support beyond what is provided by Forge.
GNU Lesser General Public License v2.1
142 stars 39 forks source link

You're fucking up seeds #97

Closed Fortist closed 6 years ago

Fortist commented 6 years ago

https://github.com/CoFH/Feedback/issues/935 https://github.com/CoFH/Feedback/issues/987 https://github.com/CoFH/Feedback/issues/823 https://github.com/CoFH/Feedback/issues/868

BASICALLY, because you made all the seeds the oredict "seed" they give out the same thing regardless of which seed in the Phytowhatever Insolator.

Darkhax commented 6 years ago

After taking a while to look at this code, and asking some other devs to look it over as well, I think it is blatantly obvious that this is an issue with Thermal Expansion and the system KL has created. However, based on the issues you have shared he seems very intent on passing this issue on. To help solve this issue I have added a config option so you can disable the changes to the seed ore dict.

KingLemming commented 6 years ago

No, it's really not. There's absolutely no reason to mess up the OreDict with stuff like "seed," "ingot," or "ore."

Don't do that - it offers no actual benefit, and there is already ESTABLISHED convention for this - listAllSeeds. Harvestcraft has been doing this for years and not screwing things up.

jaredlll08 commented 6 years ago

@Fortist modders are people too, just because stuff is broken doesn't give you a reason to be rude to them.

@KingLemming actually, you are in the wrong here, as shown in the OreDictionary class (provided by forge here: https://github.com/MinecraftForge/MinecraftForge/blob/1.12.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java) )

All logs are oredicted together: https://github.com/MinecraftForge/MinecraftForge/blob/1.12.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java#L87-L88

All planks are oredicted together: https://github.com/MinecraftForge/MinecraftForge/blob/1.12.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java#L89-L90

All saplings are oredicted together: https://github.com/MinecraftForge/MinecraftForge/blob/1.12.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java#L98

All leaves are oredicted together: https://github.com/MinecraftForge/MinecraftForge/blob/1.12.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java#L99-L100

All dyes are oredicted together: https://github.com/MinecraftForge/MinecraftForge/blob/1.12.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java#L150

All sand blocks are oredicted together: https://github.com/MinecraftForge/MinecraftForge/blob/1.12.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java#L184

All sandstone blocks are oredicted together: https://github.com/MinecraftForge/MinecraftForge/blob/1.12.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java#L185-L186

All glass blocks are oredicted together: https://github.com/MinecraftForge/MinecraftForge/blob/1.12.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java#L205

All glass panes are oredicted together:

https://github.com/MinecraftForge/MinecraftForge/blob/1.12.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java#L209

So it is PERFECTLY valid to oredict items like this.

If you made a machine that used dyes like you are using seeds, would you complain to forge about their bad oredict conventions?

KingLemming commented 6 years ago

@jaredlll08 - Your examples do not counter my point - there is nothing wrong with "prefixPostfix" notation, that has LONG been accepted. Things like "sand" are also fine as it's a full block/item name, which is also acceptable. In fact, the only possibly questionable entries in the Forge defaults are the records. The issue is when you oredict something with ONLY a prefix - you should not oredict all ores as "ore" in addition to what they actually are.

I'm aware of what the Ore Dictionary allows for, and I understand full well what can be done with it. But I also know that everything Forge has added by default was done with purpose. The only one which is sort of annoying is "dye" but it still falls within convention because "dye" is the name of the item.

Now, there's also another convention which is used that could probably solve this issue properly - mod prefixing.

As I understand it, these oredicts are being thrown on vanilla items for the purposes of Bookshelf recipe compatibility. In that case, the convention used by many other mods would be to do bookshelf:seeds as an oredict name. That will guarantee OreDict support for these recipes without the possibility of messing up everything else.

jaredlll08 commented 6 years ago

I never said there was anything wrong with "prefixPostfix", what I'm saying is in this instance, according to forge OreDictionary, "seed" is a valid entry, much like how Forge Oredicts all chests as "chest", and then oredicts each of them individually (https://github.com/MinecraftForge/MinecraftForge/blob/1.12.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java#L213-L218)

As for the mod prefixing, I have never seen that before and kinda goes against the whole purpose of the oredict as stated here: https://mcforge.readthedocs.io/en/latest/utilities/oredictionary/#oredictionary

Items that are registered to the OreDictionary become interchangeable with other items that have the same OreDictionary name.

So it seems counter intuitive to say "hey this oredict belongs to me, if you want to use it, you need to hardcode your mod to use it"

P3rf3ctXZer0 commented 5 years ago

@Fortist remove cofh based mods. (Previous thermal dynamics user)