-
-
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
Addresses issue #6519 #6532
Addresses issue #6519 #6532
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 for taking on this task! If you run grunt yui:dev
you can see your docs changes locally. They currently look like this:
What's going on here is that it doesn't render correctly when you place more description after the parameters. All of your description has to be above the examples and parameters sections for it to get correctly placed in the top section.
src/webgl/p5.Camera.js
Outdated
* @param {Number} [fovy] - camera frustum vertical field of view, | ||
* from bottom to top of view, in <a href="#/p5/angleMode">angleMode</a> units. | ||
* The default value is now variable and based on the canvas size as: | ||
* this.defaultEyeZ = 800; |
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.
Instead of writing this as code, maybe we can write what's going on in plain English? i.e. we pick a field of view that ensures you can see exactly width
pixels across horizontally and height
pixels across vertically 800 units away from the camera.
src/webgl/p5.Camera.js
Outdated
* @param {Number} [near] frustum near plane length or the near clipping plane. The default value = this.defaultEyeZ * 0.1; | ||
* @param {Number} [far] frustum far plane length or the far clipping plane. The default value = this.defaultEyeZ * 10; | ||
* | ||
* However,If you prefer a fixed field of view, you can use the old defaults: |
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.
Rather than referring to these as the old defaults, we can motivate why someone might want to do this by starting with "If you want a fixed field of view". To ensure that you can see width
across horizontally and height
across vertically at that fixed field of view, you need to place your camera (height/2) / tan(fovy / 2)
away from the subject, and ten go on to explain how the near/far values are derived from that.
src/webgl/p5.Camera.js
Outdated
* - Far clipping plane: eyeZ * 10 | ||
* | ||
* @example | ||
* Set a fixed field of view |
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.
Was this intended to be a new example with code?
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.
Was this intended to be a new example with code?
yes my bad , I thought of providing a separate example for the perspective function's syntax with the old default values. But now , i think there's no need of a separate example for this. I 'll just remove this one.
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'm also noticing that the camera clips through the cubes in the example now. Can we maybe also adjust the translate()
call in the example to push the scene farther back to avoid this?
I'll just work on these suggestions, and get back to you .Thanks a lot for the suggestions..! @davepagurek |
@davepagurek , I have made the changes please have a look at them and let me know if some more changes need to be made. |
Due to some unwanted things happening in the repository I had to take a brutal step of deleting the repository . I'll get back to solving this issue very soon @davepagurek . |
Addresses issue #6519
hey @davepagurek , I tried working on this issue ,Please let me know if some changes need to be made to this solution. I am open to incorporate any of your suggestions to solving this issue. Thank you!
Changes:
main...Garima3110:p5.js:perspective
Screenshots of the change:
npm run lint
passes