Conversion of (partially) fixed size Eigen matrices

[remcodevos]

Hi,

I've a question regarding the Eigen integration and an issue I'm experiencing with the conversion between Corrade::StridedArrayView2D and dynamic Eigen matrices: is there a specific reason why both the rows and columns need to be dynamic for the conversion?

magnum-integration/src/Magnum/EigenIntegration/DynamicMatrixIntegration.h

Lines 123 to 145 in 50ac4db

	/**
	@brief Convert an Eigen expression to @relativeref{Corrade,Containers::StridedArrayView2D}
	@m_since_latest
	Takes care of any Eigen expression that was not handled by the one-dimensional
	overload above.
	*/
	template<class Derived> inline typename std::enable_if<
	#ifndef DOXYGEN_GENERATING_OUTPUT
	Eigen::internal::traits<Derived>::ColsAtCompileTime == Eigen::Dynamic && Eigen::internal::traits<Derived>::RowsAtCompileTime == Eigen::Dynamic
	#else
	...
	#endif
	, Containers::StridedArrayView2D<typename Derived::Scalar>>::type arrayCast(const Eigen::DenseCoeffsBase<Derived, Eigen::DirectWriteAccessors>& from) {
	return {
	/* We assume that the memory the Eigen expression is referencing is in
	bounds, so the view size passed is ~std::size_t{} */
	{const_cast<typename Derived::Scalar*>(&from(0,0)), ~std::size_t{}}, {std::size_t(from.rows()),
	std::size_t(from.cols())},
	{std::ptrdiff_t(from.rowStride()*sizeof(typename Derived::Scalar)),
	std::ptrdiff_t(from.colStride()*sizeof(typename Derived::Scalar))}
	};
	}

I understand the specialisation to return a Corrade::StridedArrayView1D for the scenario where either the row or the column is one-dimensional. However, for the scenario where you'd have an Eigen matrix with only dynamic rows and a fixed number of columns at compile time - or the other way around with fixed rows and dynamic columns - the template function does not match as the Eigen::Dynamic check fails for the fixed dimension. The same issue arises for Eigen matrices with fixed size that are not one-dimensional, e.g. Eigen::Matrix<float, 128, 128>. In those cases no function is available to perform the conversion:

Eigen::Matrix<float, 128, 128> matrix;
auto view = Magnum::EigenIntegration::arrayCast(matrix);

error: no matching function for call to ‘arrayCast(Eigen::Matrix<float, 128, 128>&)’

Looking at the existing implementation it seems to me that it should work for those scenarios, and relaxing the check to only ensure that the one-dimensional specialisation does not apply makes it possible to use the conversion for these types of matrices: Eigen::internal::traits<Derived>::ColsAtCompileTime != 1 && Eigen::internal::traits<Derived>::RowsAtCompileTime != 1.

However, I'm not 100% sure that I'm understanding the implementation correctly or missing something else related to the conversion that makes the Eigen::Dynamic required and instead another specialisation is necessary for (partially) fixed size matrices.

[mosra]

Hi,

what you suggest makes sense. Unfortunately I don't remember the exact reason why it was done like this, it's possible that it was due to being adapted from the conversion between fixed size Eigen matrices and (fixed size) Math types, so #74 just implemented it "for the other set of types". It's likely that removing such a restriction will make the code shorter and simpler -- as far as I can see, there's no case where this would conflict or cause ambiguous overloads.

I'm not sure when I'll be able to look at this, however, it'll likely take some weeks. If you want to try to update the code and send a PR, that'd be much appreciated -- you likely have a much better understanding of Eigen API than I do, I use it so seldom that I have to re-learn it every time. There are tests for the dynamic variants, so it'd be about extending them to all the static / static+dynamic combinations and making sure it works the same way for all.

Otherwise, if you can wait until I get to this, I think something like the following could work? I.e., converting to a dynamically-sized matrix before passing it to arrayCast(). I hope it doesn't cause a massive memory copy tho, that'd be silly. Or if it does, would there be an equivalent with an Eigen::Map or some such?

Eigen::Matrix<float, 128, 128> matrix;
Eigen::MatrixXf dynamicMatrix = matrix;
auto view = Magnum::EigenIntegration::arrayCast(dynamicMatrix);

[remcodevos]

Unfortunately I'm not an Eigen expert either, I'd have to look a bit deeper into the layout of the Eigen and Magnum data to make sure. However, what you mentioned and linked in #74 regarding the stride does suggest that it should not be an issue.

I'm limited on time as well but I'll see if I can find some time to familiarize myself with the project and the tests, and see if I can contribute a fix.