Fabric API

Fabric API

106M Downloads

Attribute#mergeAttributes doesn't respect modifier UUIDs

bluebear94 opened this issue ยท 15 comments

commented

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.

commented

I'll take a look when I can.

commented

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?

commented

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

commented

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

commented

They're both part of the same module.

commented

IMO they should become two different modules then.

Also yes #855 will not affect this.

commented

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.

commented

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

commented

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

commented

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

commented

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

commented

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.

commented

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.

commented

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

commented

Closed by #1355.