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.

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

Inventory Locks show up twice in Tinkers' Construct GUI #9

Closed
Darkosto opened this issue Apr 11, 2022 · 10 comments · Fixed by SlimeKnights/TinkersConstruct#4990
Closed
Assignees
Labels
bug Something isn't working

Comments

@Darkosto
Copy link

Describe the bug
When using certain Tinkers' Construct GUIs the locks will appear more than once (they are doubled):

image

After speaking with the Tinkers' dev, he suspects the locked slots are hardcoded location in all UIs, whether it has slots or not. If you iterate the player inventory slots instead of drawing at a constant location it should work. The GUIs in question are actually using 3 sets of inventory slots: left, middle, and right

To Reproduce
Steps to reproduce the behavior:

  1. Open up Tinker's Station or Tinker's Anvil
  2. Observe inventory slot locks are now doubled

Expected behavior
To show only 1 set of locked slots

Versions (please complete the following information):

I am currently using the mod in 1.18.2, but this has been an issue since 1.18.1 so you can use that version to test also.

  • Forge version: 40.0.45
  • Mod Version: 3.1.0
@Darkosto Darkosto added the bug Something isn't working label Apr 11, 2022
@kirderf1
Copy link
Owner

The slot positions are not hardcoded, and very much depends on the menu and positions obtained through the gui.

for(Slot slot : screen.getMenu().slots)

The icons are drawn on through this widget component, which is added to every container screen on initialization.
It seems like tinkers screen is made modular with multiple screens combined into one. For some reason, they all get the same container, meaning that you'll find normal slots for all of them if you check screen.getMenu().
For example, when checking this out in debug mode, the overlay was drawn for TinkerStationScreen and TinkerStationButtonsScreen. I'd expect only the former to have a menu with slots, but instead both seem to have the same menu, so both were rendered.

My initial thought is that tinkers' construct should use some dummy container menu for their module screens such as TinkerStationButtonsScreen. However, I can't really say for sure without knowing the reason behind how they designed their screens.
@KnightMiner do you have any insight on this?

@KnightMiner
Copy link

KnightMiner commented Apr 11, 2022

I have no idea the reason behind the screens honestly, all our UI code is pretty old and I am not the maintainer for it, it really needs a rewrite (but I hate rendering enough its hard to motivate myself to rewrite it)

That said, don't screens have a method to get slots so you don't have to get them from the container? Otherwise we would be rendering the inventory items in all our different components. I would suggest checking which slot list is used to print the inventory items in the base UI code and using the same list.

@kirderf1
Copy link
Owner

Nope, slots are gotten from the container as far as I can see. Rather, slots are handled in functions such as screen.render() and screen.mouseClicked(), and the parent screen for these module screens does not appear to call these functions anywhere.

Instead, it makes calls to functions defined in ModuleScreen. Some of these functions are overridden as is, while others redirect to screen functions such as to screen.renderBg() or screen.renderLabels(), but none to screen.render() or screen.mouseClicked().

In addition, widgets are normally rendered in the screen.render() function, which is why this issue didn't affect for example InfoPanelScreen, even if it has the same issue with its container. The reason that this issue shows up for TinkerStationButtonsScreen is that its superclass SideButtonsScreen overrides screen.renderBg() to draw all widgets in that function.

I'm now very much of the impression that these module screens need to be better integrated with regular screen functionality, in addition to using more representative containers. (if they even need to be container screens at all!)

@kirderf1
Copy link
Owner

I am in the process of making contributions to Tinkers' construct targeting 1.18.2, that should fix the issue.

@Darkosto Darkosto moved this to Waiting on Dev Additions/Fixes in Journey 2 the Core May 6, 2022
@TrevorA02
Copy link

I just want to let you know that this bug occurs in 1.16.5. I don't know if you're planning a fix for 1.16.5, but again, wanted to let you know.

@kirderf1
Copy link
Owner

kirderf1 commented Aug 4, 2022

The bug comes from an improper implementation of the gui side bars in tinkers construct. I'm in the process of undoing it for the latest version of tinkers construct (currently at a standstill as I'm waiting for feedback on SlimeKnights/TinkersConstruct#4841). Because of this, I will not attempt to fix it for older minecraft versions.

@Darkosto
Copy link
Author

Hey @kirderf1

Knightminer sent me the tcon jar file with the recent PR that you setup for them. Is that designed to be a complete fix or does it still require some more work to remove the extra locks in the inventory? Currently, the extra locks are still appearing for me:
image

Thanks for all the hard work you've put into this so far!

@kirderf1
Copy link
Owner

There are currently two PRs that you might be talking about, SlimeKnights/TinkersConstruct#4988 and SlimeKnights/TinkersConstruct#4990. Only the latter fixes this specific issue. Maybe the jar was made from the first of the two PRs?

@kirderf1
Copy link
Owner

Actually nevermind, it's not from the first of those two PRs. It was probably SlimeKnights/TinkersConstruct#4841. While this PR itself is not recent, it was merged recently.

@kirderf1
Copy link
Owner

The fix to this issue has now been merged into SlimeKnights/TinkersConstruct, and should be present in its next release.

Repository owner moved this from Waiting on Dev Additions/Fixes to Done in Journey 2 the Core Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants