Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Writing 3D WKT:Z prefix #782

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Fixed Writing 3D WKT:Z prefix #782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df04b36
Fixed Writing 3D WKT:Z prefix
Siddharth-coder13 Dec 9, 2020
ff1f245
Added WKT:Z prefix test case
Siddharth-coder13 Dec 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion example/01_point_example.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ int main()
assign_values(d3a, 1, 2, 3);
assign_values(d3b, 4, 5, 6);
d3 = distance(d3a, d3b);

std::cout << wkt(d3a) << std::endl;


// Other examples show other types of points, geometries and more algorithms
Expand Down
5 changes: 5 additions & 0 deletions include/boost/geometry/io/wkt/detail/prefix.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ struct prefix_point
static inline const char* apply() { return "POINT"; }
};

struct prefix_point_z
{
static inline const char* apply() { return "POINT Z"; }
};

struct prefix_polygon
{
static inline const char* apply() { return "POLYGON"; }
Expand Down
9 changes: 6 additions & 3 deletions include/boost/geometry/io/wkt/write.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,14 @@ struct double_closing_parenthesis
/*!
\brief Stream points as \ref WKT
*/
template <typename Point, typename Policy>
template <typename Point, typename Policy1, typename Policy2>
struct wkt_point
{
template <typename Char, typename Traits>
static inline void apply(std::basic_ostream<Char, Traits>& os, Point const& p, bool)
{
os << Policy::apply() << "(";
if(dimension<Point>::type::value == 3) os << Policy2::apply() << "(";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note about style here, there should be an empty space between if and (, there are not one line ifs etc. You may want to consult https://github.com/boostorg/geometry/wiki/Guidelines-for-Developers

else os << Policy1::apply() << "(";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: do we always want a Z in our 3D point? Until now we just did without and it works without.
If we suddenly add a Z here it might be incompatible for users (if any) who use this functionality. So it's a breaking change.

Apart from this it should be a compile time condition i/o a runtime condition. So we should dispatch based on dimension. But I'm not sure about that either, because if it is optional (as it probably should be), that might be inconvenient.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see the specification indeed mentions a Z for three dimensional points. It's unclear if a three dimensional point without a Z is acceptable or not.
But what do we do with four dimensional points? Or even more? We've not much support for them of course, but we can calculate distances for them...

In my personal opinion, for a three dimensional point, a list with three coordinates is already enough. In reading we can read or not read the Z (that is already implemented).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree with @barendgehrels

Another issue here is that after this change the following code

boost::geometry::read_wkt("POINT M(1 2 3)", p);
std::cout << boost::geometry::wkt(p) << std::endl;

will output

POINT Z(1 2 3)

I think we should decide how we want to support more than 2 dimensions and probably issue #664 is the right place for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@barendgehrels points out a valid issue.

But what do we do with four dimensional points?

From the OGC SFS point, we have:

  • OGC SFS 1.1.0 restricted to 2D geometries
  • The extended WKT/ or 2.5D WKT implemented by OGR, PostGIS, etc. allow 2, 3, 4 dimension coordinates and do not use Z or M tags, see Adam Gawne-Cain, 1999, Simple Features Revision Proposal
  • OGC SFS 1.2.x allows 2, 3 and 4 dimensions using the Z or M tags: POINT Z(x, y, z), POINT M(x, y, m), POINT ZM(x, y, z, m). I think many may consider M as pseudo-dimension or not a dimension but a measure value.

a list with three coordinates is already enough

It seems desirable indeed to keep the current WKT I/O that does not read/output the Z and M tags.

Perhaps, an acceptable solution would be an optional flavor parameter, or similar, with current

  • boost::geometry::read_wkt(wkt, geometry, flavor
  • boost::geometry::wkt(geometry, flavor)

where flavor is enum, something like:

enum /*class*/ wkt_flavour
{
  ogc2d,        // OGC SFS 1.1.0
  ogc3d,        // OGC SFS 1.2.x
  ogc25d,       // Adam Gawne-Caine, 1999, Simple Features Revision Proposal
  ewkt = ogc25d // PostGIS terminology
};

or as an entirely separate OGC-compliant boost/geometry/gis/io/wkt/ extension.

stream_coordinate<Point, 0, dimension<Point>::type::value>::apply(os, p);
os << ")";
}
Expand Down Expand Up @@ -350,7 +351,8 @@ struct wkt<Point, point_tag>
: detail::wkt::wkt_point
<
Point,
detail::wkt::prefix_point
detail::wkt::prefix_point,
detail::wkt::prefix_point_z
>
{};

Expand Down Expand Up @@ -417,6 +419,7 @@ struct wkt<Multi, multi_point_tag>
detail::wkt::wkt_point
<
typename boost::range_value<Multi>::type,
detail::wkt::prefix_null,
detail::wkt::prefix_null
>,
detail::wkt::prefix_multipoint
Expand Down
2 changes: 2 additions & 0 deletions test/io/wkt/wkt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ void test_all()
{
using namespace boost::geometry;
typedef bg::model::point<T, 2, bg::cs::cartesian> P;
typedef bg::model::point<T, 3, bg::cs::cartesian> PZ;

test_wkt<P>("POINT(1 2)", 1);
test_wkt<bg::model::linestring<P> >("LINESTRING(1 1,2 2,3 3)", 3, 2 * sqrt(2.0));
Expand All @@ -200,6 +201,7 @@ void test_all()
"LINESTRING(1 0,2 0,3 0)");
test_relaxed_wkt<P>("POINT ( 1 2) ", "POINT(1 2)");
test_relaxed_wkt<P>("POINT M ( 1 2)", "POINT(1 2)");
test_relaxed_wkt<PZ>("POINT (1 2 3)", "Point Z(1 2 3)");
test_relaxed_wkt<bg::model::box<P> >("BOX(1 1,2 2)", "POLYGON((1 1,1 2,2 2,2 1,1 1))");

test_relaxed_wkt<bg::model::linestring<P> >("LINESTRING EMPTY", "LINESTRING()");
Expand Down