-
Notifications
You must be signed in to change notification settings - Fork 151
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
Add clarification tooltips to blocks in the multiblock preview #1584
Add clarification tooltips to blocks in the multiblock preview #1584
Conversation
This type of code doesn't look correct to me.
You should instead be checking something like:
|
I don't want to introduce feature creep, but it would be good if you implement this in a way thinks about a possible future PR e.g. how many hatches/buses you really need. pseudo code:
I don't think you can actually implement it that simplistically since these recipemap constraints are actually hidden behind predicates and might not be the real constraint? |
Just spitballing; Absent some additional plumbing to expose the real constraints, you could hack it by adding something like
This would give the JEI plugin a hint that it can bypass the full constraint system and use the simpler source constraints. |
src/main/java/gregtech/integration/jei/multiblock/MultiblockInfoRecipeWrapper.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/integration/jei/multiblock/MultiblockInfoPage.java
Outdated
Show resolved
Hide resolved
I also think that tooltip map could be cached in the recipe setup. It does not need to be dynamic. I believe the way you are doing it, it will be creating that map on every rendering tick? |
tooltipList.add("gregtech.multiblock.preview.any_hatch"); | ||
abilityTooltipMap.put(MultiblockAbility.EXPORT_ITEMS, tooltipList); | ||
abilityTooltipMap.put(MultiblockAbility.IMPORT_ITEMS, tooltipList); | ||
abilityTooltipMap.put(MultiblockAbility.EXPORT_FLUIDS, tooltipList); |
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 are sharing the same list between the abilities?
So my previous attempt to add a tooltip to the export items would also be added to the others.
How about implementing it this way?
// Cached version
private Map<MultiblockAbility, List<String>> abilityTooltips;
public Map<MultiblockAbility, List<String>> getAbilityTooltipMap() {
// Generate on first use
if (this.abilityTooltips == null) {
this.abilityTooltips = Maps.newHashMap();
generateAbilityTooltips();
}
return this.abilityToolTips;
}
// Helper method
protected void addAbilityTooltip(MulitblockAbility ability, String tooltip) {
List<String> tooltips = this.abilityToolTips.putIfAbsent(ability, Lists.newArrayList());
tooltips.add(tooltip);
}
// The default implementation
protected void generateAbilityTooltips() {
addAbilityToolTip(MultiblockAbility.EXPORT_ITEMS, "gregtech.multiblock.preview.any_hatch");
addAbilityToolTip(MultiblockAbility.IMPOTY_ITEMS, "gregtech.multiblock.preview.any_hatch");
addAbilityToolTip(MultiblockAbility.EXPORT_FLUIDS, "gregtech.multiblock.preview.any_hatch");
addAbilityToolTip(MultiblockAbility.IMPORT_FLUIDS, "gregtech.multiblock.preview.any_hatch");
}
This is easier to use (harder to get wrong) and would make my subclassing example less verbose
@Override
protected void generateAbilityTooltips() {
super.generateAbilityTooltips();
addAbilityToolTip(MultiblockAbility.EXPORT_ITEMS, "my.tooltip");
}
It also adds caching on first use.
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.
There is still a problem with the way you are proposing warjot : you can't override existing tooltip without rewriting the main par of generateAbilityTooltips()
I think a removeTooltip(MulitblockAbility ability, String tooltip)
method might be useful.
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 don't think that remove is very useful?
You can remove tooltips you know about, but later additions to the default processing that you don't know about at development time might still be wrong.
If you really are overridding behaviour such that default tooltips are wrong, you probably shouldn't be calling super.
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.
well i think the multiblock which require a minimal or very specific tier for some hatches may want to use super for other hatches or in case the default tooltips added changes. I think the steam multiblock from gregicality could be an exemple.
This looks like good idea but I would like to open here discussion how we can make this to show minimal tier for tier dependent MultiblockParts like rotors, energy outputs and so on. As that would significantly enchant this. |
src/main/java/gregtech/integration/jei/multiblock/MultiblockInfoRecipeWrapper.java
Outdated
Show resolved
Hide resolved
The way I see it, is that each JEI MB definition can override the generateToolTips() to add tips that are specific to its usage. In future, if the opaque predicate problem is solved, these tool tips could instead be generated from the real MB pattern and recipe map automatically instead of having to define them in the JEI pattern. |
I really like @warjort suggestion regarding implementation and I think that is the way to go forth on this. With possibility to override (and possible not call at all) default values we could make this work for anyone. |
9ebcb1f
to
6e1e5e2
Compare
I have done the refactoring of how the tooltips were created and stored as suggested above. However, now I am thinking that I should store tooltips for For example, when using Switching to Itemstacks would also make it easier for tooltips to be applied to more blocks in custom multiblocks created by MultiblockTweaker, as blocks in those structures have less of a chance of being I am putting this comment here for some feedback. |
Does anyone know if there is a way to apply Switching the formatting to being in lang codes instead of in the Tuple is not that big of a deal, I just wanted to see if anyone knew how to apply formatting outside of the localization entry. |
You need to use setStyle and then apply color inside of the Style object |
Do you mean?
Using ITextComponent instead of that Tuple looks like a much better api. |
src/main/java/gregtech/integration/jei/multiblock/MultiblockInfoPage.java
Outdated
Show resolved
Hide resolved
src/main/java/gregtech/integration/jei/multiblock/MultiblockInfoPage.java
Outdated
Show resolved
Hide resolved
*/ | ||
protected void addAbilityTooltip(ItemStack itemStack, Tuple<String, TextFormatting> tooltip) { | ||
|
||
List<Tuple<String, TextFormatting>> tooltipList = this.abilityTooltips.getOrDefault(itemStack, 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.
Why default "null" ? Looks weird :-)
*/ | ||
public Map<ItemStack, List<Tuple<String, TextFormatting>>> getAbilityTooltipMap() { | ||
// Generate on first use | ||
if (this.abilityTooltips.size() == 0) { |
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.
isEmpty() is usually better - but since you are using an array list it won't make much difference here.
e.g. somebody could change it to use a LinkedList or something where size() is not a trivial operation
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.
Actually, this is an Object2ObjectOpenCustomHashMap which ironically does:
public boolean isEmpty() {
return size() == 0;
}
I have switched to using an ITextComponent for the second map parameter and right now am working on some default tooltips for multiblocks in GTCE. |
What:
Adds a clarification tooltip to item buses and fluid hatches in the multiblock preview and the component list specifying that the multiblock structure can be formed with any tier bus/hatch.
How solved:
Added a new default tooltip to
MultiblockInfoPage
. It was done this way for addon mods, which implement the assembly line that only uses ULV tier buses.The implementation of the tooltips themselves was tacked onto the existing tooltips for the preview, and a new method was created for the components list
Outcome:
Adds a clarification tooltip to buses/hatches in the multiblock preview
Additional info:

Possible compatibility issue:
None expected