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

WIP: fix #271198: Add a more flexible way to choose noteheads in drumset for percussion instrument #3620

Closed
wants to merge 3 commits into from

Conversation

lasconic
Copy link
Contributor

@lasconic lasconic commented Apr 9, 2018

No description provided.

@lasconic lasconic added the work in progress not finished work or not addressed review label Apr 9, 2018
@@ -111,9 +113,46 @@ EditDrumset::EditDrumset(const Drumset* ds, QWidget* parent)
pitchList->setColumnWidth(1, 60);
pitchList->setColumnWidth(2, 30);

QComboBox* combos[] = { wholeCmb, halfCmb, quarterCmb, doubleWholeCmb };
for (QComboBox* combo : combos) {
// TODO replace smuflRanges by a list of selected symbols
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danieljray Could you provide a full list of notehead glyphs from smufl that would make sense for drums. Not only for VDL but more globally? Right now, I used every single one in the noteheads range http://www.smufl.org/version/latest/range/noteheads/ which is too much (parenthesis etc...) but not enough since http://www.smufl.org/version/latest/range/roundAndSquareNoteheads/ and http://www.smufl.org/version/latest/range/slashNoteheads/ are not included... So we will need to hardcode a list...

Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend to add the following, currently missing from 3.0:

noteheadXOrnate
noteheadTriangleRightBlack
noteheadCircleSlash
noteheadRoundWhiteWithDot
noteheadLargeArrowUpBlack

else if (tag == "noteheads") {
while (e.readNextStartElement()) {
const QStringRef& tag(e.name());
_drum[pitch].noteheads[int(NoteHead::name2type(tag.toString()))] = Sym::name2id(e.readElementText());
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit dangerous for the changes in future formats, may lead to crash. I would add index boundary check before. I will add within a weekend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. It's rough code for the moment.

@@ -119,6 +119,7 @@ class NoteHead final : public Symbol {
HEAD_H,
HEAD_H_SHARP,

HEAD_CUSTOM,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Watch out, the addition of this custom type can have weird implication since HEAD_GROUPS is used to "loop through" the enum. I believe I catch all problems but still, review welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'm working on it. What is the best way to introduce changes? Should I make a PR based on your changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, you can do a PR based on my branch. Let's talk when you are done.

for (auto symName : (*smuflRanges())[range]) {
SymId id = Sym::name2id(symName);
if (!excludeSym.contains(symName)) {
QIcon icon;
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 painting here works well with some glyphs and less well with others.. no idea why for the moment. The glyph are supposed to be positioned the same.

@lasconic
Copy link
Contributor Author

See #3631

@lasconic lasconic closed this Apr 19, 2018
@anatoly-os anatoly-os removed the work in progress not finished work or not addressed review label Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants