Honour an explicit dynamics="0" on MusicXML note import

[jonashogstrom]

Resolves: #33803

The dynamics attribute of the <note> element expresses the note velocity as a percentage of the MIDI 1.0 default forte value of 90. Positive values were imported correctly (dynamics="50" → velocity 45), but an explicit dynamics="0" — a silent note, e.g. a ghost note or an unpitched gesture written with an x notehead — was indistinguishable from an absent attribute (doubleAttribute() returns 0 in both cases) and was silently dropped by the velocity > 0 guard, so the note played at full default velocity.

This PR detects the attribute's presence with hasAttribute() instead of testing the computed value, and clamps the imported velocity to [1, 127]:

• The lower bound of 1 is the lowest value representable in the score model, which reserves velocity 0 for "unset" (Note::customizeVelocity() also clamps to this range at playback time, so 1 is what a "silent" note effectively plays at in all backends).
• The upper bound matches the [0, 127] clamping already applied to direction-level <sound dynamics> values; previously e.g. dynamics="200" was stored as velocity 180, above the MIDI maximum, which every playback path clamps anyway and which round-tripped back out as an out-of-range dynamics value.

Test: new noteDynamics unit test importing a five-note file covering the absent attribute (velocity stays unset), dynamics="0" (→ 1), dynamics="50" (→ 45, the already-working case), and dynamics="200" (→ clamped to 127).

Verified end-to-end: a measure with a dynamics="0" note now exports MIDI with that note at velocity 1 (effectively silent) while unmarked notes are unaffected; previously it played at the full default velocity 80.

• I signed the CLA as jonas.hogstrom
• The title of the PR describes the problem it addresses.
• Each commit's message describes its purpose and effects, and references the issue it resolves. If changes are extensive, there is a sequence of easily reviewable commits.
• The code in the PR follows the coding rules.
• I understand all aspects of the code I'm contributing and I'm able to explain it if requested.
• The code compiles and runs on my machine, preferably after each commit individually. I have manually tested and verified that my changes fulfil their intended purpose.
• No prior attempts to resolve this problem exist, or if they do, I listed them in my PR description and described how I avoided repeating past mistakes.
• There are no unnecessary changes.
• I created a unit test or vtest to verify the changes I made (if applicable).

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 655e8999-39af-4944-9ea0-5ba44e4ec9e6

📥 Commits

Reviewing files that changed from the base of the PR and between 80f1fe3 and 3339e9c.

📒 Files selected for processing (4)

• src/importexport/musicxml/internal/import/importmusicxmlpass2.cpp
• src/importexport/musicxml/tests/data/testNoteDynamics.xml
• src/importexport/musicxml/tests/data/testNoteDynamics_ref.mscx
• src/importexport/musicxml/tests/musicxml_tests.cpp

✅ Files skipped from review due to trivial changes (1)

• src/importexport/musicxml/tests/data/testNoteDynamics_ref.mscx

🚧 Files skipped from review as they are similar to previous changes (2)

• src/importexport/musicxml/tests/data/testNoteDynamics.xml
• src/importexport/musicxml/internal/import/importmusicxmlpass2.cpp

---

📝 Walkthrough

Walkthrough

This PR fixes velocity handling in the MusicXML pass 2 importer for the dynamics attribute on notes. The implementation now distinguishes between a missing dynamics attribute (no user velocity set) and an explicit dynamics="0" (clamped to velocity 1). Two changes occur in importmusicxmlpass2.cpp: the velocity calculation only happens when the attribute exists, and the guard for calling setUserVelocity() checks for attribute presence rather than a positive velocity value. A new test XML file and GoogleTest case validate the behavior across missing attributes, zero values, mid-range values, and out-of-range values.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Honour an explicit dynamics="0" on MusicXML note import' clearly and specifically describes the main change: fixing import handling of zero-value dynamics attributes.
Description check	✅ Passed	The description is comprehensive, references issue #33803, explains the root cause and fix, includes clamping rationale, and all checkboxes are marked complete with clear technical details.
Linked Issues check	✅ Passed	All primary coding objectives from issue #33803 are met: the fix distinguishes absent attributes from dynamics="0", imports it correctly as velocity 1, applies proper clamping [1,127], and maintains backward compatibility with testing.
Out of Scope Changes check	✅ Passed	All changes are directly related to the issue: modified parsing logic, added test fixture file, created unit test, and added reference file—no unrelated alterations detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Linked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped musescore/muse_framework.git.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Jojo-Schmitz]

Why not using a testNoteDynamics_ref.mscx and

TEST_F(MusicXml_Tests, noteDynamics) {
    musicXmlImportTestRef("testNoteDynamics");
}

Check my backport to 3.x in Jojo-Schmitz#1223

[jonashogstrom]

Done in the latest push. Converted noteDynamics to musicXmlImportTestRef("testNoteDynamics") with a committed testNoteDynamics_ref.mscx, matching the convention in the rest of the file.

The reference covers the same four cases the test data exercises — dynamics="0" → velocity 1, dynamics="50" → 45, dynamics="200" → 127 (clamped), and no <velocity> for notes without the attribute. I moved the per-case reasoning into a comment at the top of testNoteDynamics.xml so the expected values stay documented.

[Jojo-Schmitz]

I live it much better that way, thanks