Convert LINE_WIDTH property to spatium #23875
Conversation
miiizen
force-pushed
the
line-width-to-sp
branch
from
d65af90
to
cb92d2b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
src/engraving/dom/stem.h
Outdated
| Millimetre lineWidth() const { return m_lineWidth; } | ||
| double lineWidthMag() const { return m_lineWidth * mag(); } | ||
| void setLineWidth(Millimetre lineWidth) { m_lineWidth = lineWidth; } | ||
| double point(Spatium sp) const override { return sp.val() * score()->style().spatium(); } |
There was a problem hiding this comment.
I don't think this should use score()->style().spatium() though, or should it? Cause that returns the score-wide value, but this item may be on a small (or otherwise scaled) staff. We should always use item->spatium().
Also, like Casper, I too prefer these implementations to be in the .cpp file (the idea being that .h files should #include as few other .h files as possible)
There was a problem hiding this comment.
This gives correct results - scaling is done in lineWidthMag() which scales by mag(). mag() applies both the stave spatium and the chord's scale. But actually maybe there's a UI issue I missed:
If you have a scaled stave, the "Thickness" property appears to the user to be measured in relation to the stave - (eg. by default always 0.1sp which is what we want). If you then make the chord cue size, the stem's thickness is reduced, but the value appears to remain at 0.1sp.
I can change point to use item->spatium and lineWidthMag to use chord()->instrinsicMag to make this more clear. There might have to be some file compatibility changes for reading old files.
src/engraving/dom/tuplet.h
Outdated
| Spatium bracketWidth() const { return m_bracketWidth; } | ||
| void setBracketWidth(Spatium s) { m_bracketWidth = s; } | ||
|
|
||
| double point(Spatium sp) const override { return sp.val() * score()->style().spatium(); } |
There was a problem hiding this comment.
Potentially the same spatium/chord mag confusion. If so, we won't need these overrides at all.
src/engraving/dom/engravingitem.h
Outdated
| @@ -403,7 +403,7 @@ class EngravingItem : public EngravingObject | |||
|
|
|||
| bool isPrintable() const; | |||
| bool isPlayable() const; | |||
| double point(const Spatium sp) const { return sp.val() * spatium(); } | |||
| virtual double point(const Spatium sp) const { return sp.val() * spatium(); } | |||
There was a problem hiding this comment.
I like what this method does, but I find the name a bit misleading. "Point" for me is immediately associated to an (x, y) pair of data. Can we think of something a bit clearer? Perhaps something like double absoluteLenFromSpatium(const Spatium), which could have in future a corresponding opposite absoluteLenToSpatium. @cbjeukendrup what do you think?
Also, pure pedantry: the const keyword isn't really necessary when the parameter is passed by copy (as opposed to passing by reference, where const makes a big difference).
There was a problem hiding this comment.
Or even just absoluteFromSpatium, as it's not always necessarily a "length".
miiizen
force-pushed
the
line-width-to-sp
branch
from
6083503
to
211c648
Compare
miiizen
force-pushed
the
line-width-to-sp
branch
from
211c648
to
312b041
Compare
Resolves: #19367
Resolves: #23040
This PR changes the type of the
LINE_WIDTHproperty to Spatium from Millimetre. The Millimetre type was causing some issues and inconsistencies between the score and parts and with spanners, due to it needing to be rescaled on any spatium change and this being applied inconsistently. It was also in some cases not true to size. For example, text lines set to a thickness of 0.15sp were actually 0.1488sp.