Fabric: Attribute#mergeAttributes doesn't respect modifier UUIDs

[bluebear94]

Attribute#mergeAttributes completely ignores the UUIDs of the modifiers in the input multimaps, causing problems for code that depends on particular UUIDs (such as Item.ATTACK_DAMAGE_MODIFIER_ID). I'm not sure what the intended behavior for this method is.

[Boundarybreaker]

I'll take a look when I can.

[bluebear94]

Actually, since the purpose of mergeAttributes is to prevent duplicate entries in the tooltip, couldn't the current calls to that method be replaced with one that merges only the multimaps (using ImmutableMultimap.Builder#putAll), while handling duplicate entries when the tooltip is displayed?

[bluebear94]

I don't mean to be rude, but is there any progress on this issue?

[bluebear94]

It's been 3 weeks since this issue was opened; any response would be nice.

[Boundarybreaker]

855 might be helping with it? I've taken a step back from modding for the time being due to personal circumstance.

[bluebear94]

855 concerns mining levels. It isn't relevant to attribute modifiers at all from what I can get from the code.

[Boundarybreaker]

They're both part of the same module.

[shedaniel]

IMO they should become two different modules then.

Also yes #855 will not affect this.

[Boundarybreaker]

I mean, AttributeManager solely exists to help deal with Fabric Tool Attribute API, so it doesn't make sense to spin it out. We're not NPM.

[shedaniel]

No, I mean that the mining level stuff is completely unrelated to tool attributes

[Boundarybreaker]

uhhh, no? the mining level stuff was literally one of the primary points of tool attributes

[shedaniel]

The attributes mean the actual entity attributes, like attack speed and stuff. The mining speed and is effective is not really an attribute in my opinion.

[shedaniel]

Anyway, I completely went off-topic here, sorry about that. As stated, #855 won't fix this issue and this issue should be looked into.

[i509VCB]

I may look into this a bit later and write unit tests for the merging. No guarantees though