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

Normalized up vector of P5Camera tilt (Follow up update of PR #7598) #7681

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Forchapeatl
Copy link
Contributor

@Forchapeatl Forchapeatl commented Mar 29, 2025

This is a follow up update of PR #7598
Resolves #7680

Changes:

Screenshots of the change:

PR Checklist

@Forchapeatl
Copy link
Contributor Author

@davepagurek , please have a look at this update. It is a follow up from #7598

@Forchapeatl
Copy link
Contributor Author

@franolichdesign , Please have a look at the follow up.

@franolichdesign
Copy link

franolichdesign commented Mar 29, 2025

@Forchapeatl Thanks for raising the follow up issue and dealing with it - I forgot that I had offered to raise the issue myself! The changes you've made look good though I'm not currently able to test them from the source code as I mentioned before.

Just a quick question about coding style. Currently we have:

    let centerX = this.centerX;
    let centerY = this.centerY;
    let centerZ = this.centerZ;

    // move center by eye position such that rotation happens around eye position
    centerX -= this.eyeX;
    centerY -= this.eyeY;
    centerZ -= this.eyeZ;

This could be written more compactly as:

    /* camera center relative to eye */
    let centerX = this.centerX - this.eyeX;
    let centerY = this.centerY - this.eyey;
    let centerZ = this.centerZ - this.eyeZ;

I don't think that this makes any difference to performance and I don't know if it conforms to p5 coding style guidelines. But such a change does seem worthwhile if only to reduce the p5 library file size. Apologies if I'm being too fussy!

@Forchapeatl
Copy link
Contributor Author

Hello @franolichdesign . I think there is no other major issue . I love your coding style but we should just leave the styling as it is.

@franolichdesign
Copy link

@Forchapeatl Fair enough. Thanks for your support with my first contribution to p5.

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.

Normalize up vector of Camera's rotateView to avoid round / cummulative errors
2 participants