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

Vec3 refactor #226

Merged
merged 5 commits into from
Oct 21, 2019
Merged

Vec3 refactor #226

merged 5 commits into from
Oct 21, 2019

Conversation

hollasch
Copy link
Collaborator

In addition, this change removes the unnecessary inline keywords. On
Windows + Visual C++, this yields a slight performance improvement, but
really it was just a sanity check to ensure that this change didn't harm
performance. Your mileage may vary.
- Use class initialization for constructors
- Eliminate unused methods and functions
- Move methods into class definition
- Eliminate unnecessary `inline` keywords
- Simplify implementations, using existing functions where possible
- Binary operators have params u,v instead of v1,v2
- Drop unnecessary vec3:: qualifiers for method defs moved into class
Resolves #153
Resolves #156
Resolves #215
There are still many places in the code where we're using `vec3` for
color data, but this change fixes the lighting oversaturation problem,
and also replaces colors with NaN components with a signalling color
(pure cyan).

Resolves #93
@hollasch hollasch added this to the v3 milestone Oct 21, 2019
@hollasch hollasch requested a review from a team October 21, 2019 00:30
@hollasch hollasch self-assigned this Oct 21, 2019
@hollasch hollasch merged commit 6c7d72a into development Oct 21, 2019
@hollasch
Copy link
Collaborator Author

Post reviews welcome.

Copy link
Collaborator Author

@hollasch hollasch left a comment

Choose a reason for hiding this comment

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

You can ignore the olor change — this is reworked significantly in commit a12aab2 in the development branch.

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.

1 participant