Skip to content

Update examples that use createCamera() to explicitly call setCamera() #7607

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

Merged

Conversation

webermayank
Copy link

@webermayank webermayank commented Mar 7, 2025

#7602 fix
updated the documentation to call setCamera() in case it hadn't been called, in the examples which uses createCamera().

Copy link

welcome bot commented Mar 7, 2025

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

@webermayank
Copy link
Author

@perminder-17

did my PR didnt solve the issue?
Please address me that changes require, i'll do that

The extra lines that were useless at the bottom, i tried to revert those changes but it keeps coming when i staged the files, may its the formatter of my ide, tell me if it will make any issue , i'll remove those unnecessay changes.

@webermayank
Copy link
Author

I correct the unwanted changes, sorry for inconvenience

@GregStanton
Copy link
Collaborator

GregStanton commented Mar 21, 2025

Hi @webermayank! Thanks for working on this. I'm adding it to a list of documentation changes that need to be reviewed. We should be able to review and merge this by the end of the month. In the meantime, in case you'd like to continue helping out, I'll outline some additional steps you can take.

  1. Enter the following search phrase into Google: site:p5js.org/reference -site:*.p5js.org/ "createCamera"
  2. In the description of this PR (at the top), add a list of links to each page in the search results (there should be around 40 results). If you see a message at the end saying similar results have been omitted, click to show the omitted results too.
  3. Tag each link with one of the following: [need to check page], [no change needed], [change needed], [change made], [change reviewed]

Each link can start out with "[need to check page]." Once all the pages are tagged with either "[no change needed]" or "[change reviewed]", we should be able to merge this PR. If you want to do this work (except for the reviewing), just let us know, and at least one of us (@davepagurek, @perminder-17, @holomorfo, or myself) will do a review, so we can merge this. If you don't have time to work on this by the end of next week, we'll help out.

Also, @davepagurek:

I could look into this more, but since you're already familiar with the changes, I thought I'd ask you a couple things first.

  1. Does createCamera on p5.Framebuffer have the new behavior? I guess it probably should, for consistency. If it doesn't, we may need to update the code and then update the documentation. For example, I suppose we might need to update both https://p5js.org/reference/p5/createCamera/ and https://p5js.org/reference/p5.Framebuffer/createCamera/.
  2. Might we need to update the docs for setCamera() too? I'm thinking that in any examples where setCamera() is already called, we should be fine, but maybe the description of setCamera() itself should be updated, since it's no longer just for switching between multiple cameras.

@webermayank
Copy link
Author

@GregStanton, Okay i'll do it, i'll mention all the links related to the reference which include createCamera and create a list with checks.
Tell me if anything else needs to be updated, i would love to address those.

@GregStanton
Copy link
Collaborator

Thanks so much @webermayank!

@davepagurek
Copy link
Contributor

p5.Framebuffer.createCamera is updated too! For the createCamera docs, arguably it's still fine since it's switching between the default camera and user-created cameras too, but we can definitely refine the wording to be clearer in the future.

@davepagurek davepagurek merged commit 55db905 into processing:dev-2.0 Mar 29, 2025
2 checks passed
@GregStanton
Copy link
Collaborator

Hi @davepagurek! Did you check the pages that are listed at the top of this PR? @webermayank Did a great job of listing all those possible dependencies, but based on the checkboxes in the list, it looks like they hadn't been updated yet when you merged the PR? I'm trying to get us all on the same page with respect to reference dependencies. The idea is to have one PR that covers both (a) the reference page for the feature that was directly changed, and (b) the dependent reference pages that mention the feature in the description or use the changed feature in the examples.

Otherwise, we may end up with reference pages that have outdated descriptions. Those would be very hard to catch when we do overall sense checks (the reviewer would need to be super familiar with all changes that have been made in 2.0), so those likely wouldn't get fixed at all. Those kinds of oversights probably wouldn't be too frequent, but we'd also run the risk of having a lot of broken code examples when we do the sense check. That'd be quite likely, and it'd be much harder to coordinate fixes at that point, since the reviewer typically won't be the person who made the updates (that's by design). (Sorry if you did already do this stuff. I just want to make sure we're all on the same page, and I'm trying to save a bit of time by not rechecking everything myself.)

Also, are the docs for p5.Framebuffer's createCamera() updated, as well as any reference dependencies, or just the code?

@webermayank
Copy link
Author

@GregStanton , i think i mistakenly forgot to allow edit by maintainers check box may be that's why they are still unchecked,
Now i have enabled it. Sorry for trouble

@davepagurek davepagurek mentioned this pull request Mar 29, 2025
3 tasks
@davepagurek
Copy link
Contributor

Hey @webermayank, no worries -- part of why I wanted to merge was so that I could get a deploy of the current state of examples + docs live on https://beta.p5js.org/reference/, which also makes it easier to click through reference links (there are some bugs on the local development server where you also have to add a / to the URL bar manually right now.)

I noticed a few small areas where one example on a page was missing setCamera while the others still had it, and I noticed an additional bug where the linePerspective() getter in 2.0 had stopped working, so I've updated those in a new PR here: #7688

@GregStanton sorry for moving quickly and not maintaining the 1 PR per section. I've looked through the camera section of the docs in the process though, so we can consider this to be part of the sense check for this subject area.

@GregStanton
Copy link
Collaborator

@webermayank No problem! You've been a great help.

@davepagurek Well, I should probably apologize for moving too slowly before you apologize for moving too quickly 😅 Also, thanks for your help with checking the docs, and for the extra explanation. That helps me to understand some of the challenges.

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

Successfully merging this pull request may close these issues.

4 participants