Yellow-Dog-Man / Resonite-Issues

Issue repository for Resonite.
https://resonite.com
141 stars 2 forks source link

Ability to specify rounding type for Round node #3111

Open yoshiyoshyosh opened 1 month ago

yoshiyoshyosh commented 1 month ago

Is your feature request related to a problem? Please describe.

Currently, the Round node defaults to banker's rounding, which creates unintuitive behavior for the context of what most people would use rounding for in resonite:

Image

This is because MathX.Round(float) uses Math.Round(double), which uses the default value of the MidpointRounding enum, which is MidpointRounding.ToEven.

Describe the solution you'd like

An additional field on the Round node to specify the MidpointRounding used for the node. For backwards compatibility, this will most likely need to default to ToEven. There is a Math.Round(double, MidPointRounding) overload that can be used in place internally to accommodate this.

Describe alternatives you've considered

Currently, using Round to Int then casting the result back into the float type, which is annoying.

Additional Context

No response

Requesters

yosh

Frooxius commented 1 month ago

I wouldn't add the field to the existing node to keep it simple, but we can add another variant which has both inputs to give you more control (or potentially just expose the other rounding methods as another set of nodes).

yoshiyoshyosh commented 1 month ago

I wouldn't add the field to the existing node to keep it simple

Does this mean simple on the implementation side, rather than the user side? I personally would prefer adding on to the existing node... just because /that/ feels simpler on the ux side for me...

but we can add another variant which has both inputs to give you more control (or potentially just expose the other rounding methods as another set of nodes).

I'm not sure if this might be the best approach since, in .NET 8, there are more midpoint rounding methods available. When Resonite updates to .NET 8, it'd be nice (albeit niche) to use these new rounding methods too. If they were separate nodes, though, there'd be five separate nodes... just for rounding. wouldn't seem great from a ux perspective to me

ko-tengu commented 1 month ago

Yeah, from a user experience I think it'd be generally far better to expose stuff like this on a single node and not end up with a list of nodes and a normal user not strictly knowing which one they need. There's a lot of existing nodes as example with optional settings that you can pull out if you need to change them from their defaults.

Frooxius commented 1 month ago

I don't think that's necessarily better, because it also adds cognitive load to the user and makes the node bigger in cases where you might not care about using alternate method (or even which method is used).

What I think is best solution is to just have one node that has the default method and another which lets you specify. That way if you want a fancier one with more customization, you pick that one, if you don't care, just go for the simpler one.

yoshiyoshyosh commented 1 month ago

That sounds nice, especially since it could involve Math.Round(Double, Int32, MidpointRounding) if one wants to also include how many decimal places to round to as well...

Frooxius commented 1 month ago

We'd probably do that as separate node too.

yoshiyoshyosh commented 1 month ago

I mean, if both are separate, then I'm not sure how great that'd be either, given how you'd need Round, RoundWithPlace, RoundWithMidpoint, RoundWithMidpointAndPlace in the event you wanted to use all... really I think it should be an "all and nothing" type deal. Round for simple rounding to nearest integer if you don't care about any fluff and RoundWithParams (or some other clear name) for tuning the place count and midpoint resolution as well. do the same thing with RoundToInt and this design feels the most Right to me as an end user