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

Fix glitches for SlotWidgets inside ScrollableListWidgets #1558

Merged
merged 3 commits into from
Apr 19, 2021

Conversation

warjort
Copy link
Contributor

@warjort warjort commented Apr 5, 2021

What:
Trying to use a slot widget inside a scrollable list widget leads to a number of glitchy behaviours.

The main cause of this is that the slots are NOT drawn inside the normal drawInBackground() handling where the ScrollableListWidget applies a scissor. Instead they are drawn "out of band" in ModularUIGui.

How solved:

  1. When a ScrollableListWidget determines the positions of its components and its size, it now visits each of its widgets and gives them the option to remember its area that is actually visible.
    The SlotWidget does this. It exposes this area through a new interace "gregtech.api.gui.IScissored" on its slot delegate.
    When the ModularUIGui displays the slots, it checks for this new interface and uses it as a scissor to determine how much (if any) of the slot should be drawn.
    The end result is that ModularUIGui can now do the same scissor handling for slots that would normally be done in drawInBackground if the slots behaved like other widgets.

  2. The ScrollableListWidget now checks if a widget is within its visible area when it checks if a widget can be clicked.
    Previously it just used the AbstractWidgetGroup handling which assumes all sub widgets were visible if the group is.

  3. A number of places use Slot.isEnabled() to check for behaviours, like showing tooltips and some behaviours internal to SlotWidget itself. SlotWidget.isEnabled was not taking into account whether it was actually visible inside a ScrollableListWidget.
    SlotWidget.isEnabled() has been changed to use the scissor from (1) when determining if it is enabled.

Outcome:
Putting a slot widget inside a ScrollableListWidget is not as glitchy as it was.
I can't guarantee I have spotted all bugs in this area.

Additional info:
The workbench (crafting station) has a ScrollableListWidget that contains slots, but notably it doesn't use the SlotWidget.
It has its own implementation. I wonder if that is because of the issues I have fixed here?

Possible compatibility issue:
All of these changes except one only affect SlotWidgets inside ScrollableListWidgets. I don't think anybody else has done this, otherwise they would have seen these issues.

The one exception is the change to ScrollableListWidget.isWidgetClickable(Widget) which affects all subwidgets.
The fix seems correct even for non slot widgets, in that you shouldn't be able to click widgets outside the bounds of the
visible scroll area.

@warjort warjort mentioned this pull request Apr 6, 2021
@LAGIdiot
Copy link
Member

Thanks for spotting and fixing this issue. I have idea what you did but just be sure that we are on same page can you please provide some pictures or gif of problems which this fixes.

@LAGIdiot LAGIdiot added rsr: revision Release size requirements: Revision subsystem: render type: bug Something isn't working subsystem: gui labels Apr 13, 2021
@warjort
Copy link
Contributor Author

warjort commented Apr 13, 2021

These are from my addon mod.
If you want to try yourself, change the CoverKeepInStock to have more than 6 slots (my current workaround is to disable the scrolling).

Here you can see the slots leaking outside the scroll area.

2021-04-13_20 30 05

This second image is hard to see (the highlighted white area), but these are clickable slots over the player's inventory.

2021-04-13_20 29 44

@warjort
Copy link
Contributor Author

warjort commented Apr 13, 2021

I have been testing it by making a modpack with this PR and a version of my addon with more than 6 slots.

Copy link
Member

@LAGIdiot LAGIdiot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good I had correct idea what it was. Code seems solid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rsr: revision Release size requirements: Revision status: accepted subsystem: gui subsystem: render type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants