Sash items don't affect minecraft:generic.movement_speed attribute, making multiply (operation 2) modifiers have unexpected outcomes

[Partonetrain]

Mod Loader

Both Fabric and Forge (I confirm that I have tested both loaders and will specify both loader versions below)

Minecraft Version

1.20.1

Botania version

1.20.1-443

Modloader version

1.20.1

Modpack info

personal custom pack

The latest.log file

N/A

Issue description

The sash items work by moving the player more if they are already moving, making multiple multiply speed attribute modifiers act unexpectedly. For instance, The Sojourner's Sash would have Operation 2, value 1.32 attribute modifier.

https://minecraft.wiki/w/Attribute#Operations

The multiply operation acts on every modifier, and since the sash doesn't actually have a modifier, the outcome is weird

All speeds are in approximate m/s, measured with Speedometer

unchanged walk speed: 4.7584 w/ Slowness 1: 4.035 (-15%) w/ Speed 1: 5.6979 (+20%) w/ Both Effects: 4.8432 (+1%) - notice how it's not 5%. This is the expected vanilla behavior w/ Sojourner's Sash: 6.2907 (+32%) w/ SS + Slowness 1: 5.577 (+17%) - notice how it is -15% +32% w/ SS + Speed 1: 7.2392 (+52%) - notice how it is +32% +20% w/ SS, Slowness, and Speed: 6.3857 (+~33%) - around 32%+1%

Steps to reproduce

1. Wear
2. Add multiply attributes by commands, potion, or other mods. Can be done in survival with slowness or speed potions
3. run calculations

Other information

This is more of "consequence of how it works" rather than a bug, but it doesn't match vanilla behavior for speed modifiers. It won't be noticeable for most players, but rather it has caused weirdly specific interactions in my personal modpack, and will begin to matter at higher speeds

Feel free to close if this is a nonissue

[TheRealWormbo]

Interesting find. We should probably switch to attributes for the speed boost, like the step-up boost is already handled. Same likely goes for the jump modification, which also sets player movement instead of adding attribute modifiers.

[Partonetrain]

Unless we want to add another dependency (such as AdditionalEntityAttributes), the Jump Height attribute will have to wait until 1.20.5 (at which point the step height attribute will also be in vanilla)

[TheRealWormbo]

I believe the plan is to stick with 1.20.1 until we replace Forge with Neoforged, which might only happen for 1.21.

[Partonetrain]

I just realized - it's probably done this way to prevent the Field-of-View change. If the speed method was changed to an attribute, would it be possible to mixin to the client code and check for the sash modifier and prevent the FOV change from that one attribute?

[TheRealWormbo]

Further investigations show that the sash items specifically boost forward motion, and only while the player is on ground or in creative flight. The way the sash applies its speed modification generally matches the way flowing water/lava applies its push, but maybe the player's speed modifications should also be accounted for in the sash movement boost.

[anoomolu]

Would the latter change you proposed make it work more like Thaumcraft’s Travellers Boots? (Where forward velocity is taken into account for sprint jumping, for example)

[TheRealWormbo]

As I've never really gotten into Thaumcraft (it's just not my playstyle), I don't know what the Travellers Boots do. I would like to stick with the forward speed boost only applying when on ground or in creative flight, as the sash already applies it. And while I'm not sure how exactly to implement it yet, the only difference would hopefully be that the sash boost interacts with other speed boosts in a more predictable manner.