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.

Sign up for GitHub

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

Jump to bottom

Add EssentialCraft 4 compatibility #176

Merged
merged 6 commits into from
Jul 6, 2024
Merged

Add EssentialCraft 4 compatibility #176

merged 6 commits into from
Jul 6, 2024

Conversation

Wizzerinus
Copy link
Contributor

@Wizzerinus Wizzerinus commented Jun 8, 2024 •
edited
Loading

List of machines added: DemonTrade, MagicianTable, MagmaticSmeltery, MithrilineFurnace, RadiatingChamber, WindRune. There are three notes I wanted to add:

  • Demon Trade is bugged and will not accept any item with a different NBT, which breaks like 30% of the recipes of the mod itself. While GrS can set the item NBT, this should be probably noted somewhere in the docs, not sure though.
  • Magmatic Smeltery currently cannot be reloaded. The machine isn't usually being used to add things at runtime, and I am surprised it works, but then apparently something caches the output of getSubItems causing an NPE. Or I am getting trolled by a client-server desync (the most likely). Theoretically I could mixin into the clientside methods of OreSmeltingRecipe, but that seems a bit disruptive. I've currently added a check that makes sure to not reload Magmatic Smeltery recipes to avoid the NPE.
  • Also on a similar note: Magmatic Smeltery does not have a JEI handler.
  • I wanted to add the support for the mod's Modular Guns system, but since it requires having 12 textures per gun material I think it is a bit annoying to set up examples for it, could still do it though if desired.

@WaitingIdly WaitingIdly added the mod compat Relating to compatability with a mod or features of a mod label Jun 9, 2024
WaitingIdly
WaitingIdly requested changes Jun 16, 2024
View reviewed changes
Copy link
Collaborator

@WaitingIdly WaitingIdly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the delay

src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/RadiatingChamber.java Outdated Show resolved Hide resolved
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/MithrilineFurnace.java Show resolved Hide resolved
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/MagmaticSmeltery.java Outdated Show resolved Hide resolved
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/MagicianTable.java Outdated Show resolved Hide resolved
gradle.properties Outdated Show resolved Hide resolved
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/DemonTradeManager.java Show resolved Hide resolved
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/DemonTradeManager.java Outdated Show resolved Hide resolved
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/MagicianTable.java Outdated Show resolved Hide resolved
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/MithrilineFurnace.java Outdated Show resolved Hide resolved
src/main/resources/assets/groovyscript/lang/en_us.lang Outdated Show resolved Hide resolved
@Wizzerinus
Copy link
Contributor Author

all good! thank you for the review regardless

pushed most fixes, although I don't understand the ALL_MOBS one

@Wizzerinus
Copy link
Contributor Author

ok this should be all

WaitingIdly
WaitingIdly requested changes Jun 23, 2024
View reviewed changes
Copy link
Collaborator

@WaitingIdly WaitingIdly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

took a bit because i wanted to get the mixin fix for removing smeltery stuff done. which now it is.

src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/DemonTradeManager.java Outdated Show resolved Hide resolved
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/MagicianTable.java Show resolved Hide resolved
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/MagmaticSmeltery.java Outdated Show resolved Hide resolved
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/MagmaticSmeltery.java Show resolved Hide resolved
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/MagmaticSmeltery.java Outdated Show resolved Hide resolved
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/MithrilineFurnace.java Outdated Show resolved Hide resolved
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/RadiatingChamber.java Outdated Show resolved Hide resolved
@Wizzerinus
Copy link
Contributor Author

all of these resolved, with an extra fix for a magmatic smeltery crash when an invalid ore is inserted

@Wizzerinus
Copy link
Contributor Author

done

WaitingIdly
WaitingIdly reviewed Jul 5, 2024
View reviewed changes
src/main/java/com/cleanroommc/groovyscript/compat/mods/essentialcraft/MagmaticSmeltery.java
import essentialcraft.api.OreSmeltingRecipe;
import org.jetbrains.annotations.Nullable;

@RegistryDescription(reloadability = RegistryDescription.Reloadability.DISABLED, admonition={
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since theres only one admonition, can remove the curly braces around it. should have your IDE highlighting it.

@brachy84 brachy84 merged commit 9005864 into CleanroomMC:master Jul 6, 2024
@Wizzerinus Wizzerinus deleted the compat/ec4 branch July 7, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brachy84 brachy84 brachy84 approved these changes

@WaitingIdly WaitingIdly WaitingIdly approved these changes

Assignees
No one assigned
Labels
mod compat Relating to compatability with a mod or features of a mod
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Wizzerinus @WaitingIdly @brachy84