Tapping implementation #28245
Tapping implementation #28245
Conversation
mike-spa
force-pushed
the
tapping
branch
3 times, most recently
from
3b3f078 to
9f68000
Compare
bkunda
left a comment
bkunda
left a comment
There was a problem hiding this comment.
Aside from the open discussion happening internally about the design, there are a couple of trivial issues that'd be good to iron out at this stage:
- Palette icon colours not correct in dark mode
tapping-palettes_colour.mov
- Half slurs not selected when added
tapping_not-selected-palette.mov
mike-spa
force-pushed
the
tapping
branch
4 times, most recently
from
2b92bbf to
a84b31f
Compare
ghost
requested a review
from 
mike-spa
force-pushed
the
tapping
branch
2 times, most recently
from
ee2df56 to
f0e727c
Compare
ghost
added
the
miiizen
left a comment
miiizen
left a comment
There was a problem hiding this comment.
Great job, I've only spotted a few small changes
| tappings.push_back(toTapping(a)); | ||
| } | ||
| } | ||
| DO_ASSERT(tappings.size() <= 1); |
There was a problem hiding this comment.
It feels odd to me to enforce/check this when getting tappings - it might make more sense to do this check when adding to a chord?
There was a problem hiding this comment.
Yeah you're right, but it feels even weirder to count them when inserting, cause it means adding an extra for loop that does nothing (whereas here the loop would be needed anyway to find the Tapping, so the assertion check can be done for free)
src/engraving/dom/tapping.h
Outdated
| TappingText(const TappingText& t); | ||
| TappingText* clone() const override { return new TappingText(*this); } | ||
|
|
||
| virtual Color curColor() const; |
There was a problem hiding this comment.
override rather than virtual
| case ElementType::GLISSANDO: return new Glissando(parent); | ||
| case ElementType::BRACKET: return new Bracket(parent); | ||
| case ElementType::ARTICULATION: return new Articulation(parent->isChordRest() ? toChordRest(parent) : dummy->chord()); | ||
| case ElementType::TAPPING: return new Tapping(parent->isChordRest() ? toChordRest(parent) : dummy->chord()); |
There was a problem hiding this comment.
We're missing the following cases in this switch: HAMMER_ON_PULL_OFF_SEGMENT, HAMMER_ON_PULL_OFF_TEXT, TAPPING_HALF_SLUR, TAPPING_HALF_SLUR_SEGMENT, TAPPING_TEXT
| const double slurCollisionHorizOffset = 0.2 * spatium; | ||
| const double fuzzyHorizCompare = 0.25 * spatium; | ||
| auto compare = [fuzzyHorizCompare](double x1, double x2) { return std::abs(x1 - x2) < fuzzyHorizCompare; }; | ||
| for (SpannerSegment* seg1 : segments) { |
There was a problem hiding this comment.
| for (SpannerSegment* seg1 : segments) { | |
| for (SlurSegment* seg1 : segments) { |
Every segment should be a slur segment here - you can remove checks & conversions below
| mask.setWidth(0.5 * mask.width()); | ||
| const double maskPad = 0.25 * item->spatium(); | ||
| mask.adjust(-maskPad, -maskPad, 0.0, maskPad); | ||
| slurSeg->mutldata()->setMask(mask); |
4 participants
Tapping implementation