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

Added Exc variants of all methods in frustum that required them. #47

Merged
merged 4 commits into from
Sep 24, 2020

Conversation

oxt3479
Copy link
Contributor

@oxt3479 oxt3479 commented Aug 24, 2020

This PR also includes unit tests for these new functions to verify the behave the same as their exception throwing counterparts.

Signed-off-by: Owen Thompson oxt3479@rit.edu

Signed-off-by: Owen Thompson <oxt3479@rit.edu>
@cary-ilm
Copy link
Member

The Vec classes take the opposite convention: V3f::normalize() does not throw; V3f::normalizeExc() throws.

It seems we have two choices:

  1. Let the Frustum methods be inconsistent
  2. Be consistent but change the API: projectPointToScreen() would not throw (it current does); add new projectPointToScreenExc() that does throw.

Also, the integer specializations of Vec{2,3,4}::normalize() do throw, which makes them inconsistent.

Also, the Matrix classes take a different approach, with Matrix{22,33,44}::inverse(bool) takes an argument that specifies whether to throw or no. This means there's no method that can be marked noexcept.

I would vote for consistency, and for an API in which the most basic behavior is to not throw, with specially-named methods that do.

Thoughts?

@lgritz
Copy link
Contributor

lgritz commented Sep 10, 2020

My preference is default noexcept everywhere, and occasional varieties that can throw and have names that make it totally obvious that they do.

Copy link
Contributor

@kdt3rd kdt3rd left a comment

Choose a reason for hiding this comment

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

thanks! Had a few comments

src/Imath/ImathFrustum.h Outdated Show resolved Hide resolved
src/Imath/ImathFrustum.h Outdated Show resolved Hide resolved
src/Imath/ImathFrustum.h Outdated Show resolved Hide resolved
src/Imath/ImathFrustum.h Outdated Show resolved Hide resolved
@cary-ilm
Copy link
Member

@oxt3479, we discussed this and would prefer this convention:

projectPointToScreen() does not throw and is marked "noexcept".
projectPointToScreenExc() throws on error.

Same with the others. Thanks!

Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

Fantastic! Thanks for this!

@oxt3479 oxt3479 changed the title Added noexcept variants of all methods in frustum that required them. Added Exc variants of all methods in frustum that required them. Sep 14, 2020
@cary-ilm
Copy link
Member

@kdt3rd, are your request changes resolved? Can you clear them so we can merge?

@kdt3rd kdt3rd merged commit 1701cec into AcademySoftwareFoundation:master Sep 24, 2020
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.

None yet

5 participants