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

rect() : inputting negative values for width and height doesn't yield a flipped rectangle anymore...absolute values of width and height are considered #7353

Closed
1 of 17 tasks
Nishchal-Bhat opened this issue Nov 3, 2024 · 11 comments · Fixed by #7363
Labels

Comments

@Nishchal-Bhat
Copy link

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

v1.11.1

Web browser and version

129.0.6668.112

Operating system

ChromeOS

Steps to reproduce this

Steps:

  1. use the rect() function with negative values passed in for width and height

Snippet:

rect(100,100,40,40)
rect(100,100,-40,-40) // gives the same rectangle
@Nishchal-Bhat Nishchal-Bhat changed the title rect() : inputting negating values for width and height doesn't yield a flipped rectangle anymore...absolute values of width and height are considered rect() : inputting negative values for width and height doesn't yield a flipped rectangle anymore...absolute values of width and height are considered Nov 3, 2024
@paul-uz
Copy link

paul-uz commented Nov 5, 2024

Just spotted this. Pretty poor regression.

@davepagurek
Copy link
Contributor

This was changed in #7290.

Before considering this a regression we want to fix (a negative width is arguably undefined behaviour?), @martinleopold and @ff6347 do you have opinions on what the expected behaviour here should be?

@paul-uz
Copy link

paul-uz commented Nov 5, 2024

@davepagurek upon double checking, I'm not sure that is where the change happened, as the changes in that PR is for arcs. however, comparing 1.11.0 (which works) and 1.11.1, i can't see anywhere else that might of caused this regression/change

@davepagurek
Copy link
Contributor

I believe modeAdjust, which was modified, is also is used by rect, and they added unit tests for rectangles too.

@davepagurek
Copy link
Contributor

Also ccing the rendering stewards in case anyone has thoughts: @limzykenneth, @ChihYungChang, @teragramgius, @tuminzee, @Zarkv, @robin-haxx, @Gaurav-1306

@martinleopold
Copy link
Contributor

martinleopold commented Nov 5, 2024

This was changed in #7290.

Before considering this a regression we want to fix (a negative width is arguably undefined behaviour?), @martinleopold and @ff6347 do you have opinions on what the expected behaviour here should be?

I think this was caused by my PR. It's pretty much undefined behaviour as the reference doesn't talk about negative widths and heights. Some comments in the code were mentioning "p5 supports negative width and heights for rect" but not more.

Note that prior to my PR there were virtually no tests for the basic shape rendering/and shape modes, so no 'defined' behaviour in that regard.

Using negative width/height to flip rectangle/ellipse/arc does make sense to me though – for rectMode(CORNER) at least. I think we should revert to this behaviour and use the opportunity to update the docs accordingly as well.

@paul-uz
Copy link

paul-uz commented Nov 5, 2024

Negative height and width has been part of p5js for as long as I can remember. This is certainly an accidental regression and a breaking change. It makes 1000% sense as it's the easiest way to draw rectangles from the bottom up due to p5's "odd" coordinate system

@limzykenneth
Copy link
Member

Yes, it is a regression in this case. Feel free to file a PR for a fix.

@ffd8
Copy link
Contributor

ffd8 commented Nov 6, 2024

I just ran into this too – I usually render FFT audio bars with something like rect(x, height, w, -freq), and just wondered why it stopped working?! Indeed just like circle() and ellipse() the negative value has always been an inversely drawn form since many versions ago.

@limzykenneth
Copy link
Member

@Nishchal-Bhat @paul-uz This should be fixed in #7363 now, you can try it here. If no other problem with this is found I'll probably do a release some time next week.

@Nishchal-Bhat
Copy link
Author

@martinleopold @limzykenneth @paul-uz ok nice...thanks a lot for your time and effort 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants