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

Already on GitHub? Sign in to your account

data-driven transform expressions, implements #664 #1243

Merged
merged 4 commits into from Jun 7, 2012

Conversation

Projects
None yet
3 participants
Contributor

lightmare commented Jun 6, 2012

I'm not sure whether cherry-picking relevant commits from our master branch onto a fresh mapnik/master branch was the right way to do this - if there's a better way I can scratch this request and make another.

There are a lot of changes in this patch, some of which are not necessary to deliver the functionality. I will describe those first, and try to explain why I included them.

  • agg::trans_affine::identity - not necessary. Without it, converter_traits<T,mapnik::affine_transform_tag>::conv_type can't get rid of trans_ member, unless there is some other global trans_affine instance it can pass to conv_transform constructor.
  • namespace value_adl_barrier - not necessary. Avoids expressions like a / b where neither a nor b are of type mapnik::value (and don't have a matching operator/ defined) from being compiled as value(a) / value(b). Described in more detail in #1228.
  • agg_renderer<T>::debug_draw_box() - not necessary at all, I left it there because it helped me a lot when debugging collision detection, and I believe it can be helpful to others as well.
  • agg_renderer<T>::render_marker() - this method was called from several places like this render_marker(pixel_position(x-0.5*w, y-0.5*h), ...). Inside the function, the half-size offset was eventually reverted by mtx.translate(pos.x+0.5*w, pos.y+0.5*h) - of course it worked fine, but in my opinion didn't make much sense, because (x-0.5*w, y-0.5*h) are the coordinates of the top left corner only if the marker is not transformed, otherwise it's adding pears to apples. So I changed the calls to render_marker(pixel_position(x, y), ...), but didn't go any further - I considered changing shield_symbolizer_helper::get_marker_position to return center coordinates, but then I'd have to change other renderers accordingly, which would be a bit too many opportunities for new bugs. Plus I wasn't sure whether you'd agree that using center coordinates would be clearer.
  • attribute_collector - changed into a template taking container type as a parameter (similar to path_processor<T>::collect_<U>) - not very useful, but at the time it allowed me to have an instance in transform_processor<T>::attribute_collector<U> with only a forward declaration.
  • vertex_converter - changed the order of template parameters T and P to match the order in the constructor and in args_type, and added A for trans_affine.
  • process_markers_symbolizer.cpp - some time ago we sort of fixed #1119, that is included. It's not ideal, because in process() we compute the extent of the transformed marker, and markers_placement then slides this box along the path. To make it completely correct (if I'm not mistaken), markers_placement would have to take the original marker bounding box and transformation, transform and remember all four corners (not their bounding box) and for each placement along the path apply local transformation (position and angle on the path) upon those four pre-transformed corners and use the bounding box of that for collision detection.

Now to the real thing. I took the svg transform grammar, replaced every occurence of double_ with expr (taken from within expression_grammar), wrote a set of transform node types, an attribute_collector and an evaluator that applies the stored transform list. Simple ;-)

Some transform expressions have optional parameters, for example rotate(angle [cx cy]). I store each parsed transform expression in a distinct node type for later re-creating the expression string, with each parameter in a member of type expr_node (optionals set to value_null). I could've stored each optional parameter in a boost::optional<expr_node> and in fact rewrote it that way - but then I realized that the former (with value_null meaning "not set") allows you to write transform="rotate([angle] [cx] [cy])" in your style, and it will simply work if you have nulls in cx, cy.

Python binding set_svg_transform still uses plain mapnik::svg::parse_transform. It should be easy to make it use the new parser, I just didn't have it constantly in mind.

lightmare added some commits Mar 6, 2012

add value_null operators, improve is_null, move mapnik::value definit…
…ion into separate namespace

(cherry picked from commit 653bca6)
new feature: transform expressions are now dynamic
(cherry picked from commit 173c402)

Conflicts:

	include/mapnik/vertex_converters.hpp
	src/agg/process_markers_symbolizer.cpp
	src/agg/process_point_symbolizer.cpp
	src/agg/process_polygon_pattern_symbolizer.cpp
	src/load_map.cpp
Owner

springmeyer commented Jun 6, 2012

@lightmare - wow.

Contributor

lightmare commented Jun 7, 2012

Forgot to mention that it makes symbolizer.hpp indirectly #include "agg_trans_affine.h" - probably avoidable with a little cleanup of the headers, which I should do anyway since they're a bit messy (parse_transform.hpp containg a class that has nothing to do with parsing etc). I saw agg::trans_affine forward declared in some headers with a comment saying so that apps using mapnik do not need agg headers. Is this still required? And if yes, which headers aren't allowed to #include agg?

@artemp artemp merged commit bd9609c into mapnik:master Jun 7, 2012

Owner

artemp commented Jun 7, 2012

applied in 11c34b1

013f0aa - avoid including "agg_trans_affine.h"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment