Added support for infinite duration effects

[RemBog]

Infinite effects

Added setInfinite and isInfinite methods to EffectInstance.
Added infinite argument to /effect command to make effect infinite.
Added a new KnowTranslation: “commands.effect.success.infinite”.

[dktapps]

This is BC-breaking to any users of EffectInstance::getDuration().

I can understand the desire for this in the /effect command, but I don't think we need alterations to the internals to achieve this. Providing Limits::INT32_MAX is good for nearly 5 years.

I could get onboard with adding support for passing max instead of a number for the effect duration, but the current change is too invasive for such little benefit IMO.

[ShockedPlot7560]

I can also see the need, but even if it means breaking the BC by changing the function's bound, wouldn't it be better to use null rather than -1?

[RemBog]

To make the duration of the effect infinite, the duration must be equal to -1 in the packet, hence my decision to do this. I'll rework my pull request so that it doesn't break the BC soon. But if you have any ideas on how to avoid breaking the BC, I'd love to hear them, but I don't think setting null instead of -1 will help.

[dktapps]

I don't think it's necessary to support infinite duration in Effect itself. As I said, the max duration allows for an effect that lasts nearly 5 years.

I would prefer just accepting max as a duration value in the /effect command instead.

[RemBog]

Too bad, I think it's not clean to use a large number when you want an effect to never stop. But have it your way. Minecraft Bedrock has added something that Java has had for years, and it's this kind of detail that makes the game experience cleaner, even if it's true that in the end it doesn't change anything in the player's gameplay. Isn't the point of software to have a server support with as many features as possible in common with the basic game?

[dktapps]

It's not worth breaking backwards compatibility over.

[dktapps]

Per suggestion from @SOF3 this could mostly avoid BC breaks by using Limits::INT32_MAX as the magic infinity value. This would avoid breaking code that checks getDuration() > 0.

It would still be a behavioural change, but the only code affected would likely have intended an infinite effect anyway.

However, this would require conversion to the appropriate Bedrock values on the network layer and for NBT entity storage.

[github-actions]

This PR has been marked as "Waiting on Author", but we haven't seen any activity in 7 days.

If there is no further activity, it will be closed in 28 days.

Note for maintainers: Adding an assignee to the PR will prevent it from being marked as stale.