-
Notifications
You must be signed in to change notification settings - Fork 180
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
Port/Rework Crafting Station to MUI2 #2455
base: master
Are you sure you want to change the base?
Port/Rework Crafting Station to MUI2 #2455
Conversation
270a020
to
85a5c84
Compare
To summarize a few things I found in an in-game review that would be good to have before I go all-in with reviewing 19 files:
|
f528df5
to
07a2377
Compare
2c0f220
to
f4e1615
Compare
9914d3b
to
6160fd7
Compare
d88f038
to
1558a94
Compare
3350a30
to
f542e0b
Compare
@@ -58,7 +58,7 @@ static ItemStackHashStrategy comparingItemDamageCount() { | |||
*/ | |||
class ItemStackHashStrategyBuilder { | |||
|
|||
private boolean item, count, damage, tag; | |||
private boolean item, count, damage, tag, meta; |
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 damage not enough?
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.
iirc because of GT Tools or something like that, i made this change a long time ago.
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.
Shift clicking the output slot of the crafting station does not do anything, when you have enough items in the inventory to craft multiple of the output.
When viewing the contents of connected inventories in the inventory tab, the contents are not sorted so that all the actual items are visible at the top of the tab. Instead items are separated by any empty inventory spaces remaining in one of the inventories before showing other existing items.
this is 2 chests, 1 on either side of the station.
Crafting a recipe, locking the recipe in the memorized tab, and then crafting the recipe again removes the lock status in the memorized recipe tab.
There should be a way to clear the existing recipe displayed in the crafting slots widget, so you don't have to clock all the slots one by one. A little x
or something in the top next to the grid.
I somehow managed to reset the count of the items crafted. Not sure how. I had crafted 16 LV motors, left the GUI, came back, crafted some other stuff, and the times crafted tooltip only showed 8.
There is some delay with the scroll bar in the item source window that can really be felt. If you scroll down and then immediately scroll back up, nothing happens on the scroll back up. You have to reach the bottom/top of the scroll bar distance for this to happen, if you change directions in the middle it feels ok.
The scroll bar for the item source tab is too close to the slots in the window, and cuts off the item counts of things in the right most slots.
The item source window should get some special handling for the quantum chest I believe, because right now is shows the virtualized items and the items in the output slot. It would be better if it showed a combined total of the two.
When the crafting station is checking if it can complete a recipe, it only checks its internal inventory and the connected inventories. Do we want to check the players inventory as well?
src/main/java/gregtech/common/inventory/handlers/SingleItemStackHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/mui/widget/workbench/RecipeMemorySlot.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/mui/widget/workbench/RecipeMemorySlot.java
Show resolved
Hide resolved
@Override | ||
public void drawForeground(ModularGuiContext context) { | ||
RichTooltip tooltip = getTooltip(); | ||
if (tooltip != null && isHoveringFor(tooltip.getShowUpTimer())) { |
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 are we using tooltip.getShowUpTimer()
? We never set a time value for the hovering amount, and the default time is 0 ticks.
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 mimics mui2's item slot tooltip draw
src/main/java/gregtech/common/mui/widget/workbench/CraftingOutputSlot.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/mui/widget/workbench/CraftingOutputSlot.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/integration/jei/JustEnoughItemsModule.java
Outdated
Show resolved
Hide resolved
@@ -42,6 +42,7 @@ dependencies { | |||
api("codechicken:codechickenlib:3.2.3.358") | |||
api("com.cleanroommc:modularui:2.5.0-rc2") { transitive = false } | |||
api("com.cleanroommc:groovyscript:1.2.0-hotfix1") { transitive = false } | |||
api("curse.maven:inventory-bogosorter-632327:4951607-deobf-4951608-sources-4951609") |
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 is added as a dep, but nothing is done with it in this PR that I can see, do we still need it?
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 is required due to ModularContainer implementing a bogosorter interface, which is loaded from GregTechGuiTransferHandler
use networkutils methods
fix removing recipe on client
it should craft the item once (at least it does for me)
uh, not sure how that could happen, and i don't think i'm reproducing it.
this is a mui2 issue i think, would need to look into it more
that's the grid default, but i might be able to change it
i don't think previous logic checked the player inventory, but i could look into adding that |
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.
The crafting station doesn't handle special crafting recipe with fluid containers. EG, you can use a drum of creosote to craft treated wood in a normal crafting table, but this is not recognized in the crafting station.
Some weird highlighting going on with the crafting grid when I placed in ghost ingredients by hand. Started in the bottom right, and then added items counter clockwise. I think it failed on the stick recipe when I added the two items into the grid, but then did not update the tint as I added more items into the grid. Yeah, testing this with jungle planks, it forms the stick recipe after placing in two planks, but then the tint does not disappear after invalidating the stick recipe by placing a third plank in the grid.
Found a way to void items. When crafting a lot of an item, such that the internal inventory fills up, continueing to shift click craft the item will void the output, instead of placing it in the players inventory or dropping it on the ground if the players inventory is full (I don't remember if vanilla drops the items on the ground if the players inventory is full, or just prevents more crafts). I was testing this with the Invar Wrench recipe, and this only happened when shift clicking the craft.
No tool crafting sounds are played when crafting items with GT tools in the crafting station. This does not apply to the breaking sound though, that works fine.
Found some weird slot tinting issue, where some slots are tinted darker than other slots. I had the recipe for Circuit Boards in the table, and then shift clicked in the recipe for iron plates from JEI, and two of the slots were tinted darker than they should be for empty slots.
For the memorized recipe grid, while it is not full, new crafts are added to the beginning of the list and displayed in the top right slot. However, after the grid is filled up new recipes are added to the bottom right slot. Previously new recipes would always be added to the top left slot, and if the grid was full the bottom right slot would be deleted, with the slot next to it taking its place. This is to always have the most recent crafted recipe not immediately be pushed off the grid if you craft another new recipe.
move used memorized recipe to the front
these recipes work fine for me
this is fixed.
fixed
i'm actually not sure how to get the tool crafting sound
couldn't reproduce
i couldn't reproduce this, but it might be fixed now
this is fixed now |
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.
The implemented dragging ghost ingredients from JEI doesn't really work. You can get the ghost ingredient onto your cursor, but when you click the grid with the item, nothing happens.
Durability render for tools in the crafting matrix is offcenter.
Can't click items from the output slot if there are items on the players cursor, even if the items match. You might want to look at the previous implementation of the various button clicks on the output slot, and see what all edge cases they solved.
Cannot use R/U JEI buttons on items in the memorized recipe widget
You can have both a locked and nonlocked memorized recipe of the same output (Tested with invar plates). If there is a locked recipe already, a new one should not be added, especially if it is the same recipe.
This may be hard, but right now, if the internal inventory is full, shift clicking a recipe will not complete the recipe, even if all the inputs would be consumed, thus freeing a slot in the internal inventory. Maybe it would be possible to calculate if a slot would be freed after using all the inputs, and if so craft the item, depositing the output into the now vacant input slot?
src/main/java/gregtech/common/gui/widget/craftingstation/CraftingSlotWidget.java
Show resolved
Hide resolved
public IWidget createCraftingOutput(PosGuiData guiData, PanelSyncManager syncManager) { | ||
var amountCrafted = new IntSyncValue(this::getItemsCrafted, this::setItemsCrafted); | ||
syncManager.syncValue("amount_crafted", amountCrafted); | ||
amountCrafted.updateCacheFromSource(true); // todo remove |
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.
remove?
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 is to work around an issue with int sync values where they don't set their cache on construction, there's a mixin in #2672 but it's also fixed in mui2 rc3
src/main/java/gregtech/common/metatileentities/storage/CraftingRecipeLogic.java
Show resolved
Hide resolved
fix crafting with same item in hand
clarify todo
What
the fabled pr that ports the
crashingCrafting Station to cleanroom mui, and also reworks the crafting station logic.i now believe it is feature complete and ready for review
Implementation Details
rework crafting station logic
port crafting station to mui2
Todo/Notes
Outcome
crafting station will continue to exist