AcademySoftwareFoundation / Imath

Imath is a C++ and python library of 2D and 3D vector, matrix, and math operations for computer graphics
https://imath.readthedocs.io
BSD 3-Clause "New" or "Revised" License
356 stars 104 forks source link

IMATH_HOSTDEVICE for hsv2rgb() ? #370

Open AlexeySmolenchuk opened 4 months ago

AlexeySmolenchuk commented 4 months ago

I've just tried to use Imath as a vector library for CUDA. I'm not sure if it's supposed to work like this, but I saw some promising comments in the source, so I've tried.

All hsv2rgb() in ImathColorAlgo.h are marked as IMATH_HOSTDEVICE but rely on hsv2rgb_d() which is a host-only function. Please correct me if I'm wrong.

If it's some porting is currently ongoing, it would be very helpful to see which parts of the library are already supported by CUDA.

Thank you, Alexey

oxt3479 commented 4 months ago

That would be an oversight on my part. Right now I am determining why hsv2rgb_d() was not marked IMATH_HOSTDEVICE in the first place. Depending on what reason I discover I can either implement an alternative that can support CUDA or make sure hsv2rgb() is not marked IMATH_HOSTDEVICE.

AlexeySmolenchuk commented 4 months ago

why hsv2rgb_d() was not marked IMATH_HOSTDEVICE in the first place

Because it's implemented in separate cpp I guess. Another candidate would be ImathMatrixAlgo.cpp

I see this old investigation task about the header-only version started https://github.com/AcademySoftwareFoundation/Imath/issues/13 unfortunately without any outcome.

meshula commented 4 months ago

ImathMatrixAlgo.cpp only really needs to have the explicit instantiations (https://github.com/AcademySoftwareFoundation/Imath/blob/edb64a8507ab5d6e4c0b3b11c49b01c4e5a9bb8d/src/Imath/ImathMatrixAlgo.cpp#L1241 ) defined in it; it could be moved to a header which might resolve the header only goal. I think it's in fact the case that we don't need the instantiations any more, either. They were originally implemented like that to solve for limitations of VS2005, which have long since been resolved, to my knowledge.

It would be harmless to move ColorAlgo to a header at this point as well.

13's outcome was in fact that the majority of Imath did become header only; there's just a few outliers left to tackle.