constrain(), when low > high, returns high

[Teajey]

Found that constrain() just outputs high if low is greater than high. e.g.

constrain(10, 20, 0) returns 20

Which wasn't very helpful in the situation I was using it. Perhaps it would be better that when low is greater than high the values are swapped?

constrain(10, 20, 0) same as constrain(10, 0, 20) returns 10

Nature of issue?

• [x] Existing feature enhancement

Most appropriate sub-area of p5.js?

• [x] Math https://github.com/processing/p5.js/blob/215fcbbeddc00760b337a4449ad390210cd4d9dc/src/math/calculation.js#L116-L119

Feature enhancement details:

constrain() swaps low and high, if low is greater than high.

[grege2]

Great catch @Dhomiss. You're right, there's absolutely no validation of the args to constrain(), it assumes low < high. I agree with you, that a reasonable fix would be to reverse low and high when low > high. Other related calls like min() and max() don't assume any order in the args, a user would reasonably like to interpret the constrain() call as "keep my value between these two other values".

[grege2]

Having jumped in with my $0.02, I'll put my hand up to fix this. I'll wait a day or two to see if the community has any comment.

[limzykenneth]

My opinion is that automatically swapping them around when low > high is too magical. It complicates what the function actually does by making the logic flow ambiguous. If validate parameters can check to make sure that low < high and if not send a friendly error message, that would be better in my opinion.

[Spongman]

i think that testing the relative values of parameter is currently beyond the scope of the parameter validation stuff. it's really limited to the types of parameters as far as those that can be encoded in the YUI docs. YUIdocs does have the capability for declaring some very simple limits on the values of parameters (such as '>= '), but the parameter validation stuff in p5 doesn't even support that yet.

I do think though, that we should have some centralized means for reporting various kinds of errors (such as these kind of more complex parameter validation errors), including mechanisms for debug-only warnings, user-adjustable verbosity, and perhaps piping errors directly into the web editor.

[meiamsome]

I think this may be too magical. Particularly, I think this is a problem with map() where putting in the arguments backwards changes the behaviour in a logically consistent way. The question becomes, if we force the order here to be correct, why wouldn't map do that too, which would break a lot of use cases for map.

+1 for giving a FES error if possible, though, not from _validateParameters - perhaps we should have a _assert(<case>, <message>)?

[limzykenneth]

perhaps we should have a _assert(<case>, <message>)

If that is possible it would be great, I'm not that familiar with the inner workings of FES so I shall defer here. 😉

[grege2]

Thanks @meiamsome, I see map() does do the mathematically correct thing with backwards ranges. I gave it a little thrashing here here Anyway you can see it must do the consistent thing from the clean code. But .. it does have to do a messy range test when it does the "withinBounds" check, since it uses constrain() for that. (Seems like constrain() limitations are propagating inside p5.js internals ? But it's the only instance I can see:

~/gitprojects/allsrc $ grep -E 'constrain\(' *.js
calculation.js: *   let xc = constrain(mouseX, leftWall, rightWall);
calculation.js:    return this.constrain(newval, start2, stop2);
calculation.js:    return this.constrain(newval, stop2, start2);
image.js:  duration = p5.prototype.constrain(duration, 0, 15);
image.js:  fps = p5.prototype.constrain(fps, 0, 22);

Well, putting together the comments so far, options seem to be ..

1. Do nothing, the user will spot that the range is backwards soon enough, and we're not designing nuclear power stations here.

2. Check and reverse the backwards args, like in map().

3. Add an FES extension that reports if the range is backwards, but does nothing more.

4. As for 3. but reverse the args as well.

Thinking a bit more, a backwards range in constrain() may not be the user just providing a static low/high pair backwards. The range could be computed from other app data, and be backwards due to a problem in that computation. If we silently "fix" the range (option 2), we hide the earlier error, which could be a more lengthy headache for the user than just letting it break. (I guess that's partly what folks above mean by 'magical'.). Ciao e buona giornata.

[Spongman]

i would rather not add extensions to FES that serve a single purpose. if there's a specific parameter validation that needs to happen for a given method, i think that the best place for that code is in the method itself, not somewhere that's called by nearly every other public method in the library.

[lmccart]

I think it makes sense to leave this as is for the reasons everyone has elaborated here. We may add a FES extension in the future but as @Spongman suggests, it would be nice to do a more complete pass and see if there are a number of cases where this type of checking would be useful, and build the functionality around this broader set of uses cases. For now, perhaps we can close this particular issue?

[grege2]

Ok by me.

On Mon, 18 Feb 2019 at 04:22, Lauren McCarthy notifications@github.com wrote:

I think it makes sense to leave this as is for the reasons everyone has elaborated here. We may add a FES extension in the future but as @Spongman https://github.com/Spongman suggests, it would be nice to do a more complete pass and see if there are a number of cases where this type of checking would be useful, and build the functionality around this broader set of uses cases. For now, perhaps we can close this particular issue?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/processing/p5.js/issues/3517#issuecomment-464483919, or mute the thread https://github.com/notifications/unsubscribe-auth/Af3lnLbZhwHXgbG5h2mm9NUCQ-_ZzN7bks5vOY_bgaJpZM4a5FI8 .

-- Greg Edwards