Imath
Imath copied to clipboard
Add simple base types
This defines "simple" data only types for imath which can be used in API or in C for data carriage. Then the vector, matrix, box classes are updated to use them as storage, or in the case of box, adapt between so min / max can have meaningful vector types since c++ doesn't allow a good type punning mechanism for that. vec and matrix are fine since we're specialising to either a general thing, or to a pod type, but not trying to use operators on the stored type itself beyond simple arithmetic.
NB: We could make the types simpler / more general if we were to change the api to template on things like just "vec", instead of "vec2<T, Store>", and then enable_if / whatever if the dimensions match expected, reuse the has_xy checks, or whatever...
I'm a little confused about how this is going to be used to make generic APIs. I think one of our goals should be to make it easy to do this:
void my_api_call (Vec3fLikeThing v); // or maybe `const Vec3fLikeThing& v`?
where Vec3LikeThing is not namespaced, hides which version of Imath is used by my_api_call, silently accepts an Imath::Vec3f. The imath_3f_t is not namespaced, but it's also C and has no constructors (from elements, or from Vec3, or from generic things). The Vec2StorageAdapter is namespaced (and behind detail
at that), so it will get mangled to something different for every major release, and it doesn't have any interesting constructors, so I'm not sure how you get that kind of data. The Vec3 itself has the right constructors and interop, but are still behind the namespaces like always.
I'm trying to do something similar (though maybe clumsily) here in this PR: https://github.com/OpenImageIO/oiio/pull/3330/files#diff-f8cdbabc1d9e89da4c55bc3b6faa5ace460bfa06653eb0e34938c37529d8b08aR228
Maybe there's some convergence of ideas that gets us all the desirable properties at once?
yeah, my idea was that you'd have some amazing function, say a function like this that is meant to modify something in place (how that is amazing, I'm not sure, but bear with me):
void my_cool_transform(imath_v3f_t &v);
then the implementation of that, you'd have:
void my_cool_transform(imath_v3f_t &inout) { IMATH_NAMESPACE::V3f &space = static_cast<IMATH_NAMESPACE::V3f &>( inout ); space[0] = 1.f + space.length(); space[1] = 2.f; space[2] = 3.14f; }
and you'd be done - the API has a stable type, never to change, but you can use the c++ object and operate on the same memory as the C type. I can certainly see adding a helper (ifdef'ed to only in c++) to the base type structs that adds the cast operators such that it does not require the above static_cast, such that you could then say
auto &space = inout.as_vec3(); // or something, not sure what would be most clear
(could also provide a helper like the static_cast is just more meaningful for imath)
and if someone wants to define a vec3 that uses a simd 4 storage, then can trivially do that, but you can't do the above reference thing (at least across page boundaries) without crashing, but is then separate register on the stack with a copy and a store at the end, which is more what it looks like you're doing with the vec3param stuff in your oiio path:
mysimdvec3f space = inout; // stuff inout = space;
further, I think I did one aspect of what you did in oiio for Imath::Box if you look at that one, where I add the cast operators to transform box into the C type (and back), but otherwise leaves box alone because for box, you really want min / max to be full-fledged classes when in c++. However, that requires the different usage I talk about above where you are not able to (really) operate on things by reference with out a reinterpret_cast (we could add a routine that does a reinterpret / type punning, which I started to get to and is the part of the CI that is failing on older compilers where I'm validating that is safe). As an aside, it does bother me that I've gone one path for box, another for vec / matrix, so would like to figure out what's best.
However, long story short, I thought we wanted to get to data-only carriers of data to be used for public API, and an easy way to reference that data in place (i.e. when looping over a std::vector of V3f) and operate on it with the power of C++. But the other direction, things like the below should also work:
using IMATH_NAMESPACE; V3f foo; exr_get_v3f_attr( "foo", &foo ); my_cool_transform( foo );
as well as the inplace manipulation I mention above...
so this is an attempt to create that, I am happy for other ways and thoughts!
Yeah, but what does the caller look like?
I think we won't want it so that the caller has to say static_cast<imath_v3f_t&>(blah)
for every param that is conceptually a V3f.
Yeah, but what does the caller look like?
I think we won't want it so that the caller has to say
static_cast<imath_v3f_t&>(blah)
for every param that is conceptually a V3f.
I'm not sure I understand? in my branch, V3f ends up derived from the imath_v3f_t type (and matrix is analogous), so if you have an api call that takes a imath_v3f_t, it just naturally passes (casting to base class is transparent, it's the downcast that is not), so you don't have to do anything, you just call the routine.
V3f someparam = { 1.f, 0.f, 0.f };
oiio_setNormal( someparam );
where setNormal might be defined as
void oiio_setNormal( const imath_v3f_t &v );
am I missing what you're asking? I agree that it is a confusing that as a user of your oiio library, I have to know that when I see imath_v3f_t, that maps to V3f that I can use to have all the cool functionality. I think what you're getting after is that something like if instead (assuming it's a c++ routine), we have the above defined as
void oiio_setNormal( const imath::param::v3f & );
that might clue people in more that the types are portable between each other?
I'm also ok with literally just removing most of what I've done here, and getting rid of the versioned namespace from Imath entirely, and while there might be a C struct defined for use in C contexts, and some portability between the two as I have, largely, it could just become
void oiio_setNormal( const imath::V3f &v );
with no shim type in between...
Oh, I see, yes. I think I was missing that because V3f was derived from imath_v3f_t, that means you can pass a V3f to it. I was just looking at imath_v3f_t and thinking "that has no constructors at all". I get it now.
At this point, I assume this PR is stale, and anything we'd do from here probably deserves a fresh start? I'm going to close this so it no longer appears to need attention, but it's obviously fine to re-open or re-submit if this is an approach we want to pursue.