Hud screen edit#77
Conversation
|
I would like to keep the reset and move buttons accessible from the config gui screen |
|
if you want, I can add another button to the 'renderer edit screen' that when clicked will make all buttons invisible until a key is pressed or something like that |
|
I merged the updated |
| import java.lang.reflect.Field; | ||
| import java.lang.reflect.Method; | ||
|
|
||
| public class RendererGuiButton extends ConfigGuiButton { |
There was a problem hiding this comment.
I disagree with the changes in this class, please roll back your changes and keep the two buttons like it was before
There was a problem hiding this comment.
I understand but I very much disagree with this.
-
I am keeping the
Move HUD Buttonbut as shown in the image it is no longer an "Image Button" and its text isPosition.
If you like its text can beMove HUD. -
There is no reason for the
Reset Buttonto be placed here.
2.1. I am already creating such a button for use inside theHud Edit Screen.
2.2 It is bad placement. Players cannot see where the hud is once they press this button here, they have to press on theMove HUDaswell to see its new position.
There was a problem hiding this comment.
ok can you change the text to something else than "Position", "Move HUD" would be more explicit, also you might need to add a color to the palette for the text color
| private final RendererPosition rendererPosition; | ||
| private final GuiScreen parent; | ||
| private final ColorPalette colorPalette; | ||
| private final Button[] buttons; |
There was a problem hiding this comment.
don't make an array but create mutliple fields
| this.rendererPosition.updateAbsolutePosition(); | ||
| this.adjustBounds(); | ||
| final ScaledResolution res = new ScaledResolution(mc); | ||
| this.rendererPosition.updateAbsolutePosition(res); |
There was a problem hiding this comment.
mark deprecated the method this.rendererPosition.updateAbsolutePosition(); that is now not used anymore
|
|
||
| private GuiUtil() {} | ||
|
|
||
| public static final int MOUSE_LEFT = 0; |
There was a problem hiding this comment.
This field should be used throughtout the codebase instead of the magic value when comparing with mouseButton.
This is not just for this pull request but for further onces aswell.
There was a problem hiding this comment.
ok but I dislike it being here, just make it a local variable in the RendererEditGuiScreen
There was a problem hiding this comment.
for the ConfigGuiScreen I can add an MouseClick enum or something to remove the magic numbers
There was a problem hiding this comment.
I understand this and I will make it a local variable for now.
but think of it like this, many different classes check mouseButton:
ConfigGuiScreen, RendererEditGuiScreen, ColorEditScreen, ConfigGuiButtons, etc...
and it would be convinient and easy to read if this value was shared.
also for developers who create their own elements.
and there is no need to create an enum for this, this is simple enough and very understandable to me and I assume other devs.
later on you can also add MOUSE_RIGHT = 1 if you want
|
|
||
| private static final class Button extends ClickGuiButton { | ||
| private final ResourceLocation icon; | ||
| private final String hoveringText; |
There was a problem hiding this comment.
add the hovering text feature to the parrent class click gui button directly
There was a problem hiding this comment.
I will do this.
I also have an idea to add possible hovering text to ConfigUIElements like in my codebase.
I will show you what I mean in a different pull request but keep it in mind
| drawTexturedModalRect(this.width / 2 - 7, this.height / 2 - 7, 0, 0, 16, 16); | ||
| } | ||
|
|
||
| private static final class Button extends ClickGuiButton { |
There was a problem hiding this comment.
create a standalone class named TexturedButton that draws the icon
| if (clickedButtonIdx == 0) rendererPosition.resetToDefault(); | ||
| else if (clickedButtonIdx == 1) rendererPosition.setRelativePosition(originalRelativeX, originalRelativeY); | ||
| else break success; | ||
| final ScaledResolution res = new ScaledResolution(mc); |
There was a problem hiding this comment.
save the scaled resolution as a field that is initializd in initGui
| private int prevX, prevY; | ||
|
|
||
| public RendererEditGuiScreen(RendererManager rendererManager, IRenderer renderer, GuiScreen parent) { | ||
| public RendererEditGuiScreen(RendererManager rendererManager, IRenderer renderer, GuiScreen parent, ColorPalette colorPalette) { |
There was a problem hiding this comment.
don't need to add ColorPalette as a parameter just change the type of the parent parameter to ConfigGuiScreen
| } | ||
| } | ||
| if (clickedButtonIdx != -1) { | ||
| success: { |
There was a problem hiding this comment.
you shouldn't use labels and break and also this is bugged because if clickedButtonIdx is not 0 or not 1 it will loop infinitely
There was a problem hiding this comment.
you are mistaken, this is not how either works.
the loop I have is itterating over the buttons array, it only breaks early if I find a clicked button because i can only click on one button.
now the label after it is just for simplicity, because if clickedButtonIdx is either 0 or 1 I am updating the relative position and then need to update the absolute position, so what I am doing is updating the relative position, (see if (clickedButtonIdx == 0) and else if (clickedButtonIdx == 1))
and then updating the absolute position and returning from the function to not trigger isDragging.
(see code after else break success)
if clickedButtonIdx isn't 0 or 1 it will simply break from the success block and move on to isDragging logic. (see else break success)
There is nothing in this code that can cause an infinite loop and the only reason its written this way is because if I do decide to add more buttons I can write the logic easily at the end of the success block
There was a problem hiding this comment.
I understand your reason for not wanting to use labels so I wont. but you are mistaken in your image.
what happens when you break from a labeled-block is that you go to the end of the block.
what would actually happen in my code if clickButton == 2 is that the code will continue from }.
example:
int clickButton = 2;
success: {
if (clickButton == 0) {}
else if (clickButton == 1) {}
else break success;
log("0 or 1!");
return
}
log("not 0 or 1!");
if clickButton is 2, it will log: not 0 or 1!.
if clickButton is 0 or 1 it will log: 0 or 1!
shvaich
left a comment
There was a problem hiding this comment.
Agreed with some.
To what I didnt agree I commented.
Whatever you decide on after reading my replays i will do
| } | ||
| } | ||
| if (clickedButtonIdx != -1) { | ||
| success: { |
There was a problem hiding this comment.
you are mistaken, this is not how either works.
the loop I have is itterating over the buttons array, it only breaks early if I find a clicked button because i can only click on one button.
now the label after it is just for simplicity, because if clickedButtonIdx is either 0 or 1 I am updating the relative position and then need to update the absolute position, so what I am doing is updating the relative position, (see if (clickedButtonIdx == 0) and else if (clickedButtonIdx == 1))
and then updating the absolute position and returning from the function to not trigger isDragging.
(see code after else break success)
if clickedButtonIdx isn't 0 or 1 it will simply break from the success block and move on to isDragging logic. (see else break success)
There is nothing in this code that can cause an infinite loop and the only reason its written this way is because if I do decide to add more buttons I can write the logic easily at the end of the success block
|
|
||
| private GuiUtil() {} | ||
|
|
||
| public static final int MOUSE_LEFT = 0; |
There was a problem hiding this comment.
This field should be used throughtout the codebase instead of the magic value when comparing with mouseButton.
This is not just for this pull request but for further onces aswell.
| import java.lang.reflect.Field; | ||
| import java.lang.reflect.Method; | ||
|
|
||
| public class RendererGuiButton extends ConfigGuiButton { |
There was a problem hiding this comment.
I understand but I very much disagree with this.
-
I am keeping the
Move HUD Buttonbut as shown in the image it is no longer an "Image Button" and its text isPosition.
If you like its text can beMove HUD. -
There is no reason for the
Reset Buttonto be placed here.
2.1. I am already creating such a button for use inside theHud Edit Screen.
2.2 It is bad placement. Players cannot see where the hud is once they press this button here, they have to press on theMove HUDaswell to see its new position.
|
|
||
| private static final class Button extends ClickGuiButton { | ||
| private final ResourceLocation icon; | ||
| private final String hoveringText; |
There was a problem hiding this comment.
I will do this.
I also have an idea to add possible hovering text to ConfigUIElements like in my codebase.
I will show you what I mean in a different pull request but keep it in mind






made changes to the hud edit screen and the hud button.
also removed 2 icons and added 2 icons