Skip to content

Conversation

@ALongStringOfNumbers
Copy link
Collaborator

@ALongStringOfNumbers ALongStringOfNumbers commented Apr 25, 2021

What:
This PR transfers the overlapping multiblock description on the JEI display page into a specific JEI info page, so that the description does not make it harder to see the structure.

In addition, information has been added about the various ways that the multiblock preview can be manipulated.

Also, this should be the final remaining point from #1089, so that might be able to be closed after.

How solved:
Removes the description drawing from the drawText method in MultiblockInfoRecipeWrapper and moves it to GTJEIPlugin, making an accessor for InfoPage in the process.

Outcome:
Moves multiblock descriptions to a JEI info page and adds an informational hover.

Additional info:
Current:
javaw_2021-04-25_12-17-44

In PR:
java_2021-05-22_13-00-01

Hover:
java_2021-05-22_13-54-28

java_2021-04-22_00-04-31

Also still works for localization:
java_2021-04-22_00-14-38

Possible compatibility issue:
Addons that don't already override MultiblockInfoPage#getDescription will need to do so now.

@ALongStringOfNumbers ALongStringOfNumbers marked this pull request as ready for review April 26, 2021 08:11
@warjort
Copy link
Contributor

warjort commented Apr 26, 2021

 GTUtility.drawCenteredSizedText(recipeWidth / 2, -FONT_HEIGHT, localizedName, 0x333333, 1.3);

Its not a good idea to use these negative coords. This is what it looks like for me with "auto" gui scale.

2021-04-26_13 49 17

@warjort
Copy link
Contributor

warjort commented Apr 26, 2021

Maybe you could add a dummy "I" (for information) widget next to the L:A button that has the mouse controls as a tooltip?

@LAGIdiot
Copy link
Member

LAGIdiot commented May 4, 2021

I think it would be better to add some widget which will on hover show these preview manipulation choices instead putting them up there.

@ALongStringOfNumbers
Copy link
Collaborator Author

I am working on a widget/button that will be able to display the information.

@ALongStringOfNumbers
Copy link
Collaborator Author

With my most recent commit I have removed the information from the top, which was put there by scaling in the negative y, and moved instead to an informational hover icon, which will display information in a tooltip when hovering over the icon. I have updated my original post with new images

@ALongStringOfNumbers
Copy link
Collaborator Author

ALongStringOfNumbers commented May 6, 2021

As a note, I fixed the somewhat dim look on the information hover, see the original post for the faded version, and see here for the non faded version:
java_2021-05-05_21-18-51

This was done by changing some calls in WorldSceneRenderer to avoid calling some methods that Forge had marked as do not use, because they did not accept the PR to fix the issue. However, this change made the button highlighting bleed into the information hover, as mentioned in #1452. I am looking into how to fix the button highlight bleed now.

@warjort
Copy link
Contributor

warjort commented May 7, 2021

This was done by changing some calls in WorldSceneRenderer to avoid calling some methods that Forge had marked as do not use, because they did not accept the PR to fix the issue

Did you commit those changes? I don't see them.

@ALongStringOfNumbers
Copy link
Collaborator Author

Did you commit those changes? I don't see them.

I have not committed the changes because I have not found out what is causing the button bleed that comes with the changes. The specific changes though were moving away from GLSTM.popAttributes and GLSM.pushAttributes to call GL11.glPopAttributes and GL11.glPushAttributes

@LAGIdiot
Copy link
Member

I don't have knowledge regarding fixing rendering issues so I can't offer my help here. But if anyone has any idea we would appreciate it because I don't want to put it in with render issue.

@LAGIdiot LAGIdiot added the status: help needed Extra attention is needed label May 15, 2021
@ALongStringOfNumbers
Copy link
Collaborator Author

Just as clarification, the button highlight bleed does not occur in the current version of the PR, it is just the somewhat dim information hover.

The highlight occurs again after I made the changes as stated above, which is also what makes the information hover not as dim. I have not committed these changes though because I did not find out what was causing the button highlight issue.

@ALongStringOfNumbers
Copy link
Collaborator Author

The last set of commits have fixed the button highlight bleeding issue that I was seeing and demonstrated above. That should finalize this PR and make it ready for review again. I would be happy for feedback, especially on my previous two commits fixing the button highlighting.

@LAGIdiot
Copy link
Member

Maybe we mention all possible keys to manipulate that scene. Like: RMB to drag, Mouse wheel to reset. I don't see any rendering issues expect for that dim problem witch is making it unusable in middle of night. And we need definitely to fix it.

Strange thing is it somehow follow day/night cycle and it is not correctly shown on screenshots.

@LAGIdiot
Copy link
Member

LAGIdiot commented May 22, 2021

I did some quick testing and these two calls to minecraft.entityRenderer.enableLightmap() is what Fs whole GUI. When I removed them GUI behaved correctly, except for:

  • info button that was dim
  • dimming buttons under on hover - fixed by rendering hover after buttons (last)
  • highlighting problem on button hover - fixed by rendering them after all active elements (Thanks for comment in code @warjort)

@LAGIdiot
Copy link
Member

Eureka! Info button is not dim anymore

@LAGIdiot
Copy link
Member

I have made PR to fix rendering problems: ALongStringOfNumbers#3

@LAGIdiot LAGIdiot added rsr: minor Release size requirements: Minor and removed status: help needed Extra attention is needed labels May 22, 2021
@LAGIdiot
Copy link
Member

This needs to be released as minor because we will stop showing description for our addons till they put it in info page.

@ALongStringOfNumbers
Copy link
Collaborator Author

I have pulled in the PR to fix the rendering dimming and to cleanup minor things, and have updated the first post to display everything at the current state of the PR.

@ALongStringOfNumbers
Copy link
Collaborator Author

With that last commit, I added a bit more information to the hovering text box, as mentioned in a previous comment, and have updated the image in the original post

@LAGIdiot
Copy link
Member

LAGIdiot commented May 23, 2021

Thanks for your hard work!

Now this should be reviewed by someone other then me because I worked on it.

Copy link
Collaborator

@Exaxxion Exaxxion left a comment

Choose a reason for hiding this comment

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

Code looks good to me with the latest revisions. Testing in game I'm not seeing any graphical quirks like highlight bleed that were previously occurring. The description is showing up on the JEI info category for uses of the multiblock controller like one would expect for each multiblock.

It certainly seems usable in its current state, though if possible it would be great if we could eventually figure out how to not have that large empty gap at the top of the category page.

As an aside, I think there are some typos or outdated information in the description strings of some of these multiblocks but those are outside the purview of this PR.

@warjort
Copy link
Contributor

warjort commented Jun 1, 2021

though if possible it would be great if we could eventually figure out how to not have that large empty gap at the top of the category page.

I had a look at it before and concluded it would be pretty hacky.

JEI does a calculation based on the screen height (depends on gui scale) and a config parameter called maxRecipeGuiHeight.
And then it basically pads the top and bottom so your recipe layout is centred within that common height for all recipes.

If you read the config parameter from JEI and got the screen height from minecraft you could repeat the calculation and then choose a background height for your recipe layout that fills it.

But the gui scale and JEI parameter can be changed at runtime, breaking your calculation.

@LAGIdiot
Copy link
Member

LAGIdiot commented Jun 5, 2021

As discussed I will merge this to master now so Addons have more time to adjust changes, later this will be part of next release which will be Minor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants