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
[1.12.x] Updated GuiTabs PR #4389
Conversation
import net.minecraft.client.renderer.OpenGlHelper; | ||
import net.minecraft.client.renderer.RenderHelper; | ||
import net.minecraft.client.renderer.texture.TextureAtlasSprite; | ||
-import net.minecraft.client.renderer.texture.TextureMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't change imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uhm import net.minecraft.client.renderer.texture.TextureMap;
appears as not used import
EDIT: oh right its a MC patch
+ | ||
import org.lwjgl.input.Keyboard; | ||
|
||
+import com.google.common.collect.Sets; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't change imports.
* @param name The name of the tab | ||
* @param icon The icon | ||
*/ | ||
@SideOnly(Side.CLIENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the annotation again once the class is annotated.
*/ | ||
public static List<GuiTab> getTabsForGui(Class<? extends GuiContainer> guiContainer) | ||
{ | ||
return tabRegistry.containsKey(guiContainer) ? tabRegistry.get(guiContainer) : new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer Collections.emptyList()
rather than new ArrayList<>()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is meant to be modifiable, you should simply use get
int size = getTabsForGui(guiContainer.getClass()).size(); | ||
if (size > 0) | ||
{ | ||
for (int i = 0; size > i; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use an iterator or a stream for this instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that readable enough ?
if (size > 0)
{
IntStream.range(0, size).filter(i ->
!getTabsForGui(targetGui).contains(getTabsForGui(guiContainer.getClass()).get(i))
).forEach(i ->
getTabsForGui(guiContainer.getClass()).get(i).addTo(targetGui)
);
}
tabs.addAll(GuiTab.getTabsForGui(this.guiContainer.getClass())); | ||
maxPages = setMaxPages(); | ||
getTabs(); | ||
for (GuiTab tab : getTabs())// Set the first page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any point to call getTabs()
twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, noticed this as well.. removed
this.guiContainer = guiContainer; | ||
this.guiContainerClass = guiContainer.getClass(); | ||
tabs = new ArrayList<>(); // get a copy of the stored list so it can be modified without consequence | ||
tabs.addAll(GuiTab.getTabsForGui(this.guiContainer.getClass())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tabs = new ArrayList<>(GuiTab.getTabsForGui(this.guiContainer.getClass()));
int end = Math.min(getTabs().size(), (page + 1) * maxTabsPerPage); | ||
|
||
for (GuiTab tab : getTabs()) | ||
{ // < end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is that comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No clue
invS = new InventoryBasic("Test", true, 27); | ||
switch (ID) | ||
{ | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This indentation is weird...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shrug used: https://github.com/ForgeEssentials/ForgeEssentials/blob/develop/misc/intellijformat.xml
but I guess that switch is redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is fine. It is a test mod, which no one really cares.
} | ||
|
||
maxPages = setMaxPages(); | ||
for (GuiTab tab : getTabs())// Set the first page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I replace this with:
page = getTabs().stream().filter(tab -> tab.isActiveTab(guiContainerClass)).findFirst().map(GuiTab::getTabPage).orElse(page);
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it's fine as-is
@Override public boolean isActiveTab(Class<? extends GuiContainer> guiContainer) | ||
{ | ||
return super.isActiveTab(guiContainer); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this override
private Class<? extends GuiContainer> parentContainer; | ||
private Class<? extends GuiContainer> targetGui; | ||
|
||
public static final GuiTab VANILLA_INVENTORY_TAB = new GuiTab("default_inventory", new ItemStack(Items.SKULL, 1, 3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the skull be user-aware?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but not sure what would be a proper way to do that
ItemStack skull = new ItemStack(Items.SKULL, 1, 3); | ||
skull.setTagCompound(new NBTTagCompound()); | ||
NBTTagCompound nbttagcompound = new NBTTagCompound(); | ||
NBTUtil.writeGameProfile(nbttagcompound, Minecraft.getMinecraft().getSession().getProfile()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the player skull (with the Player's skin) is only visible in Single Player.
But when joining a server the skull gets replaced with the default (Steve/Alex Skin).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about the skull of the client player?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why's it have to be the player's head, can't it just be the default steve head?
it's cool and all but if there's problems with it it's not really worth dealing with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boy, it is not that hard... You can get a skull of a certain player fairly easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean like, the "cool" look will be when the player plays on single player. but when being on a server the default skin (steve/alex) will display. I guess its not that important ?
Also liach, would love to know how I could do that :)
{ | ||
if (parentContainer.equals(GuiContainerCreative.class)) | ||
{ | ||
FMLLog.log.warn("Cannot add tabs to creative inventory. Use CreativeTabs instead"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should throw an exception instead of logging.
} | ||
else if (parentContainer.equals(InventoryEffectRenderer.class)) | ||
{ | ||
FMLLog.log.warn(name + " tab has a parent that is ambitious with the creative inventory. cannot proceed with registry."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also throw an exception.
ambitious
-> ambiguous
?
*/ | ||
public GuiTab addTo(Class<? extends GuiContainer> parentContainer) | ||
{ | ||
if (parentContainer.equals(GuiContainerCreative.class)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for .equals
here, ==
should be fine.
* | ||
* @author Subaraki, Modified by Ash Indigo | ||
*/ | ||
public class GuiTabList |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more than a "List of GuiTabs", isn't it? Is there perhaps a more suitable name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class is responsible for holding a list of GuiTabs and rendering, so I would suggest GuiTabMenu.
FMLLog.log.warn("Cannot add tabs to creative inventory. Use CreativeTabs instead"); | ||
return this; | ||
} | ||
else if (parentContainer.equals(InventoryEffectRenderer.class)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.equals
-> ==
The lighting "issues" are from a forge test mod that i pr'ed, #4127 |
ah so its fine then ? |
Yeah, the lighting issues are from the test mod, put any other block in your inventory. |
Perhaps the collection of tabs should be purely event based instead? There could be situations where you need to conditionally add a tab, or add a variable amount of tabs. You could use |
@@ -95,6 +99,17 @@ public void onTabClicked(GuiContainer guiContainer) | |||
GuiTabsTest.INSTANCE.sendToServer(new TabMessageHandler.TabMessage(0)); | |||
} | |||
}.addTo(GuiInventory.class); | |||
ItemStack is = new ItemStack(Items.APPLE); | |||
is.addEnchantment(new net.minecraft.enchantment.EnchantmentProtection(Enchantment.Rarity.COMMON, EnchantmentProtection.Type.FALL), 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use Enchantments.PROTECTION?
You are instantiating a new Enchantment
object (like Item
, Block
, or Potion
) without registering it.
{ | ||
ctx.getServerHandler().player.openGui(GuiTabsTest.instance, message.id, ctx.getServerHandler().player.world, | ||
ctx.getServerHandler().player.chunkCoordX, ctx.getServerHandler().player.chunkCoordY, | ||
ctx.getServerHandler().player.chunkCoordZ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are the player's chunk coords (world coords / 16). is there any reason they're being used? if the gui handler doesn't need them just pass zeroes instead, it's cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, until today I thought they were like a requirement, (I know its stupid but I thought it might need the exact location of the player to open the gui). replaced with 0s
A question/suggestion: Should there be "default" tabs for the vanilla containers? (Like Creative tabs) EDIT: Those tabs could be placed inside like... VanillaGuiTabs or something like that |
Important poll vote http://www.strawpoll.me/13927765 |
By the total votes so far, I've decided to make some default tabs for some vanilla containers: |
I still like the idea of tab groups which can be added. Could be useful for e.g. charging blocks, where all "equipment" tabs are accessible (for ease of charging). |
I think it would be simplest if you just used the chest icon for the default tab, like the "survival inventory" tab does in the Creative GUI. |
True, but ugh, tbh wouldn't it be better even if MC used the player's skull for the inventory instead of the chest in Creative too? |
The chest icon could mean a different gui (a chest). What happens if someone adds a "portable chest"? Making it a player head reduces confusion |
Glad to see this is being continued. |
// get a copy of the stored list so it can be modified without consequence | ||
this.guiContainerClass = attachable.getMainGuiContainer().getClass(); | ||
List<GuiTab> tabsForGui = GuiTab.getTabsForGui(this.guiContainer.getClass()); | ||
GuiTab defaultTab; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the default gui be converted into a private field and have a private setter and a public getter ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you just use the vanilla string-keys instead of using your own? That way it would automatically be translated already.
*/ | ||
public String getLocalizedName() | ||
{ | ||
return "guiGroup." + name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why "guiGroup" why not "guiTab" or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so recommend to change this to guiTab
?
I used guiGroup as in reference of the creative tabs, but a change can be made
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use guiGroup.
.
Hey, I'm really confused about the "main gui automation" progress/mechanics... so here is a Poll, please vote on it... and if there are any questions and things I can do to make it work, ping me on discord sokratis12GR#2764 |
I think there should be some sort of default that is used if the modder does not implement the interface themselves. Where it would get the icon from I don't know. |
Well, I guess that could be achieved if the mod author or someone creates an library/utility mod adding the default tab for the main mod so that other mod authors could add/create addons without conflicts. |
This has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
/** | ||
* Dictates how many tabs per page are drawn | ||
*/ | ||
public final static int MAX_TABS_PER_PAGE = 12; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static final
* Dictates how many tabs per page are drawn | ||
*/ | ||
public final static int MAX_TABS_PER_PAGE = 12; | ||
private int page; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currentPage
?
*/ | ||
private Class<? extends GuiContainer> guiContainerClass; | ||
private List<GuiTab> tabs; | ||
private static final ResourceLocation TEXTURE_TABS = new ResourceLocation("textures/gui/container/creative_inventory/tabs.png"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put this constant with the max page numbers?
* | ||
* @author Subaraki, Modified by Ash Indigo | ||
*/ | ||
public class GuiTabMenu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be extended? If yes, you may consider leaving some fields protected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure. If so, which fields would be best to be protected ?
* Adds 25 tabs to the player inventory and 13 to a test gui Also checks if item stack icon and ResourceLocation icons are working | ||
*/ | ||
@Mod(name = "GuiTabsTest", modid = "guitabtest", version = "0.0.0.0") | ||
@EventBusSubscriber() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need parentheses
private static ListMultimap<Class<? extends GuiContainer>, GuiTab> tabRegistry = ArrayListMultimap.create(); | ||
|
||
private ItemStack iconStack = ItemStack.EMPTY; | ||
private ResourceLocation iconResLoc = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nullable
private String name; | ||
private Class<? extends GuiContainer> parentContainer; | ||
private Class<? extends GuiContainer> targetGui; | ||
private static Minecraft mc = Minecraft.getMinecraft(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should make fields protected since they are actively accessed in both this and the subclasses.
private Class<? extends GuiContainer> targetGui; | ||
private static Minecraft mc = Minecraft.getMinecraft(); | ||
|
||
public static final GuiTab VANILLA_INVENTORY_TAB = new GuiTab(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about moving these mess into another utility class to make things neater?
* | ||
* @param name The name of the tab | ||
*/ | ||
private GuiTab(String name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you move them into another class, you can make this package private so that mods cannot use it.
Seems that commits are somewhat messed up |
Dunno what happened. Tho I've asked about this at Mezz's Discord Server, I've been told that it should be alright (Shouldn't have any impact other than the PR's GuiTabs). |
The file changes here include things you never edited, it needs to be fixed. |
Ugh, for the time being please ignore my commits, trying to solve things out and will make as you said a completely new PR from a branch and not master. |
I'm no git expert, but I'd say: Don't merge the origin changes into your branch but rebase your branch onto the the origin branch. |
This. Merging in a PR usually causes nothing but problems. |
So the actual difference is here: I agree that github is a little bit fuzzy here. |
BTW, I think I found what was causing the weird recipe book thing with the crafting table container. |
After some discussion in Mezz's discord server. There was a decision to not include automatic generation of default gui tabs. If a mod, gui container wants to have a default tab, they should register the gui tab inside the gui container's constructor. |
I'd love to see this pr come to a merge in forge ... ! |
Oh, another thing I would like to note, is should the methods be renamed according to: ModCoderPack/MCPBot-Issues#565 ? fields: methods: |
@sokratis12GR I think you can name the methods whatever you want. Those name changes might be applied at any time, and does not affect the compiling of forge. |
@liach, I edited my comment, but really would like to know if I should do the changes, for like the main ones |
Will create a new PR (fixing the Commit history). |
This PR has been closed in favor of #4654. |
Updated version of #3642
Modified the code to work with 1.12.x changes
Fixed the java classes having 2 license headers
Removed an useless condition inside
GuiTabsMenu.addButtons
And a few minor changes to use
+=
and-=
where possibleImproved their functionality
Removed redundant code
Introduced Default Tabs:
DefaultVanillaGuiTabs.class
)A few refactors (name changes):
GuiTabList.class
->GuiTabsMenu.class
getLocalizedName
->getUnlocalizedName
(thats what it always was getting, users should translate their names)Location of the GuiTabs has been moved from
player.inventory.tabs
->gui.tabs
Any suggestions for improvements are welcome
Known bugs (fixed):
Credits to @ArtixAllMighty for making the original PR and @AshIndigo for updating it to 1.11.x