Enforce minimum stem length for notes within beamed groups #23197
Conversation
miiizen
force-pushed
the
18280-min-stemlen
branch
from
b01c052
to
67be097
Compare
miiizen
force-pushed
the
18280-min-stemlen
branch
from
67be097
to
2f3d262
Compare
miiizen
force-pushed
the
18280-min-stemlen
branch
from
2f3d262
to
e188ba3
Compare
|
|
||
| double startY = (isStartDictator ? dictator : pointer) * ldata->spatium / 4 + tolerance; | ||
| double endY = (isStartDictator ? pointer : dictator) * ldata->spatium / 4 + tolerance; | ||
| // min stem lengths according to how many beams there are (starting with 1) | ||
| static const int minStemLengths[] = { 11, 13, 15, 18, 21, 24, 27, 30 }; |
There was a problem hiding this comment.
Couple of things about this.
a) It is unclear at first sight in which units these values are represented. I'm guessing, from the way that you are using it later, that these are quarter-spaces? I think it would be clearer if you expressed these numbers in units of spatium (also good to write a comment that says so). This will also save you from doing that strange * 4 multiplication in desiredLen.
b) Please make this into a function, so its meaning and use are more transparent. Something like
double BeamTremoloLayout::minStemLength(int beamsCount) { ... }. Then define this array inside the funtion itself and that way you can also safe guard against out-of-range access by checking that beamsCount is within range.
c) You can make it not just const but constexpr, that way it will be evaluated at compile time.
There was a problem hiding this comment.
By the way, I know that this thing of using quarter-spaces is done a lot in the beam layout code (and it's totally fine given that beams adjust in quarter-space increments) so I wasn't critizing you for that. I just want to avoid perpetuating the bad practice of doing * 4 and / 4 all the time, which is obscure and prone to mistakes :)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| || (!ldata->up && muse::RealIsEqualOrMore(desiredY, anchor.y() - reduction)); | ||
| if (beamClearsAnchor) { | ||
| // avoid division by zero for zero-length beams (can exist as a pre-layout state used | ||
| // for horizontal spacing computations) |
There was a problem hiding this comment.
This comment is about the if condition, so better move it out of the while loop
| if (beamClearsAnchor) { | ||
| // avoid division by zero for zero-length beams (can exist as a pre-layout state used | ||
| // for horizontal spacing computations) | ||
| const double proportionAlongX = (anchor.x() - startX) / (endX - startX); |
There was a problem hiding this comment.
If this variable doesn't change within the loop it should be moved out, so we save having to recalculate it each time.
miiizen
force-pushed
the
18280-min-stemlen
branch
from
e188ba3
to
3bc8890
Compare
Resolves: #18280