Description
Increasing Access
If the user tries to draw a shape where any of the arguments are NaN it does not draw anything. I would expect that it would log an error using the friendly error system so that the user knows why their code didn't do what they intended. This would make it much easier for beginners to debug their code.
Most appropriate sub-area of p5.js?
- Accessibility (Web Accessibility)Build tools and processesColorCore/Environment/RenderingDataDOMEventsFriendly error systemImageIO (Input/Output)LocalizationMathUnit TestingTypographyUtilitiesWebGLOther (specify if possible)
Feature enhancement details
Initially I assumed that p5.js was somehow catching or avoiding a DOM error. Unfortunately it seems like the canvas API itself does not give any errors or warnings when encountering NaN values (SVG does
We would want to include an NaN check in all shape functions such as rect(), line(), etc.
Here is an example code but it's very uninteresting. It doesn't do anything.
function setup() {
createCanvas(400, 400);
vanillaJs();
}
function draw() {
background(220);
fill(0);
const myVar = undefined + 1;
rect(0, myVar, 100, 100);
line(100, 100, myVar, 100);
}
function vanillaJs() {
const canvas = document.createElement('canvas');
canvas.width = 400;
canvas.height = 400;
document.body.appendChild(canvas);
const ctx = canvas.getContext('2d');
ctx.fillStyle = 'red';
const myVar = undefined + 1;
ctx.fillRect(0, myVar, 100, 100);
}
Metadata
Metadata
Assignees
Type
Projects
Milestone
Relationships
Development
Participants
Activity
welcome commentedon May 12, 2022
Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, be sure to follow the issue template if you haven't already.
limzykenneth commentedon Aug 14, 2022
@almchung @outofambit Would you like to have a look at this? I would imagine including a check for
NaNhere as technicallyNaNis considered a number type, or even using the nativeisNaNorNumber.isNaNfunction to do this check.almchung commentedon Aug 15, 2022
Good catch! The current
isNumber()(which uses the nativeisNaN()) misses the case where Javascript returns the global propertyInfinity: L349. It's unclear whetherInfinitycan be considered a number, for the purpose of our drawing functions, we may only want to take finite number arguments. One remedy can be usingisFinite()to only pass cases where finite numbers are passed as arguments. From MDN doc,isFinite()returns:limzykenneth commentedon Aug 15, 2022
In the case of
NaN(not string that parses toNaN) it will satisfy the first condition oftypeof NaN == "number"and return true right there. Directly usingisNaN()only can take care of most cases I think, but there may be edge cases I didn't think of. For infinity we'll need to leave it be as it has some utility when dealing with number comparisons (max()andmin()for example).almchung commentedon Aug 16, 2022
Sorry, I totally misread last time. I agree using
isNaN()should solve this.Number.isNaN()doesn't count in the fact that a correctly formatted numerical string argument will be converted into a number.Found another bug while testing:
nullargument would be converted to 0 and thus shouldn't throwEMPTY_VARerr msg.I still think drawing functions like
line()may benefit from usingisFinite()or some kind ofInfinitychecking, but not sure if that should be a high priority :)oscarczer commentedon Oct 19, 2022
Is anyone currently working on this? If not I might have a crack at implementing the finite/infinite checking aspect
Qianqianye commentedon Oct 19, 2022
@oscarczer Please go ahead with a fix! Thanks.
oscarczer commentedon Oct 25, 2022
Just confirming that we want null to be accepted as an input? I was going to implement an additional check here alongside finite checking to ensure that the string "null" isn't considered a valid parameter (as I don't believe that isNaN accounts for this) but saw what @almchung said and am now not so sure. I feel as if null and 0 shouldn't necessarily be treated the same as they could likely occur from different situations but if you'd like this to be the case I'll leave it as it :)
almchung commentedon Oct 25, 2022
@oscarczer I agree that
nulland0shouldn't be treated the same. In some cases (like seen inline(), people will make use of this auto null-to-zero conversion, but this would be much rare (and since this kind of usage will be intentional, people can disregard the error messages).I have a question though: in the proposed
isNumber(), whenparam's type is string [here from PR], did you intend to check for an empty string,param==""instead ofparam=="null"? I think it would be meaningful ifisNumber()can catch the empty string case (especially sinceisNaN("null")would returntrueandisNaN("")would returnfalse).oscarczer commentedon Oct 26, 2022
@almchung I was under the impression that isNaN(null) returns false as for some reason it is given the type of number (I think actually because of the instance it is able to be turned into 0?). I assumed that this would extent to isNaN("null") as well however I could definitely be wrong about that. Given this, it might also be worth putting a check for null in the first case of the param switch. However, irrespective of that I'm more than happy to add a check for isNaN("") as well as you're right about that also being an edge-case worth accounting for
almchung commentedon Oct 28, 2022
Javascript can be confusing -- it would be helpful to check their String coercion rules on the official reference here.
I don't think we will need to add a separate case for checking
null, sincetypeof(null)==objectwould followdefaultcase where the current code already returnsfalse.Getting
"null"or""for your parameter would be a different case, where yourtypeof("null")andtypeof("")are bothstring-- our current code would treat"null"as a string (so we get an expected output), however, the empty string will be converted into0(which is a special case we may want to include in our check).Hopefully this is helpful!
Bernice55231 commentedon Mar 29, 2023
Hi, I was wondering why we use
isNaN()to check under the 'string' case in the functionisNumber. From my understanding,isNaNshould be under 'number' case.limzykenneth commentedon Mar 29, 2023
@Bernice55231 For p5.js, it is possible to pass a number string (eg.
"100") to functions expecting numbers and it'll work as if the string has been parsed into a string.isNaN()is special in that it works on both numbers and strings that can be parsed into numbers, and will returntrueif it found that a string passed in cannot be parsed into a number but false if the string can be normally parsed as a valid number (which is the behaviour matching p5.js).