Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mostly offscreen primitive shapes throws error #7259

Closed
1 of 17 tasks
mimiyin opened this issue Sep 9, 2024 · 13 comments · Fixed by #7626 · May be fixed by #7621
Closed
1 of 17 tasks

Mostly offscreen primitive shapes throws error #7259

mimiyin opened this issue Sep 9, 2024 · 13 comments · Fixed by #7626 · May be fixed by #7621

Comments

@mimiyin
Copy link

mimiyin commented Sep 9, 2024

Most appropriate sub-area of p5.js?

  • Accessibility
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • WebGL
  • Build process
  • Unit testing
  • Internationalization
  • Friendly errors
  • Other (specify if possible)

p5.js version

1.8.0+

Web browser and version

128.0.6613.86 (Official Build) (x86_64)

Operating system

MacOSX

Steps to reproduce this

Steps:

  1. Draw ellipse(400, 400, 400,400);

  2. Change the y-coordinate to 440

  3. Error

  4. Draw ellipse(400, 0, 400,400);

  5. Change the y-coordinate to -1

  6. Error

I was not able to repro with changes to the x-coordinate
I was able to repro with line() and rect().
Errors go away with pre 1.8 versions.

Error:
TypeError: Cannot read properties of undefined (reading '9')
at undefined:52576:53

🌸 p5.js says:
[p5.js, line 52576] Cannot read property of undefined. Check the line number in error and make sure the variable which is being operated is not undefined.

  • More info: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cant_access_property#what_went_wrong
    ┌[https://cdnjs.cloudflare.com/ajax/libs/p5.js/1.8.0/p5.js:52576:53]
    Error at line 52576 in _gridMap()
    └[https://cdnjs.cloudflare.com/ajax/libs/p5.js/1.8.0/p5.js:52541:26]
    Called from line 52541 in _main.default._updateGridOutput()
    └[https://cdnjs.cloudflare.com/ajax/libs/p5.js/1.8.0/p5.js:52977:20]
    Called from line 52977 in _main.default._updateAccsOutput()
    └[https://cdnjs.cloudflare.com/ajax/libs/p5.js/1.8.0/p5.js:71385:22]
    Called from line 71385 in _main.default.redraw()
    └[https://cdnjs.cloudflare.com/ajax/libs/p5.js/1.8.0/p5.js:62766:23]
    Called from line 62766 in _draw()

Snippet:

function setup() {
  createCanvas(400, 400);
}

function draw() {
  background(220);
  
  // No-Error
  ellipse(400, 0, 400,400);

//   // Error
//   ellipse(400, -1, 400,400);

//   // No-Error
//   ellipse(400, 400, 400,400);

//   // Error
//   ellipse(400, 440, 400,400);
}
@mimiyin
Copy link
Author

mimiyin commented Sep 10, 2024

I can no longer reproduce this issue.

@michaelo
Copy link

Just a FYI that I may have identified the step required to reproduce the issue:

It seems to me that a key factor to trigger the error is to enable accessible output (ctrl+shift+1 to enable, ctrl+shit+2 to disable on Windows, replace "ctrl" with "cmd" for macOS).

processing/p5.js-web-editor#3243 (comment)

@eyaler
Copy link

eyaler commented Dec 4, 2024

i get consistent crashes with v1.11.2 on firefox and chrome without enabling anything and with a single negative y pixel:

line(0, 0, 0, -1)

run

also quick workaround hack i found here: #7130 (comment)
is to add gridOutput = function(){} to setup()

@davepagurek
Copy link
Contributor

Looks like in #7135 a check was added for shapes that go off of the max end of the canvas. Probably a similar PR is needed to add an additional check for the min end of the canvas (checking that all the indices are >= 0) if anyone is interested in contributing to this!

@eyaler
Copy link

eyaler commented Dec 4, 2024

@davepagurek if gridOutput is not part of the main functionally maybe in addition to the check, we should put it in a try/catch?

@davepagurek
Copy link
Contributor

Sorry for the delay @eyaler, sounds good!

@davepagurek davepagurek marked this as a duplicate of #7535 Feb 13, 2025
digitaldina added a commit to digitaldina/p5.js that referenced this issue Feb 25, 2025
Check for negative indices and skip shape drawing if index is negative
@digitaldina
Copy link

Hi! New contributor to the p5.js repo, attempting this issue
@eyaler - could you clarify what you mean by adding a try/catch?

My proposed change is build off of #7135, added a check for negatives indices in the if statement so that it skips shapes with negative indices. Let me know if that's sufficient or if I'm missing something here! thanks

@davepagurek
Copy link
Contributor

@digitaldina I think that works too! Wrapping the whole thing in a try/catch could prevent any error from escaping, but also runs the risk of us missing important bugs, so adding checks for known cases causing the issue sounds good to me.

@eyaler
Copy link

eyaler commented Feb 25, 2025

i previously closed the bug i opened in editor processing/p5.js-web-editor#3243, in favor of this one, but i actually think now that there are indeed two issues: the first is the underlying p5.js bug discussed here. but this will only fix future versions. i think the web editor should actually add the try-catch around the accessible output stuff so it doesn't fail on previous versions of p5.js - other wise it will continue to break on many existing sketches. @davepagurek - what do you think?

@davepagurek
Copy link
Contributor

@eyaler that makes sense to me!

@southwest-git
Copy link

I think gridOutput.js has this function _gridMap that doesn't expect locX and locY to be negative when checking if a shape is in canvas. I have a simple fix for this but I can't publish a new branch and hence submit a pull request. Let me know how to proceed. I can also simply share the fix here. Thanks!

@davepagurek
Copy link
Contributor

@southwest-git to submit a patch, you can fork this repo, make the change in your fork, and then create a PR from your fork to this upstream repository. There are some instructions here: https://p5js.org/contribute/contributor_guidelines/#forking-p5js-and-working-from-your-fork

Also as an additional heads up, we are close to releasing version 2.0 of p5! In addition to fixing this on the main branch, it would be great to make another branch off of dev-2.0 afterwards and making a PR into dev-2.0 to fix it there too. Those can be done one at a time if you prefer.

@himanshuukholiya
Copy link
Contributor

I think grid is a table where shapes are placed into cells based on their calculated locX and locY positions, determined by the _canvasLocator() function. The bug arises when shapes are offscreen, as their center coordinates can result in locX or locY values that are negative or greater, leading to errors when trying to access the grid table cells.

So I declared two variables x & y which takes the values of args[0] & arg[1], then check, 0 <= x < canvasWidth and 0 <= y < canvasHeight. If not, return null.

davepagurek added a commit that referenced this issue Mar 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants