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

SVG parser refactoring and improvements #3003

Merged
merged 62 commits into from
Jul 31, 2015
Merged

SVG parser refactoring and improvements #3003

merged 62 commits into from
Jul 31, 2015

Conversation

springmeyer
Copy link
Member

  • Drops requirement on libxml2 depedency
  • Switches to ptree (rapidxml inside boost::property_tree) as default xml parser
  • refactors SVG parser to collect errors
  • Adds more test coverage

Followups:

artemp added 30 commits July 14, 2015 19:45
SVG spec : ".. If 'rx' is greater than half of the width of the rectangle, then the user agent must process the 'rect' element with the effective value for 'rx' as half of the width of the rectangle. If 'ry' is greater than half of the height of the rectangle, then the user agent must process the 'rect' element with the effective value for 'ry' as half of the height of the rectangle.."
@springmeyer springmeyer added this to the Mapnik 3.0.2 milestone Jul 30, 2015
{
for (auto const& msg : p.error_messages())
{
std::cerr << "SVG PARSING ERROR:\"" << msg << "\"" << std::endl;

Choose a reason for hiding this comment

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

@artemp Should these be going to std::cerr or MAPNIK_LOG_ERROR(svg_parser)? I believe this is the cause of #3007.

Copy link
Member

Choose a reason for hiding this comment

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

@jakepruitt - yes, fixed in 3a4d574. I also made marker_cache to return marker_null() if case of any parsing/values errors. I think this is desired logic : only accept SVG we can interpret correctly and fail otherwise + logging errors.

REQUIRE(y1 == 25);
REQUIRE(x2 == 10);
REQUIRE(y2 == 10);
REQUIRE(r == 75);

Choose a reason for hiding this comment

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

Question, @artemp. The gradient fixture for this test has the following format

<radialGradient id="grad1b" gradientUnits="userSpaceOnUse" cx="10%" cy="10%" r="75%" fx="00.00%" fy="25%">

This translates to the above values when get_control_points is called. Is this the expected value? Should the values be 0, 0.25, 0.10, 0.10, 0.75 respectively due to the percentage conversion?

Copy link
Member

Choose a reason for hiding this comment

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

@jakepruitt - great catch! this was a bug in grammar - fixed in 42bf230

@artemp
Copy link
Member

artemp commented Jul 31, 2015

Summary of main changes

  • added container to log SVG parsing errors
  • reimplemented to use rapidxml for parsing XML (DOM)
  • support both xml:id and id attributes ( xml:id takes precedence )
  • added parse_id_from_url using boost::spirit
  • added error tracking when parsing doubles
  • unit tests for svg_parser to improve coverage
  • fixed rx/ry validation for rounded_rect
  • fixed dimensions parsing
  • remove libxml2 dependency

artemp added a commit that referenced this pull request Jul 31, 2015
SVG parser refactoring and improvements
@artemp artemp merged commit 727341f into master Jul 31, 2015
@flippmoke flippmoke deleted the svg-parser-errors branch July 31, 2015 18:58
cjmayo added a commit to cjmayo/mapnik that referenced this pull request May 5, 2020
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