cginternals / globjects

C++ library strictly wrapping OpenGL objects.
https://globjects.org
MIT License
538 stars 59 forks source link

How to use Eigen instead of glm as base math lib #385

Closed dlyr closed 3 years ago

dlyr commented 5 years ago

Hi, I work on a project using Eigen as base vector types (for many reasons, I had to do this). I also use glbindings and globjects as graphics library (because they are very useful ;))

Currently I do conversion from Eigen to glm vec each time needed, i.e.

inline glm::vec3 toGlm( const Eigen::Vector3f& v ) {
    return glm::vec3( v( 0 ), v( 1 ), v( 2 ) );
}
void MyShaderProgramClass::setUniform( const char* name, const Eigen::Vector3f& value ) const {
// m_program is a globjects::Program
    m_program->setUniform( name, toGlm( value ) );
}

As I understand, if I want to add a native use of Eigen, I had to rewrite AbstractUniform and the corresponding implementation.

Do you have an advice on how to handle this flawlessly ? I could then PR my implementation if valuable for you.

sbusch42 commented 5 years ago

Well yes, as I see it, you would need to adapt AbstractUniform, AbstractUniformImplementation, UniformImplementation_Legacy, and UniformImplementation_SeparateShaderObjectsARB. It shouldn't be hard to do that, just some repetitive work :)

I think the following should be done for a pull request we can accept: [ ] Add an option OPTION_USE_EIGEN in the main CMakeLists.txt (OFF by default) [ ] If option is ON, find_package(Eigen) (depending on how eigen works with cmake, a cmake find script might be necessary, if so, add that to the cmake directory and make sure it finds the lib on all platforms) [ ] If option is ON and package is found, add C++-definition GLOBJECTS_USE_EIGEN using target_compile_definitions [ ] Use ifdefs in code to include the eigen-specific uniforms only when that definition is set

Please let me know if you need more help/explanation regarding this issue.

dlyr commented 5 years ago

Hi, I finally find time to start this project. I got one issue (because I'm lazy and want to avoid as much as possible copy past edit) Is it a good idea to reference implementation in a inl file ? Because I want to do something like

template<typename Derived>  void AbstractUniform::setValue(gl::GLint location, Eigen::DenseBase<Derived>& value) const { implementation().set(m_program, location, value); }

Instead of overload setValue for each Eigen types.

Moreover, could you tell me if glbinding or globjects defines macro such as GL_VERSION_2_1

All in all, if I manage to fix these design issue, the adaptation is not so hard and I hope to do it quickly. One question is also how to test, do you have a testing approach I can use to check all plateforms compatibility ?

Thanks in advance.

dlyr commented 5 years ago

I have done the firsts steps (glUniform) and I will continue to add Eigen alternative for other ref of glm. Nevertheless, I found curious to have UniformTypeHelper return UniformType::UNSIGNED_INT for glm:mat2 and other matrices. Is that on purpose ? see here https://github.com/cginternals/globjects/blob/dc68b09a53ec20683d3b3a12ed8d9cb12602bb9a/source/globjects/include/globjects/AbstractUniform.inl#L176

dlyr commented 5 years ago

Any comments on that ? I could do a pre PR to show what I have done ... even if not finished.

scheibel commented 5 years ago

Do you have a repository where I can have a look at it? At best, I can check it out and play around a bit.

dlyr commented 5 years ago

For sure, it's here https://github.com/dlyr/globjects/tree/eigen (note the Eigen branch)

scheibel commented 5 years ago

I had a look at the eigen branch. I like the integration approach. During my review, I removed some unnecessary CMake code and performed some code style guide adjustments. As I couldn't push to your branch directly, I uploaded the results to a new branch.

Do you need the unfinished vector variants? I think the current state is valuable even without a full Eigen integration. I opened a draft pull request at https://github.com/cginternals/globjects/pull/390.

dlyr commented 5 years ago

Thanks for your reply,

Well, due to alignement control in Eigen, you may have issue with std::vector (see https://eigen.tuxfamily.org/dox/group__TopicStlContainers.html ) That's why I didn't include it yet.

What about my question on UniformHelper ?

Btw I do not check if I forgot some usage of glm without having Eigen equivalent, but I have no time to do it until at least one week.

scheibel commented 5 years ago

The UniformType is the internal representation of a data type whose values can be passed as a uniform to shaders. The UniformHelper is a helper data structure that derives this value from a given C++ type during compile-time. What you found is actually an implementation error no one catched until now. Thanks for that. We'll cover the fix of the matrix values in issue #391.

dlyr commented 5 years ago

Sorry for the delay, we will test this version with our rendering engine and I will confirm if it works as expected. In the meantime, review are welcome.

scheibel commented 3 years ago

The basic support is added with #390. If issues arise or anyone is missing features with respect to the eigen integration, please open new issues. Closing this for now.