-
Notifications
You must be signed in to change notification settings - Fork 214
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.
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution.
Since every new improvement or feature needs to be covered with test case(s), you may consider adding some to https://github.com/boostorg/geometry/tree/develop/test/io/wkt
@awulkiew In #664 (comment) you suggested
What did you have in mind? This PR seems to follow my suggestion I made in #664 (comment) @Siddharth-coder13 The commit and description of PR that addresses an existing issue should contain reference to the issue (i.e. |
Yeah, sure from next time I will make sure this. |
Do this PR needs more changes? @mloskot |
@Siddharth-coder13 No, for now it does not, but we need to wait and clarify my question #782 (comment) in order to confirm this is actually the kind of change we need and want. |
@awulkiew, your reviews are needed on this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR but it's not yet clear enough if we want to have a Z
by default.
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() << "("; | ||
else os << Policy1::apply() << "("; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
orM
tags, see Adam Gawne-Cain, 1999, Simple Features Revision Proposal - OGC SFS 1.2.x allows 2, 3 and 4 dimensions using the
Z
orM
tags:POINT Z(x, y, z)
,POINT M(x, y, m)
,POINT ZM(x, y, z, m)
. I think many may considerM
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.
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() << "("; |
There was a problem hiding this comment.
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 if
s etc. You may want to consult https://github.com/boostorg/geometry/wiki/Guidelines-for-Developers
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() << "("; | ||
else os << Policy1::apply() << "("; |
There was a problem hiding this comment.
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.
example/01_point_example.cpp
Outdated
typedef model::point<double, 3, cs::cartesian> point_t; | ||
typedef model::ring<point_t> ring_t; | ||
typedef model::PolyhedralSurface<ring_t> poly_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the library is now C++14, the aliases could be declared with the using
declarations, which is a bit neater, IMHO, as it puts the most important part, the name, to the left.
example/01_point_example.cpp
Outdated
poly_t polyhedron2 = {{{0,0,0}, {0, 1, 0}, {1, 1, 0}, {1, 0, 0}, {0, 0, 0}}, {{0, 0, 0}, {0, 1, 0}, {0, 1, 1}, {0, 0, 1}, {0, 0, 0}}, | ||
{{0, 0, 0}, {1, 0, 0}, {1, 0, 1}, {0, 0, 1}, {0, 0, 0}}, {{1, 1, 1}, {1, 0, 1}, {0, 0, 1}, {0, 1, 1}, {1, 1, 1}}, {{1, 1, 1}, {1, 0, 1}, {1, 0, 0}, {1, 1, 0}, {1, 1, 1}}, | ||
{{1, 1, 1}, {1, 1, 0}, {0, 1, 0}, {0, 1, 1}, {1, 1, 1}} }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you reformat it to something easier to distinguish rows and columns? e.g.
{
{{...}},
{{...}},
{{...}}
};
typedef typename point_type | ||
< | ||
ring_tag, | ||
typename ring_type<polyhedral_surface_tag, PolyhedralSurface>::type | ||
>::type type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The using
too?
#include <boost/config.hpp> | ||
#ifndef BOOST_NO_CXX11_HDR_INITIALIZER_LIST | ||
#include <initializer_list> | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move this in these between
#include <vector>
<---
#include <boost/concept/assert.hpp>
|
||
using point_type = model::point<double, 3, boost::geometry::cs::cartesian>; | ||
using ring_type = ring<point_type, true, true>; | ||
using ph = Container<ring_type, Allocator<ring_type> >; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does ph
stand for?
I'd suggest to use full, descriptive names, avoid abbreviations, unless others agree those are fine.
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many blank lines
|
||
BOOST_CONCEPT_USAGE(PolyhedralSurface) | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superfluous blank
} // namepspace concepts | ||
|
||
} // namespace geometry | ||
|
||
} // namespace boost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}}} // boost::geometry::concepts
@@ -39,7 +39,7 @@ | |||
#include <boost/geometry/geometries/concepts/polygon_concept.hpp> | |||
#include <boost/geometry/geometries/concepts/ring_concept.hpp> | |||
#include <boost/geometry/geometries/concepts/segment_concept.hpp> | |||
|
|||
#include <boost/geometry/geometries/concepts/PolyhedralSurface_concept.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, do not use CamelCase in file names (i.e. PolyhedralSurface_concept.hpp
should read polyhedralsurface_concept.hpp
@@ -29,5 +29,6 @@ | |||
#include <boost/geometry/geometries/polygon.hpp> | |||
#include <boost/geometry/geometries/ring.hpp> | |||
#include <boost/geometry/geometries/segment.hpp> | |||
#include <boost/geometry/geometries/PolyhedralSurface.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CamelCase
|
||
#define BOOST_GEOMETRY_REGISTER_POLYHEDRALSURFACE_TEMPLATED(PolyhedralSurface) \ | ||
namespace boost { namespace geometry { namespace traits { \ | ||
template<typename P> struct tag< PolyhedralSurface<P> > { typedef PolyhedralSurface_tag type; }; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
< PolyhedralSurface<P> >
to
<PolyhedralSurface<P>>
{ | ||
handle_open_parenthesis(it, end, wkt); | ||
typename ring_type<PolyhedralSurface>::type ring; | ||
while(it != end && *it != ")"){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{
in separate line
@@ -42,6 +47,11 @@ struct prefix_linestring | |||
static inline const char* apply() { return "LINESTRING"; } | |||
}; | |||
|
|||
struct prefix_polyhedral_surface | |||
{ | |||
static inline const char* apply() { return "POLYHEDRALSURFACE Z"; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the Z
as per @barendgehrels #782 (comment)
@Siddharth-coder13 Please, see #789 (comment) |
d0e6a99
to
ff1f245
Compare
Changed the
wkt_point
struct to take 3 parameters rather than 2, for one for 2d and one for 3d, added theprefix_z
function inprefix.hpp,
and also added a line of code to print 3d points in01_point_example.cpp
.