Skip to content

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.

@ksen0 ksen0 requested a review from perminder-17 June 4, 2025 17:55
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