-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Update shape attribute references #6841
Update shape attribute references #6841
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nickmcintyre. It looks great. Left some minor edit comments!
@limzykenneth and @davepagurek, please feel free to chime in.
@@ -71,26 +98,75 @@ p5.prototype.ellipseMode = function(m) { | |||
}; | |||
|
|||
/** | |||
* Draws all geometry with jagged (aliased) edges. | |||
* Draws certain features with jagged (aliased) edges. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you share the reason to make the change from 'Draws all geometry' to 'Draws certain features'? It seems make more sense to 'draw geometry' than 'draw features'. If it's important to keep 'certain', how about 'Draws certain geometry'? @nickmcintyre
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reason for this is that there's no way to turn off antialiasing on lines and shapes in 2D mode, and I think this confuses a lot of users (it's come up in the discord a few times.) In 2D mode, I believe this just affects image scaling in 2D mode, so it actually doesn't affect geometry at all, only images. Somewhat confusingly, this does not affect image scaling in WebGL mode (you have to use an undocumented method on p5.Texture) but does actually affect antialiasing. The situation is unfortunately a bit complicated right now :')
I think the description in this PR does a pretty good job of mentioning the key details quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Qianqianye basically what @davepagurek said. I sketched a few test cases based on #5472 and tried to describe what I saw.
@@ -172,27 +301,76 @@ p5.prototype.rectMode = function(m) { | |||
}; | |||
|
|||
/** | |||
* Draws all geometry with smooth (anti-aliased) edges. <a href="#/p5/smooth">smooth()</a> will also | |||
* improve image quality of resized images. | |||
* Draws certain features with smooth (antialiased) edges. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar comment as 'noSmooth'
Could you share the reason to make the change from 'Draws all geometry' to 'Draws certain features'? It seems make more sense to 'draw geometry' than 'draw features'. If it's important to keep 'certain', how about 'Draws certain geometry'? @nickmcintyre
Preparing references in
src/core/shape/attributes.js
for the new website. I also took a crack at addressing #5472.PR Checklist
npm run lint
passes@Qianqianye @davepagurek @limzykenneth