Add recipes for clearing facades #1452

kirjorjos · 2024-12-31T16:31:20Z

Adds the squeezer facade recipes as mentioned in #1424

rubensworks

rubensworks · 2025-01-01T08:44:16Z

src/main/java/org/cyclops/integrateddynamics/blockentity/BlockEntitySqueezer.java

-                        for (RecipeSqueezer.IngredientChance itemStackChance : recipe.getOutputItems()) {
-                            if (itemStackChance.getChance() == 1.0F || itemStackChance.getChance() >= level.random.nextFloat()) {
+                        for (RecipeSqueezer.IngredientChance itemStackChance : recipe.assemble(oldItem)) {
+                        if (itemStackChance.getChance() == 1.0F || itemStackChance.getChance() >= level.random.nextFloat()) {


Indentation seems to be wrong here.

I definitely missed that, I'll fix that indentation and the JSON file indentations

rubensworks · 2025-01-01T08:47:04Z

src/main/java/org/cyclops/integrateddynamics/core/recipe/type/FacadeSqueezeCalculator.java

+    public static NonNullList<IngredientChance> getOutputItems(ItemStack inputItem) {
+        ItemStack facadeItemStack = new ItemStack(RegistryEntries.ITEM_FACADE);
+
+        facadeItemStack.setTag(null);


This shouldn't be necessary.

Initially, my testing was giving back facades with empty NBT which didn't stack with the ones already in world and from the other recipes, but testing now, it is in fact unnecessary so I'll remove it.

rubensworks · 2025-01-01T08:48:46Z

src/main/java/org/cyclops/integrateddynamics/core/recipe/type/FacadeSqueezeCalculator.java

+        Either<ItemStack, ItemStackFromIngredient> facadeEither = Either.left(facadeItemStack);
+        IngredientChance facade = new IngredientChance(facadeEither, 1.0F);
+
+        CompoundTag nbt = inputItem.getOrCreateTag();


If I understand correctly, the following lines attempt to check if the item is a block?
If so, you could just check if item is instanceof ItemBlock. That would simplify this code quite a bit.

I'm not trying to check if it's a block anywhere, just grab the assumed block from the NBT, letting it be an empty ItemStack if malformed or missing NBT as that would be the case in squeezing a blank facade

You can probably use this one then: https://github.com/CyclopsMC/IntegratedDynamics/blob/master-1.21-lts/src/main/java/org/cyclops/integrateddynamics/item/ItemFacade.java#L41

rubensworks · 2025-01-01T08:51:48Z

src/main/java/org/cyclops/integrateddynamics/core/recipe/type/RecipeSqueezer.java

    }

+    public NonNullList<IngredientChance> assemble(ItemStack inputItem) {
+        if (!inputItem.is(RegistryEntries.ITEM_FACADE.asItem())) return getOutputItems();


I still think it is possible to create a custom recipe serializer type so that we don't have to hardcode things like this.
Concretely, the serializer could provide a subclass of RecipeSqueezer that just overrides this assemble method.
The "type" entry in the recipe JSON would then refer to this new serializer, e.g. integrateddynamics:squeezer_facade_unwrap.

The problem with your current approach is that things become more difficult to maintain like this.

The helper function in FacadeSqueezeCalculator could then be moved to that serializer class.

I'm fine moving the method from the FacadeSqueezeCalculator to a separate serializer, but I thought the type key in the JSON object referenced the recipe type, not the serializer?

The type refers to the serializer. For example, the clear_part.json recipe refers to the integrateddynamics:crafting_special_nbt_clear serializer, which is of the crafting recipe type.

rubensworks · 2025-01-01T08:51:59Z

...ta/integrateddynamics/recipes/mechanical_squeezer/convenience/mechanical_facade_squeeze.json

+  "item": "integrateddynamics:facade",
+  "result": {
+	"items": [
+	      {


Indentation seems a bit off in this file.

src/main/resources/data/integrateddynamics/recipes/squeezer/convenience/facade_squeeze.json

kirjorjos · 2025-03-02T04:57:18Z

I'm not sure if there's a certain way I should let you know that I believe I've addressed everything you mentioned, but I had pushed 3 commits on February 3rd that for some reason are showing grouped with the ones from December. Apologies if you had noticed and hadn't gotten to it yet, but wanted to say something as it has been a bit.

rubensworks · 2025-03-02T05:28:55Z

There should be a button to re-request a review from someone. Somewhere on the top-right of this page.

rubensworks

Thanks! This is the direction I had in mind as well :-)

buildAndRunClient.sh

...ta/integrateddynamics/recipes/mechanical_squeezer/convenience/mechanical_facade_squeeze.json

src/main/resources/data/integrateddynamics/recipes/squeezer/convenience/facade_squeeze.json

src/main/java/org/cyclops/integrateddynamics/core/recipe/type/RecipeFacadeSqueeze.java

...main/java/org/cyclops/integrateddynamics/core/recipe/type/RecipeFacadeSqueezeMechanical.java

...main/java/org/cyclops/integrateddynamics/core/recipe/type/RecipeSerializerFacadeSqueeze.java

src/main/java/org/cyclops/integrateddynamics/core/recipe/type/RecipeFacadeSqueeze.java

...main/java/org/cyclops/integrateddynamics/core/recipe/type/RecipeFacadeSqueezeMechanical.java

...ta/integrateddynamics/recipes/mechanical_squeezer/convenience/mechanical_facade_squeeze.json

src/main/resources/data/integrateddynamics/recipes/squeezer/convenience/facade_squeeze.json

...main/java/org/cyclops/integrateddynamics/core/recipe/type/RecipeFacadeSqueezeMechanical.java

rubensworks · 2025-05-18T06:17:12Z

Rebasing on master may be required as well, as there seem to be merge conflicts.

Jack-McKalling · 2025-05-18T06:40:51Z

Rebasing on master may be required as well, as there seem to be merge conflicts.

the thing says this:

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

but right above that it says this instead:

Caused by: org.gradle.internal.resource.transport.http.HttpErrorStatusCodeException: Could not GET 'https://maven.pkg.github.com/CyclopsMC/packages/org/cyclops/commoncapabilities/commoncapabilities/1.19.2-2.9.0-88/commoncapabilities-1.19.2-2.9.0-88.pom'. Received status code 401 from server: Unauthorized

(deepest nested exception)
All of that doesn't sound to me like it's just a GIT operation that would solve it, but I don't know anything.

kirjorjos · 2025-05-18T12:31:07Z

Rebasing on master may be required as well, as there seem to be merge conflicts.

the thing says this:

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

but right above that it says this instead:

Caused by: org.gradle.internal.resource.transport.http.HttpErrorStatusCodeException: Could not GET 'https://maven.pkg.github.com/CyclopsMC/packages/org/cyclops/commoncapabilities/commoncapabilities/1.19.2-2.9.0-88/commoncapabilities-1.19.2-2.9.0-88.pom'. Received status code 401 from server: Unauthorized

(deepest nested exception) All of that doesn't sound to me like it's just a GIT operation that would solve it, but I don't know anything.

If I'm not mistaking, I believe that's because the github action doesn't have an auth key.

kirjorjos · 2025-05-18T12:32:25Z

Rebasing on master may be required as well, as there seem to be merge conflicts.

I agree that github is acting weird with it as it isn't even seeing I changed half the files I changed looking now, I haven't really heard of rebasing before, but I'll do some research and give it a try.

fixed crash with squeezing empty facade by adding check for a null facadeBlockItem, outputs blank facade only now

accidentally pushed a local utility script

Reset file when rebasing

… Recipe[Serializer]Squeeze[Mechanical]

Added a static method in RecipeFacadeSqueeze that both class's assemble methods' call

Update JSON file when rebasing

kirjorjos · 2025-05-18T14:03:37Z

Rebasing on master may be required as well, as there seem to be merge conflicts.

I agree that github is acting weird with it as it isn't even seeing I changed half the files I changed looking now, I haven't really heard of rebasing before, but I'll do some research and give it a try.

I'm pretty sure I managed to rebase, but I guess if I did something wrong with it, I can try some more.

kirjorjos · 2025-05-18T16:43:47Z

Playing around in game, I am noticing a bug that this PR brings to light: If the facaded item has NBT, that NBT is lost when making the facade. I think the best way to resolve this would be storing the NBT with the facade, maybe something like {"block": {"Name": "mod_name:item_name", "tag": {...}}} as opposed to the current {"block": {"Name": "mod_name:item_name"}}? My only thought against this is that it would then cause any pre-exisiting facades to not be able to stack with new ones. I could probably add some extra logic to only have the "tag" key if there is data to fix that, but I wanted to run it by you and get your thoughts before doing much work with it. A good example item to test with is an InDy battery with some energy stored.

src/main/resources/data/integrateddynamics/recipes/special/facade_clear.json

rubensworks · 2025-05-19T04:58:16Z

Playing around in game, I am noticing a bug that this PR brings to light

This issue existed before this PR already, right?
If so, let's just create a separate issue for this.
It shouldn't be included in this PR IMO.

Besides my other comment above, this should be good to go.
So if it's ok from your end, I will test this in-game, and merge.

kirjorjos · 2025-05-19T05:49:28Z

Playing around in game, I am noticing a bug that this PR brings to light

This issue existed before this PR already, right? If so, let's just create a separate issue for this. It shouldn't be included in this PR IMO.

Besides my other comment above, this should be good to go. So if it's ok from your end, I will test this in-game, and merge.

It did exist before this PR, but it wasn't particularly an issue before this PR as there was no normal instance where the player would see something related to the NBT. Assuming you still want to make a new issue for this though, I'm good to let you test and merge.

If I'm not mistaking, it should be Empty Facade -> Empty Facade, Filled Facade -> Empty Facade, Facaded Item on both squeezers and Empty Facade -> Empty Facade, Filled Facade -> Filled Facade in a shapeless crafting grid.

rubensworks · 2025-05-20T14:49:15Z

The Empty Facade -> Empty Facade on both is probably not what we'd want.
Especially for the mechanical squeezer it's a bit counter-intuitive that this is allowed, since it's a no-op, but it will consume energy.
Maybe you could have a look at disallowing that?
After that, I will have a look myself in-game.

kirjorjos · 2025-05-20T15:14:46Z

I can explicitly disable empty -> empty if you want, the empty -> empty is more of a byproduct of allowing any data facade -> empty plus the block if there is one to also handle the case for invalid data. (Eg. If a mod got removed so the block that used to be facaded doesn't exist)

rubensworks · 2025-05-20T15:35:39Z

Removing invalid data makes sense indeed. But if the facade is really just empty, I think we should disallow it.

kirjorjos · 2025-05-20T16:12:35Z

Unless I'm missing something, it looks like I would need to update the [Mechanical]Squeezer block entity class as matches is only called on world load with a generic facade. Do you have any particular place you think would make sense to put it or you want me to just try somewhere where it works?

rubensworks · 2025-05-22T15:00:53Z

as matches is only called on world load with a generic facade

Are you sure? That's not how recipes are handled based on my recollection.
I thought recipes are always iterated over every time something is crafted (which is why we have a cache in place to optimize these loops).

Do you have any particular place you think would make sense to put it or you want me to just try somewhere where it works?

The match method should be a good place indeed.
I wouldn't put it in the block entity class in any case.

kirjorjos · 2025-05-22T15:21:52Z

I had thought i remembered matches being called on every run, but it didn't seem to be called when testing with a changed matches, so I launched in debug mode, set a break point, and confirmed it's not called once the world is loaded.

rubensworks · 2025-05-22T15:27:02Z

but it didn't seem to be called when testing with a changed matches

Probably because of the cache.

kirjorjos · 2025-05-22T16:01:48Z

Testing some more, it seems you are right that cache is making my tests behave weirdly. To confirm, the cache should reset when I leave the world and rejoin?

kirjorjos · 2025-05-22T16:29:10Z

I'm thinking I must be missing something as from a complete MC restart, after loading into the world, before trying any recipes (so the cache should be empty), I'm debugging matched on FacadeSqueeze, it returns false, but still gets processed. I also tried using the mechanical squeezer from a fresh MC restart and can't seem to catch inside the matches call in SqueezeFacadeMechanical to see why it's not working.

kirjorjos · 2025-05-23T06:14:15Z

Testing some more, it seems like it's only calling the matches method in RecipeSqueezer and RecipeMechanicalSqueezer, and unless I'm missing something (very possible), it looks to me like the logic for which method to call is inside Cyclopscore?

rubensworks · 2025-05-23T14:34:50Z

To confirm, the cache should reset when I leave the world and rejoin?

Not sure actually. It will reset in any case on a game restart.

it seems like it's only calling the matches method in RecipeSqueezer and RecipeMechanicalSqueezer, and unless I'm missing something (very possible)

I don't remember the precise mechanics myself tbh. I'd have to dive into the code myself.

kirjorjos · 2025-05-23T21:51:22Z

I guess for right now, do you want me to push what I'm 99% sure should work with a patched cyclopscore, or did you want to look at it first or...? Sorry, I'm just unsure what I should be doing here right now.

rubensworks · 2025-05-25T04:36:41Z

I suspect not changes should be needed in CC. But if you think there must be, feel free to open a PR.

rubensworks requested changes Jan 1, 2025

View reviewed changes

kirjorjos force-pushed the squeeze-facade-recipes branch from c82a47f to 390704c Compare January 1, 2025 23:31

kirjorjos requested a review from rubensworks March 2, 2025 05:40

rubensworks requested changes Mar 2, 2025

View reviewed changes