NBT Editor

NBT Editor

30k Downloads

[Bug] NBT-Editor v2.0.2.999 "Tooltip Overflow Fix" is incompatible with Axiom

KadTheHunter opened this issue · 7 comments

commented

Using NBT Editor v2.0.2.999, the "Tooltip Overflow Fix" no longer scales down tooltips if Axiom is present.

Tested on 1.21.1 with NBT Editor v2.0.2.999, Axiom v4.2.0, Fabric API 0.107.0.

No errors are printed in the logs, and given Axiom is closed source I've no way to look at its code and see what might be wrong on their end.

Test file, a sword with tooltip overflow: https://files.catbox.moe/yxln7i.nbt
(catbox link because github doesn't allow uploading .nbt to issue comments :| )

Edit: If this turns out to be an issue on Axioms end, I'll wait until NBT Editor v2.0.2 releases and then report it to Axiom.

commented

The fact that there are no errors, Axiom is closed source, and the tooltip overflow fix is very straightforward with no other good way to do it, there is nothing that I can do

I would definitely report it to Axiom (if this also happens in 1.20.6, you could even report it now)

If it's helpful to Axiom, the tooltip overflow fix simply injects right after the matrices.push() line in DrawContext#renderTooltip and uses matrices.translate(...) and matrices.scale(...) to re-position it. Refer to DrawContextMixin for details

commented

Just checked and its an issue on 1.20.4 and 1.20.6 as well, presumably on all versions.

Currently waiting on access to their bug reporting channel, will report to them alongside the inject info as soon as I can, thanks for that.

commented

According to the Axiom dev, they forcefully disable NBT Editors tooltip overflow fix:

image

and won't fix it.

Personally I disagree with them, there is every reason for NBT Editor to have this fix, but their loss I guess.

Here is the MixinNbtEditorConfigScreen.java they sent at the bottom there, if it's of any interest:

package com.moulberry.axiom.mixin.compat;

import com.moulberry.mixinconstraints.annotations.IfModLoaded;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.Pseudo;
import org.spongepowered.asm.mixin.injection.At;
import org.spongepowered.asm.mixin.injection.Inject;
import org.spongepowered.asm.mixin.injection.callback.CallbackInfoReturnable;

@IfModLoaded("nbteditor")
@Pseudo
@Mixin(targets = "com.luneruniverse.minecraft.mod.nbteditor.screens.ConfigScreen", remap = false)
public class MixinNbtEditorConfigScreen {

    @Inject(method = "isTooltipOverflowFix", remap = false, at = @At("HEAD"), cancellable = true)
    private static void isTooltipOverflowFix(CallbackInfoReturnable<Boolean> cir) {
        cir.setReturnValue(false); // Disable
    }

}

Side note: This issue was apparently first reported in August 2023, when Axiom + NBT Editor would crash when hovering over axioms buttons, but I can't find the mentioned errors/logs, nor the issue that was created. The fact that more than a year later NBT Editor hasn't been added to Axioms conflict list, and that Moulberry more than once disparaged NBT Editor/NBT Editors code has made me greatly reconsider using it. Closed source was bad enough, but this just takes it further.

Anyway, closing this issue as it's being forced by Axiom, and unless I'm misreading it, even changing things on NBT Editors end wouldn't matter, given it's overruled by Axiom.

commented

This sounds very similar to #56, which was fixed a year ago in this commit: f685f7a
The getWindow() refers to the literal game window, not the current screen (which isn't accessed via a method, it's accessed via the field currentScreen when using Yarn mappings).

I can manually bypass their "fix", but I would rather they just deleted their mixin (or checked that NBT Editor's version is >=1.13 if they would like to maintain compatibility with older version of NBT Editor).

If Axiom would like context on why this fix exists and was mixin-ed in this way:

  • This was added in v1.12.0, which added the ability to import images as lore on items. When using large images, most of the lore is cut off. As a result, it made sense to add a way to see the images.
  • When adding the overflow fix, I specifically added the config option to maintain compatibility with any mods that also do something like this (for example, scrollable tooltips). Note that the config option check is the very first thing that happens, and I use @Inject instead of @Override - this ensures that disabling the config option will prevent nearly all mod conflicts. (This was also added for configurability.)
  • This mixins into the method that renders all tooltips, as its meant to fix all tooltips; while the feature is intended for item's lore, it could still be useful for other places. This also means that if a mod renders a custom tooltip for something, it will will be automatically fixed if needed (as long as the config option is enabled). I saw this as a benefit, not a flaw. Obviously, I didn't consider the case where mods would want to render tooltips without a screen, which is why I fixed this in v1.13 when this issue was reported to me.
commented

Ok then, reopening this, and forwarding to Axiom.

commented

In the future, I'll think more carefully about what config options I leave enabled by default. (At this point, though, I don't think I need to retroactively change it, since its problems seem to be fixed.)

I understand the frustration of telling people that they need to change their config, and I appreciate that you bothered to figure out the issue in the first place. However, I do ask that, in the future, you also let me know that there is a problem rather than simply applying a patch on your end (particularly since other mod developers could end up having to apply the same patch if I never hear about it).

When adding this, I did search for a mod that already does this first, and I found mod(s) that do what ToolTipFix does. The problem is that ToolTipFix wraps the text rather than scaling it - so the lore images would break.

commented

I'll remove the mixin if it's no longer necessary. This issue can probably be closed as there's nothing that NBT-Editor needs to change.

  • When adding the overflow fix, I specifically added the config option to maintain compatibility with any mods that also do something like this (for example, scrollable tooltips). Note that the config option check is the very first thing that happens, and I use @Inject instead of @Override - this ensures that disabling the config option will prevent nearly all mod conflicts. (This was also added for configurability.)

That would be true if the config option was disabled by default. Since it was enabled by default, it would cause a crash and then users would simply go and make bug reports/support posts to me and other mod developers since users would have no way of knowing it was actually caused by the NBT-Editor mod. Yes, other mod developers and I could go and tell every person who makes a support post to go into their config and disable it, but that's just frustrating and burdensome to deal with

This mixins into the method that renders all tooltips, as its meant to fix all tooltips

There are dedicated mods for this already: https://modrinth.com/mod/tooltipfix