-
Notifications
You must be signed in to change notification settings - Fork 178
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 Filters and some Covers to MUI + Filter Rework #2345
Port Filters and some Covers to MUI + Filter Rework #2345
Conversation
6107e02
to
8a79b8b
Compare
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.
Noticed the following issue with tooltips while in creative mode with an item on the cursor and having moved the filter popup window over to the side. Seems like it would be mainly a MUI issue, but I thought I would bring it up here
ItemStacks in Filters in Robot Arms need to have a tooltip added to them explaining the various ways to increment/decrement the count of the itemstack in the filter. IE: Key Combo + Scroll, or Key Combo + Click
And example from the fluid filter:
Since you added the ability to configure filters when the are in your inventory and keeping NBT, you need to add shapeless NBT clearing recipes for all the filters
You can currently crash trying to put an itemstack into the fluid slots of a fluid filter
Currently you can shift right click to clear the filter NBT when it is in your inventory. This needs a tooltip.
And yeah, as talked about on discord, I think we should move button state in these reworked covers to match that of the machine controller
You are currently making GUIs both the old way and the new way for a bunch of the filters that were converted. Any reason you are keeping the old implementations around?
You are currently limited to 1 bucket per slot maximum in a fluid filter, when configuring it in inventory. Any reason for this?
src/main/java/gregtech/common/covers/filter/BaseFilterContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/covers/filter/FluidFilterContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/items/behaviors/filter/BaseFilterUIManager.java
Outdated
Show resolved
Hide resolved
I've switched around the background textures for the enum rows, so it should look better now |
Found a voiding issue: Give yourself a stack of filters, right click to configure, and it sets the stacksize to 1 Also, should you be able to have duplicate itemstacks in filters? I don't think it would serve any purpose |
should be fixed now with 2f200cb |
fixed with 5cb80df I also noticed that the lang for fluid filter configuration has shift left/right-click to double/halve, but MUI does not do this behavior by default, should I add that behavior back for item and fluid filter slots? |
That one I am on the fence about, since you can key combo + scroll. I think it could be left out for now maybe |
I'm not totally sure about the appearance of the fluid voiding overlays from |
i consider it the default behavior of a fluid filter, as that's the behavior for a fluid filter in a pump. I could probably make it not render the fluid amount unless the fluid filter is in a regulator or advanced fluid voiding cover. |
Phantom fluid slots have a |
i'm using that now with 5fb69a4, thanks |
src/main/java/gregtech/common/covers/CoverFluidVoidingAdvanced.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/gui/widget/orefilter/ItemOreFilterTestSlot.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/gui/widget/orefilter/OreFilterTestSlot.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/integration/theoneprobe/provider/CoverInfoProvider.java
Show resolved
Hide resolved
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.
An in progress review
src/main/java/gregtech/common/covers/filter/readers/BaseFilterReader.java
Show resolved
Hide resolved
src/main/java/gregtech/common/covers/filter/readers/BaseFilterReader.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
public void setMaxTransferRate(int transferRate) { | ||
transferRate = MathHelper.clamp(transferRate, 1, Integer.MAX_VALUE); |
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 min for transfer rate should be 0 right? Not 1? I remember that currently you can set transfer rates to 0.
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 set the max transfer rate (max stack size for items) to 0, but the normal transfer rate is clamped to a min of 1
src/main/java/gregtech/common/covers/filter/readers/BaseFilterReader.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/covers/filter/readers/BaseFilterReader.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/covers/filter/BaseFilterContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/covers/filter/BaseFilterContainer.java
Outdated
Show resolved
Hide resolved
Summary of latest changes
|
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 Robot Arm has the text for "Conveyor Mode" next to its Import and Output mode selection. This should probably be renamed to something more general.
Found an error with the Robot arm and item filters. Place an item filter in a Robot Arm in Transfer Any mode. Change the Mode to Keep Exact, and scrolling to change the item amount is not displayed until the filter is closed and re-opened.
What is the difference between the max stack size and the transfer stack size in BaseFilterContainer?
src/main/java/gregtech/common/items/behaviors/filter/BaseFilterUIManager.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/items/behaviors/filter/BaseFilterUIManager.java
Show resolved
Hide resolved
src/main/java/gregtech/common/covers/filter/OreDictionaryItemFilter.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
public void onFilterInstanceChange() { | ||
dirtyNotifiable.markAsDirty(); |
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.
Shouldn't this also run the onFilterInstanceChange
?
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.
could do that, though we never set the variable ourselves it seems
src/main/java/gregtech/common/covers/filter/BaseFilterContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/covers/filter/BaseFilterContainer.java
Outdated
Show resolved
Hide resolved
your other comments should also be addressed now |
2975608
to
63dcf5e
Compare
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.
Was testing the Ore Dictionary filter, and the Case Sensitive stuff didn't seem to work. Was testing a gold ingot, so ingotGold
and typing in gold
into the bar matched the ingot in both case sensitive and insensitive modes. Figured out that toggling the case sensitivity option does not refresh the displayed match status. This might also happen with the match all button, but I did not test that.
Kind of an ask, but you should be able to hold backspace in the Ore Dict Filter text bar.
The amount incrementing/decrementing tooltips in an item filter should really only be shown in cases where you can change the amount, like the filter being in a robot arm. Currently they are shown when just opening the filter by itself, which is a bit confusing because none of it applies.
Opening an Ore Dictionary filter, doing nothing, and then closing the GUI gives the filter an empty NBT tag. Could this be avoided at all?
There still needs to be an icon for the filter button when in a cover (like the conveyor)
Dupe Found: Place a macerator, place a Conveyor on top (used UV), place a crate on top (Used TungstenSteel), add a stack of crates to the crate, change the mode of the conveyor to import, and it will dupe the stack of crates, 1 stack in the macerator and 1 stack remaining in the crate.
And afterwords I ran the macerator, clearing some space in the input, and the conveyor would not pull in the remaining stack of crates from the crate. However, other items added to the crate would be pulled in, so I am not sure if this is an actual dupe, or just a big desync, although I am leaning towards dupe.
src/main/java/gregtech/common/gui/widget/HighlightedTextField.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/mui/widget/orefilter/OreFilterTestSlot.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/common/mui/widget/orefilter/OreFilterTestSlot.java
Show resolved
Hide resolved
this should be fixed now (for both case sensitive and match all buttons)
this is already what happens, at least with mui 2.4.3
are you talking about the "Count: #" tooltip? i could probably move that to only robot arm where it's most relevant
it's a consequence of calling
i cannot replicate this. i do remember that the crate's UI was not syncing to the client properly in the past, but i'm not seeing that anymore either |
this should be fixed now |
Ah, I see. I can fix that for the item filters, but MUI adds the tooltip for the phantom fluid slots. In order to change that I would have to override the slot's class or rebuild the tooltip from scratch. |
68968ce
to
f66cf3b
Compare
spotless
move draw calls to a more appropriate method
do `string, string` function instead
remove legacy tag when loaded remove some methods from IFilter
yeet IItemFilter.java and IFluidFilter.java test/match against Object and cast appropriately remove MatchResult's generics move similar methods into BaseFilterContainer
4f51192
to
a0534e7
Compare
f4d946f
to
6335a05
Compare
Even with the most recent commit, whitelist/blacklist modes are not preserved on existing filters as covers when loading into a world with filters created before switching to this branch. |
From testing transferring old worlds with old filters, into new worlds, I found the following issues:
|
fix missing overlays/buttons
i'm pretty sure most if not all of the legacy nbt reading is handled correctly now
both of these are fixed now
bucket mode previously was not saved (iirc) for pumps/regulators. i'll try to save it now
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.
Looks mostly good, but this PR is getting a bit too unwieldy. We should merge this now, and have a followup PR for any required bugfixes
What
Rework Filter Behavior
remove
FilterTypeRegistry
Port some Covers to MUI
Implementation Details
port filters to mui
make filters open a gui on rightclick
remove
FilterTypeRegistry
create the filter behavior as a metaitem component
filter settings are now stored/read from item NBT
deprecate old mui methods
add method
slice()
toGTGuiTextures
for slicing an image into an arraymake
CoverWithUI::createTitleRow()
staticfix buttons not being adaptable enough
Outcome
Currently, the covers ported are:
As a result of the changes to filter UI and behavior, some covers that accept filters don't work correctly:
Todo
this may be updated in the future
Potential Compatibility Issues
At the moment, some method that previously existed or had functionality may be commented out or removed. If this will cause issues, let me know. There might also some parts where NBT saved in older worlds is not read to the new filter system