Follow-up: Fix chord bracket layout across visual staves - #34434
Follow-up: Fix chord bracket layout across visual staves#34434pyonpyoco wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis change adds a dedicated 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engraving/rendering/score/chordbracketlayout.cpp`:
- Around line 84-129: Update bracketVisualSpan to apply the same 2 * spatium
minimum-length adjustment used by TLayout::layoutChordBracket() after computing
topY and bottomY, shifting both bounds equally to preserve the bracket hook
position. Ensure the adjusted span is used before the offset is applied and
returned, so bracketCollisionShape, layoutHorizontal, and
updateHorizontalSpacing match the rendered bracket.
- Around line 241-246: Update the updateForBrackets lambda in layoutSegment to
skip any bracket whose chord() is null before calling chord-dependent methods
such as onTabStaff(), bracketVisualSpan(), or pos().x(), matching the existing
guard used for segment entries.
- Around line 278-288: The updateVerticalGeometry method must regenerate the
chord bracket’s owner segment shape after relayout. After
TLayout::layoutItem(bracket, ctx), invoke TLayout::layoutChordBracket with the
bracket, its mutable layout data, and ctx.conf(), preserving the existing
horizontal-position restoration and avoiding ChordLayout::fillShape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b79776d6-1a47-47b8-a7ad-4c15ac0aba2d
📒 Files selected for processing (9)
src/engraving/rendering/score/chordbracketlayout.cppsrc/engraving/rendering/score/chordbracketlayout.hsrc/engraving/rendering/score/chordlayout.cppsrc/engraving/rendering/score/horizontalspacing.cppsrc/engraving/rendering/score/measurelayout.cppsrc/engraving/rendering/score/pagelayout.cppsrc/engraving/rendering/score/rendering.cmakesrc/engraving/rendering/score/scorehorizontalviewlayout.cppsrc/engraving/rendering/score/tlayout.cpp
💤 Files with no reviewable changes (1)
- src/engraving/rendering/score/chordlayout.cpp
|
Visual-test differences are present in In the attached example, the required horizontal spacing is A (upper staff, without ledger lines) < B (lower staff, with ledger lines). Previously, the two-staff case C followed A because spacing was based on the owner staff. This PR changes it to use Could you confirm whether the reference images should be updated for this change? 
|
4d5329d to
3a93707
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
3 participants
Follow-up to #33983 (comment)
This PR extends the previous chord bracket spacing fix to cases where the relevant chords are distributed across visual staves.
Problems addressed
Case 1
A chord bracket is visually contained within a single staff, but one of the chords spanned by the bracket is a cross-staff chord.
Case 2
A chord bracket spans chords that are visually placed on multiple staves within the same part.
In the Case 2 matrix,
*marks a rendering where the accidental and chord bracket collide.The matrix covers 16 combinations of the relative pitch ordering of the bracket anchors and voices 1 and 2 on the upper and lower staves. Each combination is repeated with the accidental placed on each of the four chords, producing 64 renderings in total.
In these cases, accidentals on the relevant chords were not consistently taken into account when positioning the chord bracket.
Root cause and design rationale
Chord bracket layout previously ran at Chord scope. In cross-staff cases, some of the geometry needed for placement belongs to another visual staff and may not yet be available, depending on staff and track iteration order.
The layout is therefore moved to Segment scope, where all chords at the same tick can be considered together. The process consists of initial horizontal placement after staff-local chord layout, horizontal spacing between Segments, and a final vertical update after staff distances are known.
The existing tablature layout path is left unchanged.
Implementation
A new
ChordBracketLayoutcomponent handles pitched chord brackets at Segment scope. Its methods correspond to the three layout stages:layoutSegment()performs the initial horizontal placement.updateHorizontalSpacing()accounts for brackets when spacing adjacent Segments.updateVerticalGeometry()performs the final update after staff spacing.MeasureLayoutruns the Segment-level pass before Segment Shapes are finalized, while page and horizontal view layout perform the final vertical update after staff spacing.The chord-level layout and relayout passes for pitched chord brackets are removed.
TLayoutalso normalizes reversed top and bottom coordinates when the bracket endpoints are manually edited.Tests
Manually verified the layout results for both cases shown above.
Test score: 33883_chordBracket4c.zip