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

Fix #270337: True dotted lines #6349

Merged
merged 1 commit into from
Nov 30, 2020

Conversation

elerouxx
Copy link
Contributor

@elerouxx elerouxx commented Jul 19, 2020

Resolves: https://musescore.org/en/node/270337

Fixes true dotted lines for text lines (with round dots)
Also fixes:

  • dash-dotted, dash-dot-dotted lines (show real dots)
  • dotted, dashed and wide dashed slurs and ties.

Before
image

After
image

The problem is: the generic Qt DotLine style only shows dots properly when using FlatCaps, and FlatCaps are not always appropriate. Any square or round cap turns dots into short dashes.

This solution replaces default Qt lineStyles dash patterns with MuseScore's own patterns, so it allows real dots, round dots, and also allows the creation of MuseScore's specific dash styles instead of sticking to the generic Qt design.

Other changes:

  • The minimum dash length set for custom patterns lowered from 1.0 to 0.01 to allow very short dashes (= dots).
  • The step increments of the spin boxes for custom dash was lowered from 1.0 to 0.25 to improve control.

Code review and feedback will be highly appreciated.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • [NA] I created the test (mtest, vtest, script test) to verify the changes I made

@Jojo-Schmitz
Copy link
Contributor

I guess you mean text lines, not staff lines?

@@ -267,7 +267,7 @@
<double>20.000000000000000</double>
</property>
<property name="singleStep">
<double>1.000000000000000</double>
<double>0.250000000000000</double>
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess here old gap width steps are still appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are right. I'll update that.
(I am going to fix dotted slurs and search what else is missing. I think they should be fixed in this PR too).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, agreed, slurs (and ties) should use the same

@elerouxx elerouxx force-pushed the real-dotted-lines-3.x branch from b2ce755 to 1b43ec5 Compare July 19, 2020 18:31
@ecstrema
Copy link
Contributor

image
Shouldn't this distance be calculated from the right?

@@ -43,7 +43,9 @@ void SlurSegment::draw(QPainter* painter) const
case 1:
painter->setBrush(Qt::NoBrush);
pen.setWidthF(score()->styleP(Sid::SlurDottedWidth) * mag);
pen.setStyle(Qt::DotLine);
pen.setStyle(Qt::CustomDashLine);
pen.setDashPattern(QVector<qreal> {0.01, 2.5});
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Jul 20, 2020

Choose a reason for hiding this comment

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

Shouldn't that 'magic' 2.5 rather be expressed in pen width units, pen.widthF() (which in turn relate to Sid::SlurDottedWidth and mag)

See https://doc.qt.io/qt-5/qpen.html#setDashPattern
Seems the pen.setStyle(Qt::CustomDashLine) is implicit, when using pen.setDashPattern()

Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Jul 20, 2020

Choose a reason for hiding this comment

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

Your current dotted slur:
image
When using pen.widthF():
image
Looks the same, but eliminates that magic number

Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Jul 20, 2020

Choose a reason for hiding this comment

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

But then again, making the dots thicker also increases their distance, and by quite a lot. In either case...
Maybe 1.5 or 2 might be a better value?
1.5:
image
2.0:
image

Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Jul 20, 2020

Choose a reason for hiding this comment

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

I think the dashed and the wide dashed (and customer dashed) lines all should use a Qt::FlatCap BTW, to not make themselves longer by their width.

Copy link
Contributor Author

@elerouxx elerouxx Jul 20, 2020

Choose a reason for hiding this comment

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

I will take a look at that pen.widthF() for comparison. About the space between dots, I went from 2.0 to 3.0 and ended up with 2.5 but It's a matter of deciding the design of this dotted "preset".
User can still set a custom dash pattern that looks like dotted and choose a gap - except, no rounded dots.
Ideally, Custom Dash might include an option to choose Round/Square/Flat caps (or maybe the whole thing so user can add rounded cap to solid lines) but either would require adding a property to be saved, and maybe options for Style... so I guess it might be an improvement for later.

Agree with flatCaps so dots in dash-dotted lines really look like dots. Trying this next.

EDIT: the values in the dash patterns (qreal) already refer to pen width, where 1.0 = 1 pen width, so dashes and gaps will scale with the pen (and with mag).

Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Jul 20, 2020

Choose a reason for hiding this comment

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

I think we should be using FlatCap on all (dashed) lines and RoundCap on dots, but never SquareCap as it makes the lines longer than expexted

Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Jul 20, 2020

Choose a reason for hiding this comment

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

1.5 indeed looks too dense, but 2.0 seems OK to me

@@ -44,7 +44,9 @@ void TieSegment::draw(QPainter* painter) const
case 1:
painter->setBrush(Qt::NoBrush);
pen.setWidthF(score()->styleP(Sid::SlurDottedWidth) * mag);
pen.setStyle(Qt::DotLine);
pen.setStyle(Qt::CustomDashLine);
pen.setDashPattern(QVector<qreal> {0.01, 2.5});
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

@elerouxx elerouxx left a comment

Choose a reason for hiding this comment

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

The problem with flat caps is that they trend to leave gaps in textline corners (hooks are separated lines so no use for joints here).
IMO, Qt lineStyles (dotted, dash dotted etc) are just internal pattern "presets" that are not well designed for caps other than flat. It would be cleaner and easier if MS just designed its own patterns for dotted/dashed dot/etc and defined them all together somewhere else, then always reference these custom internal patterns instead of using the generic Qt ones. (It would also be prettier than putting curly brackets with "magic" numbers all over the source files).
image

libmscore/slur.cpp Outdated Show resolved Hide resolved
@elerouxx elerouxx force-pushed the real-dotted-lines-3.x branch 2 times, most recently from 9f05247 to 06d4d4f Compare July 21, 2020 05:04
libmscore/slur.cpp Outdated Show resolved Hide resolved
libmscore/slur.cpp Outdated Show resolved Hide resolved
libmscore/slur.cpp Outdated Show resolved Hide resolved
libmscore/tie.cpp Outdated Show resolved Hide resolved
libmscore/tie.cpp Outdated Show resolved Hide resolved
break;
case 2:
painter->setBrush(Qt::NoBrush);
pen.setWidthF(score()->styleP(Sid::SlurDottedWidth) * mag);
pen.setStyle(Qt::DashLine);
pen.setStyle(Qt::CustomDashLine);
pen.setDashPattern(dashed);
break;
case 3:
painter->setBrush(Qt::NoBrush);
pen.setWidthF(score()->styleP(Sid::SlurDottedWidth) * mag);
pen.setStyle(Qt::CustomDashLine);
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@@ -33,6 +33,10 @@ void SlurSegment::draw(QPainter* painter) const
QPen pen(curColor());
qreal mag = staff() ? staff()->mag(slur()->tick()) : 1.0;

QVector<qreal> dotted = { 0.01, 1.99 }; // tighter than Qt Dotline equivalent { 0.01, 2.99 }
QVector<qreal> dashed = { 3.00, 3.00 };
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether here too we'd want to increase the gaps by the width to compensate for the SquareCap.
Just like you apparently do for the wide dashed line below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default Qt::DashLine is { 4.0, 2.0 }. To compensate for caps I changed it to { 3.0, 3.0 } already.
(I had first declared as { 3.01, 2.99 } but that felt a little too geeky since there are no dots involved. If it makes things clear I can do that).


//Replace generic Qt dash patterns with improved equivalents to show true dots
QVector<qreal> dotted = { 0.01, 1.99 }; // tighter than Qt Dotline equivalent { 0.01, 2.99 }
QVector<qreal> dashed = { 3.0, 3.0 };
Copy link
Contributor

Choose a reason for hiding this comment

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

see above, compensating for SquareCap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same. I better add a comment there // Qt DashLine is { 4.0, 2.0 }

QVector<qreal> dashed = { 3.0, 3.0 };
QVector<qreal> dashDotted = { 3.01, 2.99, 0.01, 2.99 };
QVector<qreal> dashDotDotted = { 3.01, 2.99, 0.01, 2.99, 0.01, 2.99 };
QVector<qreal> customDashes = { tl->dashLineLen(), tl->dashGapLen() };
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too, add one line width?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did better, { 3.0, 3.0 } is a dash with its gap (compensated already from {4,2}) and {0.01, 2.99} is a dot with its gap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{4.0, 2.0} is DashLine default. Compensated to {3.0, 3.0} for caps.
{1.0, 2.0} is DotLine default. Compensated for caps would be {0.0, 3.0} but zero doesn't work so I give 1 cent back from the gap to the dash - it would be { 0.1, 2.99} but our version of DotLine has smaller gaps { 0.01, 1.99}
The dashDotted and dashDotDotted use nominally 3.0 on every gaps. The 2.99 compensation for 0.01 dashes (ie. dots) is to keep the most consistent output along the line between dash/dashdot etc. so the gaps look aligned between styles when side to side.
Not that someone would tightly compare lines, but it looks neat if the composer wants to add instructions about lines somewhere.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't have real dots for the dashDotted and dashDotDotted lines, can we?

Copy link
Contributor Author

@elerouxx elerouxx Jul 21, 2020

Choose a reason for hiding this comment

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

Only if we assume RoundCaps for the dashes on these two, leaving SquareCaps only in Dashed and continuous.
Ideally, a new PR could add Cap Style as an option in inspector / styles / etc...
A new property to save to files and deal with and more one decision for the engraver. Would that be overkill?
EDIT: I'd prefer not to offer both flatcaps and squarecaps to final users if that was an option. Behavior is already confusing for us devs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any case, mixing straight dashes with round dots in the same line is not possible with Qt styles. Only if we create a method to calculate dashes and dots separately and then draws two overlapping lines. Just for this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I had a simple idea to draw a line with mixed round dots and sharp dashes. Would it be worth it?
I'll give it a try.

libmscore/tie.cpp Outdated Show resolved Hide resolved
@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Jul 21, 2020

These vtest fails are expected

@elerouxx elerouxx force-pushed the real-dotted-lines-3.x branch from 06d4d4f to 800f6c1 Compare July 21, 2020 16:08
@ecstrema
Copy link
Contributor

ecstrema commented Jul 23, 2020

image
Shouldn't this distance be calculated from the right?

I'll clarify a bit:

When the vertical line intersects with the horizontal line, it looks like the corner dots are really close one to each other. Closer than the other dots at least. A solution for this would be to draw the line from the right, instead of from the left, to avoid that thin space.

@ecstrema
Copy link
Contributor

image
Shouldn't this distance be calculated from the right?

I'll clarify a bit:

When the vertical line intersects with the horizontal line, it looks like the corner dots are really close one to each other. Closer than the other dots at least. A solution for this would be to draw the line from the right, instead of from the left, to avoid that thin space.

might be something for another PR though...

@elerouxx
Copy link
Contributor Author

elerouxx commented Jul 23, 2020

image
Shouldn't this distance be calculated from the right?

I'll clarify a bit:
When the vertical line intersects with the horizontal line, it looks like the corner dots are really close one to each other. Closer than the other dots at least. A solution for this would be to draw the line from the right, instead of from the left, to avoid that thin space.

might be something for another PR though...

It would be a good idea to draw the hook from the bottom and the line from the right but inverting that direction is not trivial. Also, no perfect solution because if there is a left hook it will have the same problem. Although the right hook is more "important" (more use cases). Maybe another PR indeed.

Fix #270337: True dotted lines
@elerouxx elerouxx force-pushed the real-dotted-lines-3.x branch from 800f6c1 to 3294093 Compare November 18, 2020 15:42
@vpereverzev vpereverzev added the vtests This PR produces approved changes to vtest results label Nov 30, 2020
@vpereverzev vpereverzev merged commit 7351cfe into musescore:3.x Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vtests This PR produces approved changes to vtest results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants