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

Add clarification tooltips to blocks in the multiblock preview #1584

Merged
merged 11 commits into from
Jul 13, 2021

Conversation

ALongStringOfNumbers
Copy link
Collaborator

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:
java_2021-04-25_23-10-11

java_2021-04-25_23-41-45

Possible compatibility issue:
None expected

@warjort
Copy link
Contributor

warjort commented Apr 26, 2021

This type of code doesn't look correct to me.
The hatches and buses are not necessarily implementations from gregtech/common

          MetaTileEntity mte = GregTechAPI.META_TILE_ENTITY_REGISTRY.getObjectById(tooltipBlockStack.getItemDamage());
            if(mte instanceof MetaTileEntityItemBus || mte instanceof MetaTileEntityFluidHatch) {
                tooltip.add(1, TextFormatting.WHITE + I18n.format(infoPage.getBusTooltip()));
            }

You should instead be checking something like:

if (mte instanceof IMultiBlockAbilityPart) {
    MultiBlockAbility ability = ((IMultiBlockAbilityPart) mte)).getAbility();
    if (ability == MultiBlockAbility.EXPORT_ITEMS || etc.) 
}

@warjort
Copy link
Contributor

warjort commented Apr 26, 2021

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
that introduces tool tips from the real structure info/recipe map,

e.g. how many hatches/buses you really need.
e.g.2 the minimum casings

pseudo code:

MultiblockAbility ability = ...;
if (controller instanceof RecipeMultiBlockController) {
   addMinTooltip(controller.recipeMap, ability));
   addMaxTooltip(controller.recipeMap, ability);
}

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?

@warjort
Copy link
Contributor

warjort commented Apr 26, 2021

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

public MultiblockControllerBase {

    public boolean isSimpleConstraints() {
        return false; // Override on multiblocks that don't have complicated constraints
    }
}

This would give the JEI plugin a hint that it can bypass the full constraint system and use the simpler source constraints.

@ALongStringOfNumbers
Copy link
Collaborator Author

ALongStringOfNumbers commented Apr 30, 2021

I have changed this PR to instead accept a Map<MultiblockAbility, List<String>> so that tooltips can instead be added to any MultiblockAbilityPart, and in addition, multiple tooltips can be added to the same part.

This is an example of multiple tooltips being added to the same part:
java_2021-04-30_15-41-35

The "Invalid Structure" in this image was just a test of the multiple tooltips.

@warjort
Copy link
Contributor

warjort commented May 1, 2021

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?
Probably not super important though.

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);
Copy link
Contributor

@warjort warjort May 2, 2021

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

@LAGIdiot LAGIdiot added integration: JEI rsr: revision Release size requirements: Revision subsystem: gui type: feature New feature or request labels May 4, 2021
@LAGIdiot
Copy link
Member

LAGIdiot commented May 8, 2021

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.

@warjort
Copy link
Contributor

warjort commented Jun 4, 2021

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

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.

@LAGIdiot
Copy link
Member

LAGIdiot commented Jun 5, 2021

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.
@ALongStringOfNumbers can you please try implementing that suggestion?

@ALongStringOfNumbers
Copy link
Collaborator Author

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 ItemStacks instead of MultiblockAbilitys. This is so that tooltips can be added to non-multiblock parts to more easily declare part limitations for the multiblocks.

For example, when using MultiblockAbilitys, tooltips would not be able to be applied to the Heat proof casings on the EBF, which would be something that we want to do to convey the minimum part requirement of 10 for the casings.

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 MultiblockAbilitys.

I am putting this comment here for some feedback.

@ALongStringOfNumbers
Copy link
Collaborator Author

Does anyone know if there is a way to apply TextFormatting to TextComponentTranslatable without having the TextFormatting codes in the lang entry? I wanted to switch the Tuple<String, TextFormatting> to a TextComponentTranslatable to take advantage of the substitutions in the lang file entry (So that I could create a universal tooltip for minimum casing limit, instead of having one per multiblock), however, I was not able to find a way to apply formatting when the formatting is not in the lang entry in format codes.

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.

@Archengius
Copy link
Member

You need to use setStyle and then apply color inside of the Style object

@warjort
Copy link
Contributor

warjort commented Jul 11, 2021

Does anyone know if there is a way to apply TextFormatting to TextComponentTranslatable without having the TextFormatting codes in the lang entry?

Do you mean?

new TextComponentTranslatable("translation.key").getStyle().setColor(TextFormatting.GREEN);

Using ITextComponent instead of that Tuple looks like a much better api.

*/
protected void addAbilityTooltip(ItemStack itemStack, Tuple<String, TextFormatting> tooltip) {

List<Tuple<String, TextFormatting>> tooltipList = this.abilityTooltips.getOrDefault(itemStack, null);
Copy link
Contributor

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) {
Copy link
Contributor

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

Copy link
Contributor

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;
	}

@ALongStringOfNumbers
Copy link
Collaborator Author

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.

@ALongStringOfNumbers
Copy link
Collaborator Author

ALongStringOfNumbers commented Jul 12, 2021

I have now added some tooltips for GTCE multiblocks, beyond the default tooltips saying that any tier hatch/bus can be used.

Some examples are:
java_knx7kLTW6u

java_jikpEZNYEN

java_KR3mmkyJ84

As well as tooltips for the minimum number of casings required to form the structure:
java_AX8QqovUro

As this information is not currently obtainable from the Pattern Builder, the limits are set manually for each structure.

@ALongStringOfNumbers ALongStringOfNumbers changed the title Add clarification tooltip to buses and hatches in the multiblock preview Add clarification tooltips to blocks in the multiblock preview Jul 12, 2021
@serenibyss serenibyss merged commit 68dec27 into GregTechCE:master Jul 13, 2021
@ALongStringOfNumbers ALongStringOfNumbers deleted the MBtooltip branch July 25, 2021 21:14
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.

None yet

6 participants