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

Handle overlapping caps by joining the lines #510

Merged
merged 1 commit into from Jul 11, 2019

Conversation

mrbean-bremen
Copy link
Member

To show what it does:
Here is the example used in the test in Chrome:
grafik

And here in the testrunner using SVG lib:
grafik

@mrbean-bremen
Copy link
Member Author

@H1Gdev - please have a look!

Copy link
Contributor

@H1Gdev H1Gdev left a comment

Choose a reason for hiding this comment

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

@mrbean-bremen

It differs somewhat between Chrome and Firefox.

firefox

{
if (this.StrokeDashArray.Count % 2 != 0)
strokeWidth = strokeWidth <= 0 ? 1 : strokeWidth;
Copy link
Contributor

Choose a reason for hiding this comment

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

What this condition is or will be true in case ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been used below twice - I just factored out for better readability of the code where it is used.
It seems to make sure that stroke width is set to 1 if StrokeWidth is invalid.

/* divide by stroke width - GDI behaviour that I don't quite understand yet.*/
pen.DashPattern = this.StrokeDashArray.Select(u => ((u.ToDeviceValue(renderer, UnitRenderingType.Other, this) <= 0) ? 1 : u.ToDeviceValue(renderer, UnitRenderingType.Other, this)) /
((strokeWidth <= 0) ? 1 : strokeWidth)).ToArray();
SvgUnit dashOffset = StrokeDashOffset != null ? StrokeDashOffset : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use var ?

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 think I tried that and it doesn't work with 0 - but I will check again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use SvgUnit.Empty !

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, turns out it works with 0 too - I was wrong!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

{
// add the next dash segment to the current one
dashPattern[i - 1] += dashPattern[i] + dashPattern[i + 1];
dashPattern[i] = dashPattern[i + 2];
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: this line can be incorporated into the loop below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@mrbean-bremen
Copy link
Member Author

mrbean-bremen commented Jul 11, 2019

@H1Gdev - thanks for the review!

It differs somewhat between Chrome and Firefox.

Yes - nothing we can do about that 😛

@H1Gdev
Copy link
Contributor

H1Gdev commented Jul 11, 2019

@mrbean-bremen

Yes - nothing we can do about that 😛

Is Chrome the ideal rendering ?

- overlapping cups led to a crash - fixes svg-net#508
- workaround to avoid the crash and not deviate too much from correct drawing
@mrbean-bremen
Copy link
Member Author

Is Chrome the ideal rendering ?

Maybe not ideal, but overall it seems to be nearer to the standard than the other browsers after my checks, at least most times. So yes, I tend to use it as a standard, if there is nothing matching in the W3C examples, though that maybe debatable.
Anyway I think if the browsers do not agree on details, we shall not overly worry about our implementation of these details.

Thanks for your review! I'll merge this now to fix the crash - we can always think about more improvements later.

@mrbean-bremen mrbean-bremen merged commit 773035d into svg-net:master Jul 11, 2019
@mrbean-bremen mrbean-bremen deleted the round-caps branch July 21, 2019 16:47
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

2 participants