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

Eliminate normalize and length methods for Vec<inttype> #72

Merged
merged 2 commits into from
Dec 18, 2020

Conversation

lgritz
Copy link
Contributor

@lgritz lgritz commented Dec 13, 2020

Vec<>::normalize*() has very strange semantics for integer component
types -- if exactly one axis has a nonzero component, it is set to 1,
otherwise it throws an exception because there are no unit length
integer vectors having anything other than a single nonzero
components.

The presence of these (and in particular the possibility of their
throwing exceptions) is interfering with our goal of marking the
normalize methods as noexcept, even for float types where they don't
throw exceptions.

Vec<>::length() is also strange: for integer component types would
return to a rounded integer, which is also strange because this means
it would generally not be the correct Euclidean length of the vector
(except when coincidentally the components were Pythagorean triples).

I'm not sure when the existing semantics would be useful, so I'm
proposing just marking all of these as = deleted to make it a
compile-time error to try to use these nonsensical functions on
vectors of integral component type. This allows us to eliminate the
ImathVec.cpp file, which only served to house the integer
implementations of these suspect methods.

Additionally, there were some other functions such as project() and
orthogonal() that depended on normalized vectors. So this patch
employs enable_if to ensure those templates can't be instantiated
for vectors of integral component type.

Because it's very handy to have a more readable wrapper around the
enable_if idioms, I also added that and a few other tidbits to
ImathPlatform.h (seemed like as good a place as any), which I propose
to be our standard place to put those bits of syntactic sugar that
need some tricky defines to make them work across different C++
standards (for example, that's where I make it use std::enable_if_t
for C++14 and beyond, but define it in the Imath namespace when using
C++11). I also moved the definition of IMATH_CONSTEXPR14 here as well,
as it seemed to go with the other things.

One of the hairiest parts was wrestling the Python bindings. I needed
more enable_if to skip establishing the missing methods of
integer-based vector classes. And the tests, oy the tests, they
required a bunch of logic to skip over the invalid calls for certain
types (though, luckily, it was already partly done -- whoever wrote
the Python unit tests at least recognized that some of these methods
were silly for integer-based types and skipped them in some cases).

Signed-off-by: Larry Gritz lg@larrygritz.com

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.

This is a big improvement, thanks!

Question, you say "Trick to..." here and there, without explaining the trick. Good magic show protocol ;) but here, maybe it makes sense to spell it out, like "Use enable_if to suppress..." or whatever makes sense? It wasn't clear to me if there was trick per se, as opposed to a possibly complex but idiomatic pattern?

`Vec<>::normalize*()` has very strange semantics for integer component
types -- if exactly one axis has a nonzero component, it is set to 1,
otherwise it throws an exception because there are no unit length
integer vectors having anything other than a single nonzero
components.

The presence of these (and in particular the possibility of their
throwing exceptions) is interfering with our goal of marking the
normalize methods as noexcept, even for float types where they don't
throw exceptions.

`Vec<>::length()` is also strange: for integer component types would
return to a rounded integer, which is also strange because this means
it would generally not be the correct Euclidean length of the vector
(except when coincidentally the components were Pythagorean triples).

I'm not sure when the existing semantics would be useful, so I'm
proposing just marking all of these as `= deleted` to make it a
compile-time error to try to use these nonsensical functions on
vectors of integral component type.  This allows us to eliminate the
ImathVec.cpp file, which only served to house the integer
implementations of these suspect methods.

Additionally, there were some other functions such as `project()` and
`orthogonal()` that depended on normalized vectors. So this patch
employs `enable_if` to ensure those templates can't be instantiated
for vectors of integral component type.

Because it's very handy to have a more readable wrapper around the
enable_if idioms, I also added that and a few other tidbits to
ImathPlatform.h (seemed like as good a place as any), which I propose
to be our standard place to put those bits of syntactic sugar that
need some tricky defines to make them work across different C++
standards (for example, that's where I make it use `std::enable_if_t`
for C++14 and beyond, but define it in the Imath namespace when using
C++11). I also moved the definition of IMATH_CONSTEXPR14 here as well,
as it seemed to go with the other things.

One of the hairiest parts was wrestling the Python bindings. I needed
more enable_if to skip establishing the missing methods of
integer-based vector classes.  And the tests, oy the tests, they
required a bunch of logic to skip over the invalid calls for certain
types (though, luckily, it was already partly done -- whoever wrote
the Python unit tests at least recognized that some of these methods
were silly for integer-based types and skipped them in some cases).

Signed-off-by: Larry Gritz <lg@larrygritz.com>

remove ImathCpp.h

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz
Copy link
Contributor Author

lgritz commented Dec 14, 2020

Yeah, it's just a standard C++ idiom. I've updated the commit comments to clarify that it's idiomatic, not that I'm employing some kind of unusual construct.

@lgritz
Copy link
Contributor Author

lgritz commented Dec 17, 2020

Would anybody else like to look this over or voice objections?

Copy link

@kthurston kthurston left a comment

Choose a reason for hiding this comment

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

Looks good to me. I was originally going to ask about ImathPlatform.h and whether we want the extra macros both for the c++ version (I would normally grep for __cplusplus checks when searching a library I don't know) and for the enable_if, but in reflecting on it for a moment, it does make the code much easier to read (both for compiler support checks and for just not having the enable_if template surrounding...

@lgritz
Copy link
Contributor Author

lgritz commented Dec 17, 2020

@kthurston It's a crutch I like... I find the __cplusplus values forgetful, I have to look up which precise numbers are which major releases (but "if THING >= 14" is easy), and I can never remember the precise enable_if idiom without looking it up. But I would understand if people wanted me to get rid of this stuff.

@lgritz lgritz merged commit e944991 into AcademySoftwareFoundation:master Dec 18, 2020
@lgritz lgritz deleted the lg-intvec branch December 18, 2020 20:36
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

4 participants