-
Notifications
You must be signed in to change notification settings - Fork 9
[ref:vaan/feature/procedural-crafting] Procedural Crafting #476
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.
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
base: master
Are you sure you want to change the base?
Conversation
|
I have no idea what to put in the Lore |
|
depends on pylonmc/pylon-core#522 |
|
I feel like this is ready for review even if not fully complete, also from last testing it seems that on shutdown the entities aren't loaded back up properly, i have no idea what is causing it and it would be helpful if you guys had any idea |
LordIdra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general comments
- Instead of sending messages, we should give some visual indication. maybe particles of some kind? The only thing is they'd have to be per stage which is annoying
- Hammer sends 'cannot be used on this block' message - I assume this can be fixed later when PylonWorkbenchTool (or whatever its called) interface is introduced.
- Displays need to be yeeted when the craft is finished
I would suggest making all the recipes shapeless tbh. It feels more intuitive to me (there's no logical reason why the iron screws recipe should have to be in the top left at all - or even why there would ever be a fixed arrangement given it's a workbench where you're moving stuff around as you make it). It'll make automation 10x easier to implement as well. (It'll also make the code simpler)
I would suggest eventually replacing the workbench with a table display model + structure void. The working area feels awkwardly large, I think it only needs to be like 0.7x0.7
Instead of displaying the next step in the GUI, I would suggest using display entities instead so it's more immersive. Idea for this: above the model add the following (from top to bottom):
- Text display showing number of remaining uses
- Item display with the item that needs to be used next
- Small text display with the name of the item that needs to be used next
(All should be billboards so they face the player all the time)
I think this worbench should only have an input section in the UI, and the output should just be dropped in world. Then, we could introduce some kind of 'Blueprint Workbench' which holds a specific workbench recipe and has both an input/output. This would make full automation feasible. And also it would be consistent with other early-game machines as all of them drop stuff in-world
Btw you're probably going to need some kind of Z-index for displays because some might end up overlapping each other
have only very lightly skimmed the code since I imagine a lot will still change
src/main/java/io/github/pylonmc/pylon/base/content/machines/simple/AssemblyTable.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/pylonmc/pylon/base/content/machines/simple/AssemblyTable.java
Outdated
Show resolved
Hide resolved
at this point instead of nuking basically 90% of this PR should i just make he blueprint workbench instead of the assembly table? after all that is what I interpreted this as |
|
I think we should have both, worktable for manual, blueprint for automatic. |
I didn't say to drop one of them, just to focus on only one of the for the scope of this PR, and the blueprint seems to be the most fitting atm |
|
So i don't have to nuke the whole simplified recipe system i made, 2 inventories and their handlers, and more stuff, also there is still the bug that on restart the display mess up for some reason that i can't figure out the cause |
|
Ah ok. Sure that seems fine |
|
Ok renamed everything, but before going on into using entities for everything, I want someone to help me find and fix the bug that causes the entities spawned at restart to desync with the table |
|
Also extracting some common stuff between the assembly table and blueprint workbench into a separate class |
It is going to be annoying regardless, i don't want it to mess with action bars because most plugins already use it, nor waila, so message is the best thing, i don't want to mute my pc every time i am crafting a recipe with tons of steps either, message is the best thing imo, or maybe a low volume sound if you prefer, or we can have both, one of my ideas was adding an optional sound per step after all
Yeah i have no idea how we are going to do that though, what was your idea for PylonWorkbenchTool
It was a bug, has been fixed
Is this the same for the blueprint table? if you want you can commit how you want it to look like directly tbh
Added, it always faces north regardless which is a bit annoying, the display in the GUI is staying because tbh, it is convenient to not spin around the table sometimes, also it causes no harm keeping it Once this is done i can start work on the assembly table |
Why not particles as suggested? Message just seems really jank and will be extremely annoying in a multiplayer environment where you also want to see what other players are typing.
See the last message in the procedural crafting thread
I think yes blueprint table should also have a smaller area as it feels weird to have it this big imo. If you don't want to do it then I can do it in another pr, but tbh it's not really hard just fiddly and I don't have a clear picture for how it should look either.
This is why I said '(All should be billboards so they face the player all the time)'. Use the billboard setting. |
|
Yeah let's make the models in other PRs, didn't know what billboard was, will look for the other stuff later |
Seggan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall a good job, some critiques tho:
- I would prefer the workbench to not have a GUI at all, maybe place the inputs on top?
- Can I make the displays 3d?
- Item displays are a bit too high, all 3 don't fit on my screen
- Make the item displays only display to the players within n blocks (theres a method for that I think) so we won't expose secret bases
- I can't figure out how blueprint workbench works
- How does one do stuff like add a "sonic screwdriver" manual tool and then also have an "automatic sonic screwdriver"? By which I mean what methods do I call to perform a specific step in-code?
| ); | ||
|
|
||
| List<ItemStack> requiredItems = new ArrayList<>(currentRecipe.ingredients()); | ||
| ItemStack[] unsafeItems = craftInventory.getUnsafeItems(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldnt use unsafeItems imo but thats a nit
| return builder.build(); | ||
| } | ||
|
|
||
| public void handleGui(PlayerInteractEvent event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...why do you have a custom gui handler?
| } | ||
|
|
||
| public RecipeChoice getChoice(int x, int y) { | ||
| var character = this.rows[y].charAt(x); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dude was too lazy to type one extra letter
| } | ||
| } | ||
|
|
||
| public interface StepsHolder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have a separate holder class? seems to me like overcomplicating the API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in ProceduralCraftingTable abstract class
LordIdra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you decided to go ahead with the assembly table then? I see that the class exists and has code that seems to progress the recipe. If not I'd suggest maybe just remove it for now to simplify things.
| stack = current.asStack(current.uses() - currentProgress.getUsedAmount()); | ||
| } | ||
|
|
||
| this.stepDisplay.setItem(UpdateReason.SUPPRESSED, 0, stack); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is stepDisplay a virtual inventory? (Actually - maybe you don't know about invui's items? Have a read of the invui docs - https://docs.xenondevs.xyz/invui/ - using a custom item is a much nicer approach than having a virtual inventory)
| import static io.github.pylonmc.pylon.base.util.BaseUtils.baseKey; | ||
|
|
||
| @Getter | ||
| public abstract class ProceduralCraftingTable<T extends Step.StepsHolder & PylonRecipe> extends PylonBlock implements PylonEntityHolderBlock { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class seems rather overengineered, probably partially owing to the generic, which can be removed now. I would suggest just collapsing this into BlueprintWorkbench (assuming you're not adding AssemblyTable in this PR). If there is a good abstraction we can make it later.
| public ProceduralCraftingTable(@NotNull Block block, @NotNull BlockCreateContext context) { | ||
| super(block, context); | ||
|
|
||
| this.craftInventory = new VirtualInventory(9); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit of a nitpick but prefer craftingInventory here, as craftInventory implies NMS
| NamespacedKey currentRecipeKey = pdc.get(CURRENT_RECIPE_KEY, PylonSerializers.NAMESPACED_KEY); | ||
| this.currentRecipe = currentRecipeKey == null ? null : deserializeRecipeKey(currentRecipeKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use a KEYED.keyedTypeFrom serializer here - example from RecipeProcessorPersistentDataType:
val recipePDT = PylonSerializers.KEYED.keyedTypeFrom { recipeType.getRecipeOrThrow(it) }
It'll simplify a lot of the serde logic which is nice
|
|
||
| protected final VirtualInventory craftInventory; | ||
| protected T currentRecipe; | ||
| protected Step.ActionStep currentProgress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest instead of this class, you can just make this class a PylonProcessor to track the stage, and then add a variable to track the number of uses remaining in the current stage. Or just have two variables - one for the stage and one for the number of uses - if you don't want to use PylonProcessor.
| Step.StepsHolder.Result result = this.currentRecipe.progressRecipe(currentProgress, getStep(), item, player, shouldDamage); | ||
|
|
||
| switch (result) { | ||
| case NEXT_STEP, NEXT_PROGRESS -> updateStep(); | ||
| case COMPLETED_RECIPE -> completeRecipe(); | ||
| } | ||
|
|
||
| return result.isSuccess(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like this would be simpler if this.currentRecipe.progressRecipe was inlined rather than extracted out, then you wouldn't need to have StepsHolder.Result. (I can't find anywhere else where progressRecipe is used so not a case of DRY)
|
|
||
| protected static void clearInventory(VirtualInventory inventory) { | ||
| for (int i = 0; i < 9; i++) { | ||
| inventory.setItem(UpdateReason.SUPPRESSED, i, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For inventory.setItem calls you can now use new MachineUpdateReason() which I added recently. (Won't comment on the other calls)
| if (data.mirrorX()) { | ||
| addEntityIfMissing(name + "$mirror_x", () -> new BlockDisplayBuilder() | ||
| .material(data.material()) | ||
| .transformation(new TransformBuilder() | ||
| .translate(-positions[0], 0, positions[1]) | ||
| .scale(scale[0], 0, scale[1]) | ||
| ) | ||
| .build(up) | ||
| ); | ||
| } | ||
|
|
||
| if (data.mirrorZ()) { | ||
| addEntityIfMissing(name + "$mirror_z", () -> new BlockDisplayBuilder() | ||
| .material(data.material()) | ||
| .transformation(new TransformBuilder() | ||
| .translate(positions[0], 0, -positions[1]) | ||
| .scale(scale[0], 0, scale[1]) | ||
| ) | ||
| .build(up) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will display the correct behaviour if both X and Z are mirrored - in that case there should be 4 entities.
| import java.lang.reflect.Type; | ||
| import java.util.List; | ||
|
|
||
| public record DisplayData( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be called something a bit less generic, eg AssemblyTableDisplayData (DisplayData could mean almost anything)
Progress:
Stuff to decide:
Example usage