feat!: added PolylinePattern with support for solid, dotted, dashed styles

[monsieurtanuki]

Impacted files:

• pages/polyline.dart: added a dashed polyline as example
• polyline_layer/polyline.dart: added the dashValues field for Polyline
• painter.dart: now handling the dashed polylines

Screenshot (new dashed polyline between Paris and Nice):

What now?

• the use of isDotted / segmentSpacingFactor / dashValues fields is a bit clumsy
  => we would be better off with a unique field
  => new class PolylineStyle with "plain", "dotted(space)", "dashed(dashvalues)"?
• we could reuse for the dotted style the algorithm implemented in the new method getDistanceOffset used for "dashed style" (something like: give me the offset of the point on this [A,B] segment distant from A by n pixels)
• for the moment the "final point situation" is not dealt with: what if the latest point of the dashed line is invisible? We should explicitly put a dot here, shouldn't we?

[JaffaKetchup]

Hey, this looks great! Huge thanks for ticking this off the list.

the use of isDotted / segmentSpacingFactor / dashValues fields is a bit clumsy
=> we would be better off with a unique field
=> new class PolylineStyle with "plain", "dotted(space)", "dashed(dashvalues)"?

Yes, I agree with this. I've added a PolylinePattern object which does this. It's a little bit of complex logic internally, but it keeps the externals small.

we could reuse for the dotted style the algorithm implemented in the new method getDistanceOffset used for "dashed style" (something like: give me the offset of the point on this [A,B] segment distant from A by n pixels)

We generally try to avoid including these generic tools publicly, the major exception being the simplification tools. For now, I'm happy to keep this internal, and we can always make it public in future.

for the moment the "final point situation" is not dealt with: what if the latest point of the dashed line is invisible? We should explicitly put a dot here, shouldn't we?

Yep, we should deal with this. I'm not quite sure the best way to do this, so if you have something in mind, please go ahead!

[monsieurtanuki]

Thank you @JaffaKetchup for your improvements!

[monsieurtanuki]

we could reuse for the dotted style the algorithm implemented in the new method getDistanceOffset used for "dashed style" (something like: give me the offset of the point on this [A,B] segment distant from A by n pixels)

We generally try to avoid including these generic tools publicly, the major exception being the simplification tools. For now, I'm happy to keep this internal, and we can always make it public in future.

Oh it's already an internal method, and the point was to keep it that way.
What bothers me is having two similar algorithms to maintain in the same file.

for the moment the "final point situation" is not dealt with: what if the latest point of the dashed line is invisible? We should explicitly put a dot here, shouldn't we?

Yep, we should deal with this. I'm not quite sure the best way to do this, so if you have something in mind, please go ahead!

The idea would be to say:

• "is the last point displayed or not?" - and we already know it as _paintDashedLine returns a relevant bool for that
• if so, display a "dot" on that very point - and it is already somehow implemented in the "dotted" version

Now that I'm thinking, we could also have "smart dashes" polyline pattern, something like:

• I want a dashed line
• the input is
  • a double array, like [50, 15, 20, 10]
  • and the number of time the user wants to see this pattern displayed, like 4
• we compute the total length L of the polyline
• and we divide the values (50, ...) by a factor so that [50, 15, 20, 10, 50, 15, 20, 10, 50, 15, 20, 10, 50, 15, 20, 10, 50] is displayed completely in the polyline

[monsieurtanuki]

@JaffaKetchup Looks good to me!

Still pending: what to do with the last point of a dashed line? I suggest a dot - would you like me to work on it? (I could start only this week-end).

And the "if solid / if dashed / if dotted" sequence is a bit clumsy but it's not very important - except that we have similar algorithms coded twice for dashed and dotted.

Not important suggestion: we could make a dot part of a dashed line, like Morse code (dash-space-dot-space-dash...)

[monsieurtanuki]

@JaffaKetchup I've just fixed the side effect problem:

  /// The idea is that we need to be able to display the dash pattern completely
  /// n times (at least once), plus once the initial dash segment. That's the
  /// way we deal with the "ending" side-effect.

[500, 500, 300, 1000]	[50, 50, 30, 100]	[50, 20, 25, 20]

[JaffaKetchup]

LGTM, thanks! Not entirely sure what you did in that last bit, but it seems to have worked and looks nice.

Any other maintainers want to have a review?

[monsieurtanuki]

LGTM, thanks! Not entirely sure what you did in that last bit, but it seems to have worked and looks nice.

@JaffaKetchup As explained, there's a side-effect with dash pattern - as there is obviously with dotted pattern: if the dash pattern is 50+20 pixels, what do we do if the polyline length is 60 pixels? Just display the first 50 pixels and nothing else, so that the last point (presumably very important) is not displayed at all?
The suggestion implemented here is to always display "(dash pattern) * n + first dash segment" where n > 0. So we always display the first point and the last point.

[monsieurtanuki]

Still playing with side effects, with dash segments [50, 20, 100, 100]:

don't see the final point (Nice)	see Nice as a mere dot	used a factor on segment values

1. if we don't handle side-effects, we may not see the final point
2. handling the side-effect - display the last point as a dot if needed
3. handling the side-effect - compute and use a factor so that we display an integer number of times the segments, plus once the first segment, so that the last point is always displayed

Actually I prefer solution 2, because solution 1 is problematic (no final point) and solution 3 is hard to explain anyway.

@JaffaKetchup

• Would you like me to implement solution 2?
• Would you like me to implement something similar for dotted style?
• I think the current PR has been re-labeled as "breaking change" (feat!), which is not really, is it?

[JaffaKetchup]

Hey, thanks for continuing to work on this.

Option one is unnacceptable (I agree), but I feel like option 2 and 3 are both valid. Additionally, rather than option 2, if there are enough dashes, we could do it so that it just dashes as normal, then truncates short the last dash (if it isn't a gap). Not sure how difficult this could be to implement. If you've got time, and it's not too complex, I guess allowing the user to pick between them all with some sort of PatternFit option would be good. But I don't want to exhaust you.

For dotted lines, I guess only option 3 makes sense (scaling the factor), because otherwise it will look a bit strange? Again, PatternFit could work.

The PR is breaking because it removes the isDotted option.

[monsieurtanuki]

@JaffaKetchup I've just introduced the new PatternFit field, for both dotted and dashed styles.

x	dashed [100, 40]	dotted (10)
resize
lastPoint

Outdated

[JaffaKetchup]

@monsieurtanuki I've run with those changes (thanks for those :)) with a couple more options, just so we can give users every option. It doesn't add too much complexity. Let me know what you think!

[monsieurtanuki]

@monsieurtanuki I've run with those changes (thanks for those :)) with a couple more options, just so we can give users every option. It doesn't add too much complexity. Let me know what you think!

@JaffaKetchup Thank you for your rewritings! My comments...

I'm not convinced with PatternFit having "forbidden" values, like extendFinalDash not available for dotted polylines. I would have created separate DottedPatternFit and DashedPatternFit enums instead. But that's not very important.

I don't believe your extendFinalDash works - or I haven't understood the goal.
My understanding was that instead of displaying spaces for the very last invisible parts, you were displaying lines. But the way you coded it you display only the very last offset segment: from the latest Offset, but not following the route from the latest lineTo.

• (--- --- --- ) as none
• (--- --- -----) as extendFinalDash as I've understood it
• (--- --- --- -) as extendFinalDash as it's currently coded, potentially

For extendFinalDash, I would expect a final zigzag, not a straight line.

none	scale	extendFinalDash

Working on it...

[monsieurtanuki]

@JaffaKetchup Latest push:

• PatternFit.none
• PatternFit.appendDot
• PatternFit.extendFinalDash
• PatternFit.scaleUp
• PatternFit.scaleDown

dotted	dashed

[JaffaKetchup]

LGTM! Huge thanks for your efforts :)

I'm not going to wait for another review, I'm pretty confident it's all good.

(Yep, that was a good catch. I hadn't properly considered the extension fit.)

[monsieurtanuki]

Thank you @JaffaKetchup for your reviews and comments!
Just an additional comment, as there are breaking changes anyway: perhaps the "spacing" parameter of "dotted" could be a size in pixels rather than a factor to apply on top of paint size.