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.

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

Clockwise fix #204

Merged
merged 7 commits into from
Apr 19, 2016
Merged

Clockwise fix #204

merged 7 commits into from
Apr 19, 2016

Conversation

flippmoke
Copy link
Member

@flippmoke flippmoke commented Apr 19, 2016

Fixed the way the decoder dealt with discarding interior and exterior rings when using bounding boxes. Also moved processing of vector tile area into vector tile coordinates so that clockwise and counterclockwise are more properly calculated.

/cc @springmeyer @jakepruitt

@artemp
Copy link
Contributor

artemp commented Apr 19, 2016

@flippmoke - why are you not using mapnik::util::is_clockwise? We must stamp down on duplicate code as it's only encourages 🐛 's .

@flippmoke
Copy link
Member Author

@artemp it no longer does it in the same way as it is done in a streaming fashion as the geometry is decoded. Therefore, I can no longer use the same method.

@flippmoke
Copy link
Member Author

Current Master:

./build/Release/vtile-decode ./test/data/0.0.0.vector.mvt 0 0 0 100000
z:0 x:0 y:0 iterations:100000
message: gzip compressed
8313.71ms (cpu 8282.74ms)   | decode as datasource_pbf: ./test/data/0.0.0.vector.mvt
processed 800000 features

Pull Request:

./build/Release/vtile-decode ./test/data/0.0.0.vector.mvt 0 0 0 100000
z:0 x:0 y:0 iterations:100000
message: gzip compressed
7916.56ms (cpu 7883.13ms)   | decode as datasource_pbf: ./test/data/0.0.0.vector.mvt
processed 800000 features

@@ -248,61 +272,79 @@ void decode_linestring(mapnik::geometry::geometry<typename T::value_type> & geom
{
geom = std::move(*itr);
}
else
{
geom = std::move(mapnik::geometry::geometry_empty());
Copy link
Contributor

Choose a reason for hiding this comment

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

@flippmoke:

  1. What is the reasoning for additional else in general? E.g. why set to empty vs what was it before?
  2. I think the std::move is useless here since mapnik::geometry::geometry_empty() is already a temporary and it can be just geom = mapnik::geometry::geometry_empty(); /cc @artemp for confirmation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe it actually is useless, in my mind I always wanted geom set in some manner in the code but then I remembered the default initialization for mapnik::geometry:geometry<> is mapnik::geometry::geometry_empty

@springmeyer
Copy link
Contributor

@flippmoke - this looks great, I think it is ready to 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants