[SUGGESTION] Spartan Weaponry and Spartan Shields compat

Sunconure11 commented 4 years ago

This set of compat would allow for a myriad of weapons to be made. There isn’t much more to be said.

Davoleo commented 4 years ago

I actually don't even know where to start to develop integration for that mod, mainly because it's close source and I didn't find any docs about some kind of API

Sunconure11 commented 4 years ago

There are several addons. You could also contact the dev of the mod

Xarmat-GitHub commented 3 years ago

I would love to see this. 🚀 At the moment I am work on a modpack with custom damage types (Distinct Damage Descriptions) and want to use "Spartan Weaponery" as main Weapon Mod. The Combination of all 3 mods would be amazing. Large variants of weapons of different kinds of metals that can be spread over many dimensions with custom damage types and the need to use them against different encounters.

Maybe the Spartan Shield Author would open the door for such an integration. There are others that already created integration mods. Maybe there is a way to do it.

Example: https://www.curseforge.com/minecraft/mc-mods/spartan-and-fire https://www.curseforge.com/minecraft/mc-mods/spartan-compatibility https://www.curseforge.com/minecraft/mc-mods/spartan-weaponry-arcana

TBiscuit1 commented 1 year ago

I've managed to make Metallurgy material be combined with Spartan Weaponry!

However some of the material won't trigger their effect for some reason

https://github.com/TBiscuit1/Metallurgy-4-Reforged-SpartanCompat/ Github page, I'll soon commit all the changed I've done

Davoleo commented 1 year ago

Hey @TBiscuit1, I've taken a look at your fork, first of all thanks for the effort! after taking a look in general I have some comments about the organization of code:

Code Separation

Currently most (if not all) of the code that references external libraries (or is used to create mod integration) is located in the integration.* sub-packages and I would like to keep it like this, since it helps maintainance as well as simplifying integration in some sort of logic modules. Some of your changes go against this since you're editing classes that are used to introduce content in the core mod. Below I'm listing some of these changes:

the main worrying change is the addition of the compat tool types to this enum https://github.com/TBiscuit1/Metallurgy-4-Reforged-SpartanCompat/blob/master/src/main/java/it/hurts/metallurgy_reforged/item/tool/EnumTools.java#L26
which then required you to change the getTool() methods in the Metal class to be nullable as well as increasing the size of the internal array that stores all the tool items, while the current code doesn't do anything out of the ordinary that would break in case getTool returned null, I feel like these changes pollute the internal codebase whose only responsibility should be to implement M4R original equipment.
And this specifc part of the code might also break when removing the mod since it's used very early on in the mod and as far as I've seen the code doesn't check it beforehands

This said, I understand you've tried to leverage the current core metals system to have item registration done autmatically, it's a cool idea, but I would still prefer if it was refactored to be done independently so that the code is not so much intertwined.

Code Duplication

Seems like integration API in Spartan weaponry is done in a very lazy way if we're the one who have to extend their classes and register all the items as well giving all of them models and textures; this will definitely pollute the codebase even more, so I'd like to investigate more if that's the only possible way to implement those features; also the way the API is structured forced you to create literally 22 identical classes for each tool type and it's way too much code duplication for me, vanilla tool types are just 5 so at the time I didn't bother trying to abstract to make maintainance easier, but in this case I'd prefer looking for some way to abstract it

Effect & Tool Compatibility

Implementing Spartan tools like this seem to have the upside of having effects automatically ported and available on the integration items as well, although I don't how much they would be compatible, this means if we implement integration this way there'll be a lot of testing needed to check everything is working as intended.

Textures and Models

Same as above I thought there would be a way to generate them starting from a color just like in TiCon, I'll have to investigate this more.

I also have a question, why did you have to bypass the "effect set" check in ItemUtils when you build the tooltip? it looks like a hacky solution and I would like to know what limitation the old version of the code put on what you could implement with it.

TBiscuit1 commented 1 year ago

For your info, I did this integration rather quickly and with little knowledge of Metallurgy's code, so it's botched, if I could, I would redo the whole thing

Yeah it was just to see if this worked out
Yeah, the way the spartan tools are made is def a problem, the 22 classes could def be combined into a single one with clever coding
Some of the effects are broken as I mentioned (Tartarite for exemple)
I legit didn't think about doing this

Oh and to answer the question, I have no clue on why I did that, I did that integration like a month or two ago (I had just forgot to commit)

Xarmat-GitHub commented 1 year ago

Don't want to disturb your conversation, I am following this quit a while and I am amazed when it will be released. 🙌

I have a feature request on this position: It would be nice if there is a config option to turn off material effects by mod pack creators. Just wanted to place it here, ty for the hard effort and have a blessed day. 👍

Xarmat-GitHub commented 10 months ago

I've managed to make Metallurgy material be combined with Spartan Weaponry!

However some of the material won't trigger their effect for some reason

https://github.com/TBiscuit1/Metallurgy-4-Reforged-SpartanCompat/ Github page, I'll soon commit all the changed I've done

@TBiscuit1 Sorry to bother, I am just to amazed about an integration of those shields for my modpack. Can you create a downloadable version to test and play with? (regardless of the current state and maybe missing textures / models) I would need also permission to put this into a modpack if possible. Thanks for the effort so far. Hope this will see daylight one day. 🚀

TBiscuit1 commented 10 months ago

I've managed to make Metallurgy material be combined with Spartan Weaponry! However some of the material won't trigger their effect for some reason https://github.com/TBiscuit1/Metallurgy-4-Reforged-SpartanCompat/ Github page, I'll soon commit all the changed I've done

@TBiscuit1 Sorry to bother, I am just to amazed about an integration of those shields for my modpack. Can you create a downloadable version to test and play with? (regardless of the current state and maybe missing textures / models) I would need also permission to put this into a modpack if possible. Thanks for the effort so far. Hope this will see daylight one day. 🚀

You are super lucky that I randomly went back to my modpack lmao, I can create the jar file and give it to you, what is your discord? (I'm also interested on what modpack you might be making lol)

Xarmat-GitHub commented 10 months ago

How amazing! 🚀 Here is my Discord ID: xartushig Display Name: Xarmat [@me] [1.12.2]

About the modpack:

I try to explain it as "short" as possible with focus on mechanics / features: - The player travels through 9 dimensions - [MAP](https://sites.google.com/view/dungeon-and-townsman/modpack-advanced/world-generation/dimensions) - Each DIM has 2 main metal types (mainly from Metallurgy) that in combination makes an alloy (so 3 types of metals each DIM) - Damage get split up into categories with the mod "Distinct Damage Descriptions" (mainly Slashing, Piercing, Bludgeoning damage types + magic) - [Curse Link](https://www.curseforge.com/minecraft/mc-mods/distinct-damage-descriptions) - This allow to handle damage based on the weapon the player wears (dagger vs humans = a lot of damage || dagger vs skeletons = low damage || hammer vs skeleton = a lot of damage) - With the combination of metal types, weapon types and scaling of damage / resistance values I want to generate a nice fighting experience where preparation is key (so reading a book give you hints what armor, weapon, second weapon, bow, shield, potions, rings, magic type, etc. you should take with you if you want to raid a ~Lich Dungeon without frustration) _-> prepares a way for deeper immersion_ So heaving access to metallurgy type of Spartan items is awesome for my pack. 😁 [Fancy looking pages, if you want to learn a bit more:](https://sites.google.com/view/dungeon-and-townsman/startseite)

Will wait for you to contact me. 😉

Davoleo commented 7 months ago

Implemented in 1.4.0, already available on CurseForge

Davoleo / Metallurgy-4-Reforged