Interoperability constructors #91
Conversation
|
I'm a little unsure about my having marked these as constexpr14 and noexcept -- if people feel that it's is an unnecessary constraint to require the foreign vector's operator[] be constexpr and not throw exceptions, then we can relax this. |
|
You may see from my |
src/ImathTest/testVec.cpp
Outdated
| // Test construction/assignment/paramater pass of a C array | ||
| { | ||
| float s[3] = { 1, 2, 3 }; | ||
| Vec3<float> v(s); |
There was a problem hiding this comment.
Much as I would love something like this for interoperation with my C code, I kind of wonder if it's an unsafe dead end, because the float[3] might not be reasonably distinguishable from a float*. I'd be happy enough if I got magic bridging for typedef struct { float x, y, z; } V3f;
There was a problem hiding this comment.
Unfortunately a subscript operator is not going to play out happily for this common C idiom for lightweight n-vecs
There was a problem hiding this comment.
I think I can make it work with a different (second) construct that looks to see if there's a .x, .y, .z of the right types.
There was a problem hiding this comment.
After my next meeting is over, I'll give it a stab.
|
BTW, I think your proposal will work with Eigen, certainly that would be a good robustness check versus possibly the most complicated V3f declaration in the Known Universe. |
|
I'm still tinkering, trying to get it to correctly recognize either things with .x, .y, .z, or with operator[] -- without ending up with an error for a redundant declaration for a class that has both. I'm sure this can be cracked, but it's definitely keeping me on the uncomfortable bleeding edge of my understanding of template metaprogramming. |
|
This is great to see, Larry, and I'm looking forward to integrating this approach into MaterialX as well. Responding to your thoughts above, I would agree that it would be ideal to minimize the restrictions on the foreign vector's [] operator, which may not contain noexcept or constexpr markings. As Nick mentions, testing against Eigen would be a strong proof-of-concept, since its vector/matrix classes are so fundamentally different from Imath. For helpers such as IMATH_VECTOR_EQUIVALENT, it might be more idiomatic to use an alias template (e.g. |
|
Thanks for the tips. Yes, I'm working on those templates now. Got frustrated with template error messages last night, hopefully fresh eyes today will let me figure out how to make it happy. |
|
Got it! Including using C arrays, classes that have both named members and subscripts, and a way to override false positives and negatives. Will update as soon as I'm done with dentist appointment. |
|
Updated with a revised approach:
I still have only prototyped for Vec3, if people like the design, I will |
lgritz
force-pushed
the
lg-genericvec
branch
from
120a649 to
5a9e682
Compare
|
If any of you have custom vector types lying around in your garage, please try this patch out and verify that it works to allow you to assign your vector type to a V3f, and to pass your vector type to a function that takes a |
|
Love it! Here's another goofy thing to add to the tests as a case which should trait out as NO. I'm curious how the tests hold up when functions exist named as the components. template<typename T>
struct AccessedV3 {
T x() const;
T y() const;
T z() const;
}; |
Already was there -- look in testInterop.cpp for the |
|
Oops, I seem to be failing CI for some compilers, will need some minor adjustments, will fix. |
lgritz
force-pushed
the
lg-genericvec
branch
6 times, most recently
from
56d931d to
29d33dc
Compare
|
Oh! I missed that when I read the diff. Eigen supports subscripting and the x() syntax, so I think subscripting covers it. I don't know of other vector libs that do that particularly, so I don't know of a reason to capture it as a Yes case. |
|
@meshula If you're a regular Eigen user, can you give this a quick try and see if it works? I could fumble around with it, but still not be sure I was using the idioms you care about, and I'm sure what would take me hours to set up is something you can do in minutes if Eigen is in your daily toolbox. |
|
Should we merge this as is and continue to iterate on it with subsequent PR's? Might be easier to test that way, and it's not likely to break anything. |
|
I consider this incomplete, and I'd really like to extend this work to the other classes before we merge anything. Does everybody know about the GitHub command line app? will correctly figure out this PR (#91), pull from my tree into your local copy, and let you try it out. No need to even set up remotes by hand or know where my repo is. There is no need to merge into our main repo as a prerequisite for people trying it out super easily. |
|
I'll flesh this out to the other classes over the weekend and turn into a full PR. For this draft, I was seeking to give enough of a peek that people could give feedback (pro or con) on the basic idea, do we want this, does the design seem sane, etc. I think today's iteration is looking pretty good (IMHO) but this is a new idiom for us and I don't want to force people into it if they think it unwise or have reservations about my approach to implementation. |
lgritz
force-pushed
the
lg-genericvec
branch
from
29d33dc to
2fdc8d8
Compare
|
I don't quite understand how the new constructors are related to the string initialization. I'm fine with disabling for gcc4, which in my world anyway is about to become irrelevant within weeks (at the very least, anything still depending on it is locked down on legacy versions of the whole stack and also would not be upgrading to Imath3). If we continue to have lingering suspicions of these new constructors, I'm very happy to do something like this: and that give an escape valve for somebody to if they are building software or using a compiler that has trouble and just wants to disable the interop constructors. I'm also thinking of declaring them as |
|
I encountered the problem while building Field3D, then reconstructed the offending construct inside testInterop.cpp, where simply adding the map<string,v3f> leads to the error. It looks like it's getting confused inside the has_xyz<> template. The complete error is below. This is gcc 4.8.5 with -std=c++1y. Since the same code works with gcc6, it does look like a compiler bug. Providing an option to opt out seems like a good idea. These constructs are fancy enough that they may well trip up other compilers we're not explicitly validating. In file included from /usr/include/c++/4.8.2/bits/stl_map.h:63:0, |
|
OK, two questions, then:
|
2.The failure I'm seeing depends on GCC4, which claims to support c++11 but apparently doesn't. Since the code conforms to C++11 as written, it seems reasonable to disable based on GCC4. I'm also seeing similar sorts of failures in code that has to be compiled with CUDA 6.2. |
|
Updated the PR:
|
lgritz
changed the title
|
I would request that you try again with these modifications and see if your code base is safe. Assuming that is true, I stand by this version of the PR and you can merge when it passes your test. |
|
So this is cool - type traits are cool. However, I question this change a bit. I worry about unintended object slicing because we are implicitly doing this - basically allowing people to be too loose with their types. I get that this is the point, but I worry about putting a Vec4 into a Vec3 and people not projecting that in explicitly to say "yes, I know, it's fine". Second, the sizeof check in the has_subscript stuff seems ... flawed. It is bypassing too much of the type system to my mind to be a viable thing (the has_xyz stuff is good stuff, given the above caveat). For example, if I do: struct MyPainInTheNeck float x; this will pass the "has_subscript and ! has_xyz" check for Vec3, right? And not even crash. But I contend that the z value would not be what one might expect :) So, I like the idea of easier type coersion, but it seems like we are running too fast and loose and we run the risk of causing more pain for people than we should. Should we change the has_subscript stuff to not do sizeof checks for the default implementation, but instead check if the type has a member function called size, returning an integral type, and instead only do the sizeof checks in the template specialization for the Base[N] implementation (nothing says they need to have the same implementation)? Then you would probably have to make an adapter function to access values, where the one checks the size first... But this then should work for vector, et al. without further specialization, right? Another option this to make these constructors marked as explicit, such that someone has to explictly say "yes, I want you to do the conversion". It'd at least still be foo = Vec3( othertype ); instead. Finally, might want to add support for tuples, and use std::tie to extract values, at least for the vec types, such that you can do std::tie( x, y, z ) = foo, where foo is of type std::tuple<T, T, T> for a vec3, or similar. I don't know if that helps much for the above, I think if anything it would just move the problem, but might be a handy variant to support. |
|
@kdt3rd -- yeah, I think explicit is the safer way to go. I will fix that today. |
|
Pushed update -- explicit interop constructors only. |
I have submitted a PR to Imath that adds "interopability" constructors and assignment, that use some templates to allow seamless construction, assignment, and passing of "foreign" vector types to Imath vectors. This makes it easier library A that uses Imath vector types in its API to interact with app or library B that has a custom internal vector type, without a ton of ugly casts and copies. (See AcademySoftwareFoundation/Imath#91) These created a few minor conflicts with OIIO's internal SIMD classes and their construction and casting from Imath types. This patch cleans up the ambiguities: * Make Imath::V4f -> simd::vfloat4 and Imath::M44f -> simd::matrix44 constructor `explicit`. * Make the simd types `simd()` method (returning the underlying raw SIMD type) also have a variety that returns a ref to a non-const vector, to ensure proper working of the _MM_TRANSPOSE4_PS macro. Now I am going to commit a sin. This MUST get backported to OIIO 2.2 in order for it to not break against the upcoming Imath 3.0. This doesn't break ABI compatibility, nor change OIIO's primary advertised public APIs, but it does change the technically/informally public APIs of simd.h in ways that introduce the possibility that an app using those simd vector classes *might* need minor source code editing if they relied on those implicit constructors. Such software might need certain `simd::vector4 foo = imath_bar` to become `foo = vector4(bar)` and the same for certain matrix44 assignments. It is usually a big no-no to backport any change that could require editing of source code. But since the alternative is not being able to use the new Imath, I think this is the lesser evil, and maybe nobody out there is doing the thing that would need to be changed?
…emySoftwareFoundation#2878) I have submitted a PR to Imath that adds "interopability" constructors and assignment, that use some templates to allow seamless construction, assignment, and passing of "foreign" vector types to Imath vectors. This makes it easier library A that uses Imath vector types in its API to interact with app or library B that has a custom internal vector type, without a ton of ugly casts and copies. (See AcademySoftwareFoundation/Imath#91) These created a few minor conflicts with OIIO's internal SIMD classes and their construction and casting from Imath types. This patch cleans up the ambiguities: * Make Imath::V4f -> simd::vfloat4 and Imath::M44f -> simd::matrix44 constructor `explicit`. * Make the simd types `simd()` method (returning the underlying raw SIMD type) also have a variety that returns a ref to a non-const vector, to ensure proper working of the _MM_TRANSPOSE4_PS macro. Now I am going to commit a sin. This MUST get backported to OIIO 2.2 in order for it to not break against the upcoming Imath 3.0. This doesn't break ABI compatibility, nor change OIIO's primary advertised public APIs, but it does change the technically/informally public APIs of simd.h in ways that introduce the possibility that an app using those simd vector classes *might* need minor source code editing if they relied on those implicit constructors. Such software might need certain `simd::vector4 foo = imath_bar` to become `foo = vector4(bar)` and the same for certain matrix44 assignments. It is usually a big no-no to backport any change that could require editing of source code. But since the alternative is not being able to use the new Imath, I think this is the lesser evil, and maybe nobody out there is doing the thing that would need to be changed?
|
With regards MaterialX, construction works, but assignment doesn't. There may be other issues, but this seems like a good one to look at before going deeper. MatX vector hides the data in a std::array<T, N>. Here's an example of the output for assignment and a trivial repro |
|
Can you try with the latest, in which I've made the constructors explicit? |
This is a design review, feedback requested before I proceed further.
Trying out some use of enable_if to give `Vec3<T>` constructor and
assignment from unknown types that "look like" 3-vectors -- they have
a `operator[]` that returns a T, and their size is at least
3*sizeof(T).
The main idea is that if an app has rolled its own 3-vector class, it
can seamlessly construct a Vec3 from it, assign a Vec3 to it, or pass
their type as a parameter that expects a Vec3. And if the app's custom
3-vector class employs an equivalent idiom, all those operations will
work in the reverse direction.
It also works for std::vector, std::array, and even "initializer lists",
so all of this would work:
// Your app's special snowflake custom 3-vector type.
class YourCustomVec3 { ... };
// My library has an API call where I use Imath::Vec3 as a parameter
// passing convention, because I don't know about your vector.
void myfunc (const Vec3<float>& v) { ... }
// All sorts of things people may think of as "a vector"
YourCustomVec3 myvec(1, 2, 3);
std::array<float,3> arr { 1, 2, 3 };
std::vector<float> stdvec { 1, 2, 3 };
myfunc(yourvec); // ok
myfunc(arr); // yep
myfunc(stdvec); // hunky-dory
myfunc({ 1.0, 2.0, 3.0 }); // yeah, even that
This is only prototyped for Vec3 for now, but if you all like this
idea, then I will complete the work to do this for the other vector
and matrix classes.
Signed-off-by: Larry Gritz <[email protected]>
Different approach:
* New file ImathTypeTraits.h now collects all the type traits (including
a couple things we had previously put in ImathPlatform.h).
* New traits: has_subscript, has_xy, has_xyz, has_xyzw.
* New test file: testInterop.{h,cpp}
* Plain C arrays[3] work now.
* Classes that have .x, .y, .z member variables (but no subscripting)
work now.
* Classes that have BOTH subscripting and named members work!
* It's easy to correct false positives and false negatives.
I still have only prototyped for Vec3, if people like the design, I will
extend to the other relevant types.
Signed-off-by: Larry Gritz <[email protected]>
Also change the ones that require subscripting to non-constexpr, non-noexcept, to accommodate interop with foreign types whose operator[] is not constexpr or noexcept. I have mixed feelings about this; most sane implementations *ought* to be constexpr and noexcept, but I can imagine that many are not so declared even though they could be. I have left the .xyzw direct accessors constexpr and noexcept, because simple assignment of these member variables ought to be safe. Removed unused isSameType templates (if needed, we should use `std::is_same<>`). Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
* Disable interop if the symbol IMATH_FOREIGN_VECTOR_INTEROP is defined to be 0 prior to including any Imath headers. * Also disable if we detect gcc 4.x, which seems to have a bug that prevents this idiom from compiling correctly. * Corral the interop functionality into a documentation "group" and beef up the description. Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
|
I just rebased this to the current master. We decided in TSC to go ahead and merge so we include this in 3.0, right? |
|
Yes, go for it.
…On Fri, Mar 12, 2021 at 7:12 PM Larry Gritz ***@***.***> wrote:
I just rebased this to the current master. We decided in TSC to go ahead
and merge so we include this in 3.0, right?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#91 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFC3DGOS67TRMETDMT2Q4ZLTDLCXVANCNFSM4XJVGKKA>
.
--
Cary Phillips | R&D Supervisor | ILM | San Francisco
|
|
OK, will do as soon as the CI is done. |
This is a design review, feedback requested before I proceed further.
Trying out some use of enable_if to give
Vec3<T>constructor andassignment from unknown types that "look like" 3-vectors -- they have
a
operator[]that returns a T, and their size is at least3*sizeof(T).
The main idea is that if an app has rolled its own 3-vector class, it
can seamlessly construct a Vec3 from it, assign a Vec3 to it, or pass
their type as a parameter that expects a Vec3. And if the app's custom
3-vector class employs an equivalent idiom, all those operations will
work in the reverse direction.
It also works for std::vector, std::array, and even "initializer lists",
so all of this would work:
This is only prototyped for Vec3 for now, but if you all like this
idea, then I will complete the work to do this for the other vector
and matrix classes.
Signed-off-by: Larry Gritz [email protected]