shuffle complexity into layout constructor

- should reduce mistakes in refactoring
  - gives top level ownership of evaluated_format_properties_ptr to a layout node (#2516)
  - not sure ^^ if this actually keeps it in scope enough for rendering?
  - moves data transformation functions off of text_symbolizer_properties

include/mapnik/group/group_symbolizer_helper.hpp

@@ -75,7 +75,7 @@ public:
 
     inline text_symbolizer_properties const& get_properties() const
     {
-        return placement_->properties;
+        return info_ptr_->properties;
     }
 
     pixel_position_list const& get();

include/mapnik/text/symbolizer_helpers.hpp

@@ -96,7 +96,7 @@ protected:
     mutable std::list<pixel_position>::iterator point_itr_;
     // Use point placement. Otherwise line placement is used.
     mutable bool point_placement_;
-    text_placement_info_ptr placement_;
+    text_placement_info_ptr info_ptr_;
     evaluated_text_properties_ptr text_props_;
 };
 

include/mapnik/text/text_layout.hpp

@@ -57,7 +57,13 @@ public:
     using const_iterator = line_vector::const_iterator;
     using child_iterator = text_layout_vector::const_iterator;
 
-    text_layout(face_manager_freetype & font_manager, double scale_factor, text_layout_properties const& properties);
+    text_layout(face_manager_freetype & font_manager,
+                feature_impl const& feature,
+                attributes const& attrs,
+                double scale_factor,
+                text_symbolizer_properties const& properties,
+                text_layout_properties const& layout_defaults,
+                formatting::node_ptr tree);
 
     // Adds a new text part. Call this function repeatedly to build the complete text.
     void add_text(mapnik::value_unicode_string const& str, evaluated_format_properties_ptr format);
@@ -99,7 +105,8 @@ public:
     inline text_layout_vector const& get_child_layouts() const { return child_layout_list_; }
     inline face_manager_freetype & get_font_manager() const { return font_manager_; }
     inline double get_scale_factor() const { return scale_factor_; }
-    inline text_layout_properties const& get_layout_properties() const { return properties_; }
+    inline text_symbolizer_properties const& get_default_text_properties() const { return properties_; }
+    inline text_layout_properties const& get_layout_properties() const { return layout_properties_; }
 
     inline rotation const& orientation() const { return orientation_; }
     inline pixel_position const& displacement() const { return displacement_; }
@@ -108,8 +115,6 @@ public:
     pixel_position alignment_offset() const;
     double jalign_offset(double line_width) const;
 
-    void evaluate_properties(feature_impl const& feature, attributes const& attr);
-
 private:
     void break_line(std::pair<unsigned, unsigned> && line_limits);
     void break_line_icu(std::pair<unsigned, unsigned> && line_limits);
@@ -118,11 +123,11 @@ private:
     void clear_cluster_widths(unsigned first, unsigned last);
     void init_auto_alignment();
 
-    //input
+    // input
     face_manager_freetype & font_manager_;
     double scale_factor_;
 
-    //processing
+    // processing
     text_itemizer itemizer_;
     // Maps char index (UTF-16) to width. If multiple glyphs map to the same char the sum of all widths is used
     // note: this probably isn't the best solution. it would be better to have an object for each cluster, but
@@ -132,13 +137,19 @@ private:
     double height_;
     unsigned glyphs_count_;
 
-    //output
+    // output
     line_vector lines_;
 
-    //text layout properties
+    // layout properties (owned by text_layout)
-    text_layout_properties properties_;
+    text_layout_properties layout_properties_;
 
-    //alignments
+    // text symbolizer properties (owned by placement_finder's 'text_placement_info' (info_) which is owned by symbolizer_helper
+    text_symbolizer_properties const& properties_;
+
+    // format properties (owned by text_layout)
+    evaluated_format_properties_ptr format_;
+
+    // alignments
     vertical_alignment_e valign_;
     horizontal_alignment_e halign_;
     justify_alignment_e jalign_;
@@ -154,7 +165,7 @@ private:
     pixel_position displacement_ = {0,0};
     box2d<double> bounds_;
 
-    //children
+    // children
     text_layout_vector child_layout_list_;
 };
 

include/mapnik/text/text_properties.hpp

@@ -196,11 +196,10 @@ struct MAPNIK_DECL text_symbolizer_properties
     // Save all values to XML ptree (but does not create a new parent node!).
     void to_xml(boost::property_tree::ptree & node, bool explicit_defaults,
                 text_symbolizer_properties const& dfl) const;
-    // Takes a feature and produces formatted text as output.
-    // The output object has to be created by the caller and passed in for thread safety.
-    void process(text_layout & output, feature_impl const& feature, attributes const& vars) const;
     // Sets new format tree.
     void set_format_tree(formatting::node_ptr tree);
+    // Get format tree.
+    formatting::node_ptr format_tree() const;
     // Get a list of all expressions used in any placement.
     // This function is used to collect attributes.
     void add_expressions(expression_set & output) const;

src/text/formatting/layout.cpp

@@ -83,9 +83,9 @@ node_ptr layout_node::from_xml(xml_node const& xml, fontset_map const& fontsets)
     return n;
 }
 
-void layout_node::apply(evaluated_format_properties_ptr p, feature_impl const& feature, attributes const& vars, text_layout & output) const
+void layout_node::apply(evaluated_format_properties_ptr p, feature_impl const& feature, attributes const& vars, text_layout & parent) const
 {
-    text_layout_properties new_properties(output.get_layout_properties());
+    text_layout_properties new_properties(parent.get_layout_properties());
     if (dx) new_properties.dx = *dx;
     if (dy) new_properties.dy = *dy;
     if (text_ratio) new_properties.text_ratio = *text_ratio;
@@ -100,9 +100,14 @@ void layout_node::apply(evaluated_format_properties_ptr p, feature_impl const& f
     if (jalign) new_properties.jalign = *jalign;
 
     // starting a new offset child with the new displacement value
-    text_layout_ptr child_layout = std::make_shared<text_layout>(output.get_font_manager(), output.get_scale_factor(), new_properties);
+    // we pass a null format tree since this is not the parent but a child
-    child_layout->evaluate_properties(feature,vars);
+    text_layout_ptr child_layout = std::make_shared<text_layout>(parent.get_font_manager(),
-
+                                                                 feature,
+                                                                 vars,
+                                                                 parent.get_scale_factor(),
+                                                                 parent.get_default_text_properties(),
+                                                                 new_properties,
+                                                                 formatting::node_ptr());
     // process contained format tree into the child node
     if (child_)
     {
@@ -112,7 +117,7 @@ void layout_node::apply(evaluated_format_properties_ptr p, feature_impl const& f
     {
         MAPNIK_LOG_WARN(format) << "Useless layout node: Contains no text";
     }
-    output.add_child(child_layout);
+    parent.add_child(child_layout);
 }
 
 void layout_node::set_child(node_ptr child)

src/text/placement_finder.cpp

@@ -64,19 +64,26 @@ bool placement_finder::next_position()
 {
     if (info_.next())
     {
-        text_layout_ptr layout = std::make_shared<text_layout>(font_manager_, scale_factor_, info_.properties.layout_defaults);
+        // parent layout, has top-level ownership of a new evaluated_format_properties_ptr (TODO is this good enough to stay in scope???)
-        layout->evaluate_properties(feature_, attr_);
+        // but does not take ownership of the text_symbolizer_properties (info_.properties)
-        move_dx_ = layout->displacement().x;
+        text_layout_ptr layout = std::make_shared<text_layout>(font_manager_,
-        // TODO: this call is needed (text-bug1533) ??
+                                                               feature_,
+                                                               attr_,
+                                                               scale_factor_,
+                                                               info_.properties,
+                                                               info_.properties.layout_defaults,
+                                                               info_.properties.format_tree());
+        // TODO: why is this call needed?
         // https://github.com/mapnik/mapnik/issues/2525
         text_props_ = evaluate_text_properties(info_.properties,feature_,attr_);
-        info_.properties.process(*layout, feature_, attr_);
         // Note: this clear call is needed when multiple placements are tried
         // like with placement-type="simple|list"
         if (!layouts_.empty()) layouts_.clear();
         // Note: multiple layouts_ may result from this add() call
         layouts_.add(layout);
         layouts_.layout();
+        // cache a few values for use elsewhere in placement finder
+        move_dx_ = layout->displacement().x;
         horizontal_alignment_ = layout->horizontal_alignment();
         return true;
     }

src/text/symbolizer_helpers.cpp

@@ -58,8 +58,8 @@ base_symbolizer_helper::base_symbolizer_helper(
       dims_(0, 0, width, height),
       query_extent_(query_extent),
       scale_factor_(scale_factor),
-      placement_(get<text_placements_ptr>(sym_, keys::text_placements_)->get_placement_info(scale_factor)),
+      info_ptr_(get<text_placements_ptr>(sym_, keys::text_placements_)->get_placement_info(scale_factor)),
-      text_props_(evaluate_text_properties(placement_->properties,feature_,vars_))
+      text_props_(evaluate_text_properties(info_ptr_->properties,feature_,vars_))
 {
     initialize_geometries();
     if (!geometries_to_process_.size()) return; // FIXME - bad practise
@@ -183,7 +183,7 @@ text_symbolizer_helper::text_symbolizer_helper(
         DetectorT &detector, box2d<double> const& query_extent,
         agg::trans_affine const& affine_trans)
     : base_symbolizer_helper(sym, feature, vars, prj_trans, width, height, scale_factor, t, query_extent),
-      finder_(feature, vars, detector, dims_, *placement_, font_manager, scale_factor),
+      finder_(feature, vars, detector, dims_, *info_ptr_, font_manager, scale_factor),
     adapter_(finder_,false),
     converter_(query_extent_, adapter_, sym_, t, prj_trans, affine_trans, feature, vars, scale_factor)
 {
@@ -275,7 +275,7 @@ text_symbolizer_helper::text_symbolizer_helper(
         view_transform const& t, FaceManagerT & font_manager,
         DetectorT & detector, box2d<double> const& query_extent, agg::trans_affine const& affine_trans)
     : base_symbolizer_helper(sym, feature, vars, prj_trans, width, height, scale_factor, t, query_extent),
-      finder_(feature, vars, detector, dims_, *placement_, font_manager, scale_factor),
+      finder_(feature, vars, detector, dims_, *info_ptr_, font_manager, scale_factor),
       adapter_(finder_,true),
       converter_(query_extent_, adapter_, sym_, t, prj_trans, affine_trans, feature, vars, scale_factor)
 {

src/text/text_layout.cpp

@@ -51,12 +51,52 @@ static void rotated_box2d(box2d<double> & box, rotation const& rot, pixel_positi
     box.init(center.x - half_width, center.y - half_height, center.x + half_width, center.y + half_height);
 }
 
+pixel_position evaluate_displacement(double dx, double dy, directions_e dir)
+{
+    switch (dir)
+    {
+    case EXACT_POSITION:
+        return pixel_position(dx,dy);
+        break;
+    case NORTH:
+        return pixel_position(0,-std::abs(dy));
+        break;
+    case EAST:
+        return pixel_position(std::abs(dx),0);
+        break;
+    case SOUTH:
+        return pixel_position(0,std::abs(dy));
+        break;
+    case WEST:
+        return pixel_position(-std::abs(dx),0);
+        break;
+    case NORTHEAST:
+        return pixel_position(std::abs(dx),-std::abs(dy));
+        break;
+    case SOUTHEAST:
+        return pixel_position(std::abs(dx),std::abs(dy));
+        break;
+    case NORTHWEST:
+        return pixel_position(-std::abs(dx),-std::abs(dy));
+        break;
+    case SOUTHWEST:
+        return pixel_position(-std::abs(dx),std::abs(dy));
+        break;
+    }
+}
+
 pixel_position pixel_position::rotate(rotation const& rot) const
 {
     return pixel_position(x * rot.cos - y * rot.sin, x * rot.sin + y * rot.cos);
 }
 
-text_layout::text_layout(face_manager_freetype & font_manager, double scale_factor, text_layout_properties const& properties)
+text_layout::text_layout(face_manager_freetype & font_manager,
+                         feature_impl const& feature,
+                         attributes const& attrs,
+                         double scale_factor,
+                         text_symbolizer_properties const& properties,
+                         text_layout_properties const& layout_defaults,
+                         formatting::node_ptr tree)
     : font_manager_(font_manager),
       scale_factor_(scale_factor),
       itemizer_(),
@@ -65,7 +105,53 @@ text_layout::text_layout(face_manager_freetype & font_manager, double scale_fact
       height_(0.0),
       glyphs_count_(0),
       lines_(),
-      properties_(properties) {}
+      layout_properties_(layout_defaults),
+      properties_(properties),
+      format_(std::make_shared<detail::evaluated_format_properties>())
+    {
+        double dx = util::apply_visitor(extract_value<value_double>(feature,attrs), layout_properties_.dx);
+        double dy = util::apply_visitor(extract_value<value_double>(feature,attrs), layout_properties_.dy);
+        displacement_ = evaluate_displacement(dx,dy, layout_properties_.dir);
+        std::string wrap_str = util::apply_visitor(extract_value<std::string>(feature,attrs), layout_properties_.wrap_char);
+        if (!wrap_str.empty()) wrap_char_ = wrap_str[0];
+        wrap_width_ = util::apply_visitor(extract_value<value_double>(feature,attrs), layout_properties_.wrap_width);
+        double angle = util::apply_visitor(extract_value<value_double>(feature,attrs), layout_properties_.orientation);
+        orientation_.init(angle * M_PI/ 180.0);
+        wrap_before_ = util::apply_visitor(extract_value<value_bool>(feature,attrs), layout_properties_.wrap_before);
+        repeat_wrap_char_ = util::apply_visitor(extract_value<value_bool>(feature,attrs), layout_properties_.repeat_wrap_char);
+        rotate_displacement_ = util::apply_visitor(extract_value<value_bool>(feature,attrs), layout_properties_.rotate_displacement);
+        valign_ = util::apply_visitor(extract_value<vertical_alignment_enum>(feature,attrs),layout_properties_.valign);
+        halign_ = util::apply_visitor(extract_value<horizontal_alignment_enum>(feature,attrs),layout_properties_.halign);
+        jalign_ = util::apply_visitor(extract_value<justify_alignment_enum>(feature,attrs),layout_properties_.jalign);
+
+        // Takes a feature and produces formatted text as output.
+        if (tree)
+        {
+            format_properties const& format_defaults = properties_.format_defaults;
+            format_->text_size = util::apply_visitor(extract_value<value_double>(feature,attrs), format_defaults.text_size);
+            format_->character_spacing = util::apply_visitor(extract_value<value_double>(feature,attrs), format_defaults.character_spacing);
+            format_->line_spacing = util::apply_visitor(extract_value<value_double>(feature,attrs), format_defaults.line_spacing);
+            format_->text_opacity = util::apply_visitor(extract_value<value_double>(feature,attrs), format_defaults.text_opacity);
+            format_->halo_opacity = util::apply_visitor(extract_value<value_double>(feature,attrs), format_defaults.halo_opacity);
+            format_->halo_radius = util::apply_visitor(extract_value<value_double>(feature,attrs), format_defaults.halo_radius);
+            format_->fill = util::apply_visitor(extract_value<color>(feature,attrs), format_defaults.fill);
+            format_->halo_fill = util::apply_visitor(extract_value<color>(feature,attrs), format_defaults.halo_fill);
+            format_->text_transform = util::apply_visitor(extract_value<text_transform_enum>(feature,attrs), format_defaults.text_transform);
+            format_->face_name = format_defaults.face_name;
+            format_->fontset = format_defaults.fontset;
+            format_->ff_settings = util::apply_visitor(extract_value<font_feature_settings>(feature,attrs), format_defaults.ff_settings);
+            // Turn off ligatures if character_spacing > 0.
+            if (format_->character_spacing > .0 && format_->ff_settings.count() == 0)
+            {
+                format_->ff_settings.append(font_feature_liga_off);
+            }
+            tree->apply(format_, feature, attrs, *this);
+        }
+        else
+        {
+            MAPNIK_LOG_WARN(text_properties) << "text_symbolizer_properties can't produce text: No formatting tree!";
+        }
+    }
 
 void text_layout::add_text(mapnik::value_unicode_string const& str, evaluated_format_properties_ptr format)
 {
@@ -341,63 +427,6 @@ void text_layout::shape_text(text_line & line)
     harfbuzz_shaper::shape_text(line, itemizer_, width_map_, font_manager_, scale_factor_);
 }
 
-pixel_position evaluate_displacement(double dx, double dy, directions_e dir)
-{
-    switch (dir)
-    {
-    case EXACT_POSITION:
-        return pixel_position(dx,dy);
-        break;
-    case NORTH:
-        return pixel_position(0,-std::abs(dy));
-        break;
-    case EAST:
-        return pixel_position(std::abs(dx),0);
-        break;
-    case SOUTH:
-        return pixel_position(0,std::abs(dy));
-        break;
-    case WEST:
-        return pixel_position(-std::abs(dx),0);
-        break;
-    case NORTHEAST:
-        return pixel_position(std::abs(dx),-std::abs(dy));
-        break;
-    case SOUTHEAST:
-        return pixel_position(std::abs(dx),std::abs(dy));
-        break;
-    case NORTHWEST:
-        return pixel_position(-std::abs(dx),-std::abs(dy));
-        break;
-    case SOUTHWEST:
-        return pixel_position(-std::abs(dx),std::abs(dy));
-        break;
-    }
-}
-
-void text_layout::evaluate_properties(feature_impl const& feature, attributes const& attrs)
-{
-    // dx,dy
-    double dx = util::apply_visitor(extract_value<value_double>(feature,attrs), properties_.dx);
-    double dy = util::apply_visitor(extract_value<value_double>(feature,attrs), properties_.dy);
-    displacement_ = evaluate_displacement(dx,dy, properties_.dir);
-    std::string wrap_str = util::apply_visitor(extract_value<std::string>(feature,attrs), properties_.wrap_char);
-    if (!wrap_str.empty()) wrap_char_ = wrap_str[0];
-    wrap_width_ = util::apply_visitor(extract_value<value_double>(feature,attrs), properties_.wrap_width);
-
-    double angle = util::apply_visitor(extract_value<value_double>(feature,attrs), properties_.orientation);
-    orientation_.init(angle * M_PI/ 180.0);
-
-    wrap_before_ = util::apply_visitor(extract_value<value_bool>(feature,attrs), properties_.wrap_before);
-    repeat_wrap_char_ = util::apply_visitor(extract_value<value_bool>(feature,attrs), properties_.repeat_wrap_char);
-    rotate_displacement_ = util::apply_visitor(extract_value<value_bool>(feature,attrs), properties_.rotate_displacement);
-
-    valign_ = util::apply_visitor(extract_value<vertical_alignment_enum>(feature,attrs),properties_.valign);
-    halign_ = util::apply_visitor(extract_value<horizontal_alignment_enum>(feature,attrs),properties_.halign);
-    jalign_ = util::apply_visitor(extract_value<justify_alignment_enum>(feature,attrs),properties_.jalign);
-
-}
-
 void text_layout::init_auto_alignment()
 {
     if (valign_ == V_AUTO)

src/text/text_properties.cpp

@@ -68,43 +68,16 @@ text_symbolizer_properties::text_symbolizer_properties()
       format_defaults(),
       tree_() {}
 
-void text_symbolizer_properties::process(text_layout & output, feature_impl const& feature, attributes const& attrs) const
-{
-    output.clear();
-
-    if (tree_)
-    {
-        //evaluate format properties
-        evaluated_format_properties_ptr format = std::make_shared<detail::evaluated_format_properties>();
-        format->text_size = util::apply_visitor(extract_value<value_double>(feature,attrs), format_defaults.text_size);
-        format->character_spacing = util::apply_visitor(extract_value<value_double>(feature,attrs), format_defaults.character_spacing);
-        format->line_spacing = util::apply_visitor(extract_value<value_double>(feature,attrs), format_defaults.line_spacing);
-        format->text_opacity = util::apply_visitor(extract_value<value_double>(feature,attrs), format_defaults.text_opacity);
-        format->halo_opacity = util::apply_visitor(extract_value<value_double>(feature,attrs), format_defaults.halo_opacity);
-        format->halo_radius = util::apply_visitor(extract_value<value_double>(feature,attrs), format_defaults.halo_radius);
-        format->fill = util::apply_visitor(extract_value<color>(feature,attrs), format_defaults.fill);
-        format->halo_fill = util::apply_visitor(extract_value<color>(feature,attrs), format_defaults.halo_fill);
-        format->text_transform = util::apply_visitor(extract_value<text_transform_enum>(feature,attrs), format_defaults.text_transform);
-        format->face_name = format_defaults.face_name;
-        format->fontset = format_defaults.fontset;
-
-        format->ff_settings = util::apply_visitor(extract_value<font_feature_settings>(feature,attrs), format_defaults.ff_settings);
-        // Turn off ligatures if character_spacing > 0.
-        if (format->character_spacing > .0 && format->ff_settings.count() == 0)
-        {
-            format->ff_settings.append(font_feature_liga_off);
-        }
-
-        tree_->apply(format, feature, attrs, output);
-    }
-    else MAPNIK_LOG_WARN(text_properties) << "text_symbolizer_properties can't produce text: No formatting tree!";
-}
-
 void text_symbolizer_properties::set_format_tree(formatting::node_ptr tree)
 {
     tree_ = tree;
 }
 
+formatting::node_ptr text_symbolizer_properties::format_tree() const
+{
+    return tree_;
+}
+
 void text_symbolizer_properties::text_properties_from_xml(xml_node const& node)
 {
     // The options 'margin' and 'repeat-distance' replace 'minimum-distance'.