Skip to content

Conversation

ombalgude
Copy link

@ombalgude ombalgude commented Sep 25, 2025

Hi @davepagurek! Thanks again for the detailed feedback. This new pull request addresses the comments from the previous attempt in PR #8102.

Closes #7959

Changes:

  • This PR introduces the setViewport(xmin, xmax, ymin, ymax) function, which allows users to define a custom, resolution-independent coordinate system. This makes it easier to create responsive and scalable sketches.

Addressing Feedback from PR #8102:

  • Targets dev-2.0: This PR is now correctly based on and targets the dev-2.0 branch for the upcoming p5.js 2.0 release.
  • Uses Addon Structure: The function has been implemented using the new defineAddon structure required for p5.js v2.
  • Includes Visual Tests: I have added a new visual test to ensure the feature renders correctly and to prevent future regressions.

PR Checklist

  • npm run lint passes
  • Inline reference is included / updated
  • Visual tests are included / updated

@ombalgude
Copy link
Author

ombalgude commented Oct 7, 2025

Hi @davepagurek @limzykenneth and @perminder-17

I just wanted to check in on this PR and see if there's anything I can do to help move it forward.

I'm happy to answer any questions or address any feedback. Thanks!

@perminder-17 perminder-17 self-requested a review October 7, 2025 09:11
Copy link
Collaborator

@perminder-17 perminder-17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi and really sorry for the delay in reviewing your PR.

Do you think we should keep this funciton as an add-on library as @limzykenneth said? https://beta.p5js.org/contribute/creating_libraries/

I think it can be implemented as an addon library first, if not just to test out the idea itself and work out the desired API in a more flexible way.

@ombalgude
Copy link
Author

Thanks so much @perminder-17 for the review and the detailed suggestion!

That makes a lot of sense. Creating setViewport as an addon library to test and refine the API sounds like a great approach. I'm happy to take this on.

I'll read through the library creation guide you linked and will share the repository here for feedback once I have a working version.

@ombalgude
Copy link
Author

Hi! @perminder-17 I've just pushed the requested changes.

Based on your feedback, I've refactored the setViewport feature. It has been removed from the core and is now a standalone addon located at lib/addons/p5.viewport.js, following the convention of other addons in the project.

Thank you for the guidance! Let me know if there's anything else.

here is the link of PR : #8138

@limzykenneth
Copy link
Member

This PR does not implement what it describe to implement. Also the orignal issue was not approved for implementation into p5.js core. I'll close this for now. Discussion should continue on the original issue if needed and a PR should not be filed until stewards or maintainer approves it for PR per the contributor guidelines.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants