Boost C++ Libraries: Ticket #8362: Move ctor or assignment for boost::geometry::model::ring?

owner changed

Barend Gehrels — Fri, 05 Apr 2013 21:20:55 GMT

owner changed from Barend Gehrels to Mateusz Loskot

Wed, 10 Apr 2013 08:13:32 GMT

Some additional information. *this is a multi-polygon with the following definition:

template<typename T>
class _TPolygon :
	public std::vector< boost::geometry::model::polygon< _TPoint< T >, /*ClockWise*/false, /*Closed*/false > >,
	boost::equality_comparable< _TPolygon<T>,
	boost::additive< _TPolygon<T>, _TSize<T> > >
{ ... }
namespace boost { namespace geometry { namespace traits {
	template<typename T>
	struct tag< _TPolygon<T> >
	{
		typedef multi_polygon_tag type;
	};
} } } // namespace boost::geometry::traits

The polygon definition is based on our _TPoint<T> type which is declared for use with boost::geometry as follows:

namespace boost { namespace geometry { namespace traits {
	template <typename T>
	struct tag< _TPoint<T> >
	{
		typedef point_tag type;
	};
	template <typename T>
	struct coordinate_type< _TPoint<T> >
	{
		typedef T type;
	};
	template <typename T>
	struct coordinate_system< _TPoint<T> >
	{
		typedef cs::cartesian type;
	};
	template <typename T>
	struct dimension< _TPoint<T> >
		: boost::mpl::int_<2>
	{};
	template <typename T, std::size_t Dimension>
	struct access< _TPoint<T>, Dimension >
	{
		static inline T get(_TPoint<T> const& p)
		{
			return p[(EOrientation)Dimension];
		}
		static inline void set(_TPoint<T>& p, T const& value)
		{
			p[(EOrientation)Dimension] = value;
		}
	};
} } } // namespace boost::geometry::traits

Similarly, we have our own _TRect<T> type which we plug into boost::geometry:

namespace boost { namespace geometry { namespace traits {
	template <typename T>
	struct tag< _TRect<T> >
	{
		typedef box_tag type;
	};
	template <typename T>
	struct point_type< _TRect<T> >
	{
		typedef _TPoint<T> type;
	};
	template <typename T, std::size_t Dimension>
	struct indexed_access< _TRect<T>, min_corner, Dimension >
	{
		typedef typename geometry::coordinate_type< _TPoint<T> >::type coordinate_type;
		static inline coordinate_type get(_TRect<T> const& rect)
		{
			return geometry::get<Dimension>( rect.TopLeft() );
		}
		static inline void set(_TRect<T>& rect, coordinate_type const& value)
		{
			geometry::set<Dimension>( rect.TopLeft(), value );
		}
	};
	template <typename T, std::size_t Dimension>
	struct indexed_access< _TRect<T>, max_corner, Dimension >
	{
		typedef typename geometry::coordinate_type< _TPoint<T> >::type coordinate_type;
		static inline coordinate_type get(_TRect<T> const& rect)
		{
			return geometry::get<Dimension>( rect.BottomRight() );
		}
		static inline void set(_TRect<T>& rect, coordinate_type const& value)
		{
			geometry::set<Dimension>( rect.BottomRight(), value );
		}
	};
} } } // namespace boost::geometry::traits

In our code, these classes are instantiated with template parameter T being either int or double. We have a lot of use for int-based geometry calculations.

With these geometry types we successfully call many algorithms from boost::geometry, e.g.,

template<typename T>
_TPolygon< T >::_TPolygon(_TRect< T > const& rect)
{
	boost::geometry::convert( rect, *this );
}

but the following does not seem to work (as of boost 1.52.0):

template<typename T>
_TPolygon< T >::_TPolygon(std::vector< _TPoint< T > > vecpt)
{
	// vecpt:
	// - No need to add end==begin point.
	// - Orientation is mathematically counter-clockwise (i.e., clockwise with the
	//   origin in the upper left as is the case with screen or slide coordinates).
	// - No self-intersection allowed!
	if( !vecpt.empty() ) {
		boost::geometry::convert( polygon_type::ring_type( std::move(vecpt) ), *this );
	}
}

Mateusz Loskot — Mon, 15 Apr 2013 00:26:21 GMT

Barend,

The requested move semantic is one issue, but unless I'm missing something, is the ring model supposed to be constructible directly from compatible container of compatible points? The extension constructor in ring takes iterators only, not container.

Obviously, this does not compile:

#include <vector>
#include <boost/geometry.hpp>
#include <boost/geometry/geometries/ring.hpp>
#include <boost/geometry/geometries/point_xy.hpp>
int main()
{
    typedef boost::geometry::model::d2::point_xy<double> point;
    typedef boost::geometry::model::ring<point> ring;
    std::vector<point> r1;
    ring r2;
    boost::geometry::convert(r1, r2);
    return 0;
}

Shall I extend the ring to make it constructible from the container too?

Barend Gehrels — Mon, 15 Apr 2013 06:08:44 GMT

OK for me to add a constructor with a container with its point types. It was not there because vector/deque do not have it neither.

Does this solve the whole issue? In the reported problem, std::move seems not necessary in the call to convert...

Mon, 15 Apr 2013 08:24:27 GMT

Replying to mloskot:

The requested move semantic is one issue, but unless I'm missing something, is the ring model supposed to be constructible directly from compatible container of compatible points? The extension constructor in ring takes iterators only, not container.

Replying to barendgehrels:

OK for me to add a constructor with a container with its point types. It was not there because vector/deque do not have it neither.

Although to me this seems to be a desirable extension, I am not saying that this is the only way to solve the issue. Leaving constructors alone, you could alternatively overload the convert algorithm to support conversion from vector-of-points to (Multi-)Polygon:

boost::geometry::convert( std::move(vecpt), *this );

My suggestion to call convert by way of constructing a ring_type first was merely based on my observation that convert already supports conversion from Ring to Polygon.

Replying to barendgehrels:

Does this solve the whole issue?

There are two more niceties I would like to suggest to solve the whole issue:

In boost 1.52.0, convert supports Ring to Polygon conversion, but does not support (any Geometry that supports conversion to Polygon) to Multi-Polygon conversion. Is this by design, or is this considered a missing feature?

While we are at it, is it intentional that I have to guard the conversion with "!vecpt.empty()"? I don't see a problem with creating a Ring from an empty vector-of-points and convert that to a Polygon that consist of empty vectors.

Replying to barendgehrels:

In the reported problem, std::move seems not necessary in the call to convert...

Can you explain why you think std::move is unnecessary there? It may in fact be useless if there is any link in the chain of calls that does not support move semantics, but from my perspective I should still put it there to enable move semantics where it is supported. There seems to be consensus that the compiler must not automatically turn a copy into a move. Thus I think I have to be explicit about my intent to use move semantics.

version changed

Fri, 14 Nov 2014 16:35:16 GMT

version Boost 1.52.0 → Boost 1.57.0

Fri, 14 Nov 2014 16:39:17 GMT

As far as I understand, I still cannot write an expression like this in 1.57.0:

boost::geometry::convert( polygon_type::ring_type( tc_move(vecpt) ), *this );

I'm not saying that this is a major problem or anything, just updating the ticket.

vschoech@… — Mon, 21 Sep 2015 15:39:37 GMT

This is still true in 1.59.0.