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

Recipe tooltip fixes #402

Merged
merged 13 commits into from Jul 23, 2023
Merged

Recipe tooltip fixes #402

merged 13 commits into from Jul 23, 2023

Conversation

eigenraven
Copy link
Member

@eigenraven eigenraven commented Jul 15, 2023

Fix bad lighting in recipe tooltips and sometimes blank tooltips for handlers that expect the current GUI screen to be a GuiRecipe.

Fixes GTNewHorizons/GT-New-Horizons-Modpack#13954

@eigenraven eigenraven requested a review from a team July 15, 2023 07:37
@eigenraven eigenraven self-assigned this Jul 15, 2023
@Dream-Master Dream-Master requested a review from a team July 15, 2023 07:44
eigenraven and others added 2 commits July 15, 2023 09:26
Handlers might call Minecraft#displayGuiScreen and overwrite Minecraft.currentScreen, but the height hack would unconditionally undo this again.
@wlhlm
Copy link
Member

wlhlm commented Jul 16, 2023

For posterity: this fixes an issue reported on Discord where the nei-custom-diagram handlers in tooltip widgets would only draw over the main GuiScreen area:
2023-07-15_02 52 12

I have pushed an additional commit to fix GTNewHorizons/GT-New-Horizons-Modpack#13954. The root cause was that a recipe handler (in this case nei-custom-diagram) might modify Minecraft.currentScreen (for example via Minecraft#displayGuiScreen) while the hack was applies, but this would be undone again when close is called.

However, there are actually multiple possible ways to fix these issues. The purpose of the height hack originally was to make certain legacy recipe handlers compatible with the layout changes (as I understand it). Considering that enabling it required explicit opt-in through heighthackhandlers.cfg, adding parts to it that apply even outside of the isHeightHackApplied check changes this and we should record our reasoning for this change.

The override for Minecraft.currentScreen was added in #398, because certain legacy handlers expected Minecraft.currentScreen instanceof GuiRecipe. For example, here is a snippet from IC2:

private void drawLiquidTooltip(FluidStack stack, int x, int recipe) {
    int y = 55;
    GuiRecipe gui = (GuiRecipe)Minecraft.getMinecraft().currentScreen;
    Point mouse = GuiDraw.getMousePosition();
    Point offset = gui.getRecipePosition(recipe);
    String tooltip = stack.getLocalizedName() + " (" + stack.amount + "mb)";
    GuiTooltipHelper.drawAreaTooltip(mouse.x - (gui.width - 176) / 2 - offset.x, mouse.y - (gui.height - 176) / 2 - offset.y, tooltip, x, 55, x + 12, 102);
}

The IC2 handlers are configured to have the height hack applied by default so those were fixed. However, it turns out that nei-custom-diagram also assumes a similar condition and breaks with the recipe tooltip widgets. The thing is. nei-custom-diagram is not a legacy handler, it was made with GTNH NEI in mind and didn't require the height hack previously. We could've fixed it in a number of ways:

  1. add nei-custom-diagram toheighthackhandlers.cfg
  2. fix the nei-custom-diagram code to not assume Minecraft.currentScreen instanceof GuiRecipe
  3. apply the currentScreen override unconditionally to all handlers

Here is how I see it: Why not add it to heighthackhandlers.cfg? Because it's not a really a legacy handler and if it has issues with our NEI, it should ideally be fixed in the handler. Why not fix it in nei-custom-diagram directly? IMO this should be the first option to be considered. If possible, we should fix the handlers themselves instead of applying workarounds through NEI. This might need more code. The question to ask here is whether Minecraft.currentScreen instanceof GuiRecipe is a reasonable thing to assume in a handler or if it is bad code. My gut feeling is that this is a reasonable assumption and tooltip recipe widgets breaking this assumption should be handled on NEI side. So, option 3 sounds reasonable.

The issue I see with option 3 is that it doesn't really fit with the purpose of "height hacking" (it's not meant to only apply for legacy handlers since it's unconditional). The only similarity is that it need to be applied through the same mechanism. As such I think the HeightHack class should be renamed (and the javadocs adjusted), since the height hack, i.e. overwriting guiTop and height when configured, will only be a part of the mechanism now.

What do you think @eigenraven? I hope this made sense.

@github-actions
Copy link

Warning: 2 uncommitted changes
#404

Co-authored-by: GitHub GTNH Actions <>
@eigenraven
Copy link
Member Author

@wlhlm I agree that this should be handled on the NEI side, does renaming HeightHack to CompatibilityHacks (only the class, not touching csv) seem reasonable?

@wlhlm
Copy link
Member

wlhlm commented Jul 16, 2023

@wlhlm I agree that this should be handled on the NEI side, does renaming HeightHack to CompatibilityHacks (only the class, not touching csv) seem reasonable?

Sounds good. Can you maybe move the code lines so that we have the height hack overrides together and then separately the currentScreen override. That is, move trueGui = NEIClientUtils.mc().currentScreen; below the height hack. Also, move the javadoc blurb about the height hack into the constructor closer to the height hack overrides.

@wlhlm
Copy link
Member

wlhlm commented Jul 16, 2023

Added a commit to fix overlay button offsets as reported on Discord:
image

While testing that I found a rendering bug with animations (not fixed):
2023-07-16_21 30 11

@wlhlm
Copy link
Member

wlhlm commented Jul 19, 2023

This should be ready for review now. PTAL!

@Dream-Master Dream-Master merged commit 8569431 into master Jul 23, 2023
1 check passed
@Dream-Master Dream-Master deleted the recipe-tooltip-fix branch July 23, 2023 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NEI Gregtech Pages Can Not be Interacted With
4 participants