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

Improvements around hairpins and dynamics, item snapping, alignment, staff centering, and autoplace #23046

Merged

Conversation

mike-spa
Copy link
Contributor

@mike-spa mike-spa commented May 31, 2024

Resolves: #16845 (and much else)

  • Lots of small improvements and fixes in the dragging behaviour of isolated hairpins and dynamics (dragging groups of aligned hairpins/dynamics still needs a lot more work, which I'm planning for following PRs).
  • New code infrastructure to represent linked chains of aligned elements.
  • New code infrastructure (collected in the AlignmentLayout class) to calculate the alignment of items, which is now done at the system-layout level, rather than the individual item level (and is also done more precisely).
  • Improved implementation of the center-between-staves function.
  • New logic to manage the cases where items need to centered between staves and also stay aligned to each other.
  • New options to explicitely set whether hairpins should align or not with preceding/following dynamics (or preceding/following hairpins), with corresponding new UI.
  • New options to align gradual tempo change lines with tempo text, with corresponding new UI.
  • Small technical improvements and corrections to the Shape class.
  • Introduced a score-wide value for minimum horizontal clearence in the vertical calculations of skylines. This is a temporary implementation: eventually the vertical spacing system will have a similar implementation to the horizontal spacing system with specific item-by-item values.

@mike-spa mike-spa force-pushed the improveDynamicAndHairpinDragging branch 4 times, most recently from 6a83e75 to 71e87e5 Compare June 3, 2024 15:57
@mike-spa mike-spa force-pushed the improveDynamicAndHairpinDragging branch 4 times, most recently from 549e20a to 6ea9fc6 Compare June 6, 2024 11:39
@mike-spa mike-spa changed the title Improvements around hairpins and dynamics Improvements around hairpins and dynamics, item snapping, alignment, staff centering, and autoplace Jun 6, 2024
@oktophonie oktophonie added vtests This PR produces approved changes to vtest results engraving labels Jun 6, 2024
@mike-spa mike-spa force-pushed the improveDynamicAndHairpinDragging branch from 6ea9fc6 to cc4dedf Compare June 7, 2024 15:22
@mike-spa mike-spa force-pushed the improveDynamicAndHairpinDragging branch 3 times, most recently from 171da23 to 09f45ab Compare June 10, 2024 13:41
@mike-spa
Copy link
Contributor Author

@RomanPudashkin the utests are fixed now. It's ready for review, when you can.

@mike-spa mike-spa force-pushed the improveDynamicAndHairpinDragging branch from 09f45ab to 8bd516c Compare June 11, 2024 13:32
@@ -604,6 +622,11 @@ class EngravingItem : public EngravingObject
double m_mag = 1.0; // standard magnification (derived value)
ld_field<PointF> m_pos = "pos"; // Reference position, relative to _parent, set by autoplace
ld_field<Shape> m_shape = "shape";

EngravingItem* m_itemSnappedBefore = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that there will be 2 problems with this change:

  1. these items may be deleted before the item storing them, so we will get invalid pointers here. And there will be no easy way to update the pointers, since these items probably don't have a parent-child relationship
  2. I also suspect that these pointers might start being used quite uncontrollably to change item properties at unexpected times (i.e. for hacks). Adding const could help at least with this problem...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree. Unfortunately I can't see a better way of doing this at the moment. In order to achieve these engraving requirements the items need to be aware of each other in a non-const way, because they must be able to influence each other's layout properties, position in the score, and even UI behaviour (drop operations, dragging, etc). At the moment this works safely because I have made sure that these connections get reset and recomputed at every layout, which means they never survive editing operations, so they don't risk pointing to invalidated stuff. Also, they are stored in LayoutData which is never copied so that's also safer. But yes I agree that it's not ideal.

I did try to explore a different solution when I started: instead of having item A point to B and B point to A, create a new entity C which holds the pointers to A and B. But in the end I dropped it because it wasn't much better: C still holds pointers to A and B which could become invalidated; we would have to manage a large number of C items (one for every snapping-chain) which also adds performance cost; then who is the owner of C? etc.

I'm not sure that there is a "good" solution to this until our library uses only raw pointers. One day, these could all be weak_ptr...

@RomanPudashkin RomanPudashkin merged commit 51a2537 into musescore:master Jun 12, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engraving vtests This PR produces approved changes to vtest results
Projects
Development

Successfully merging this pull request may close these issues.

Hairpin alignment can break even if dynamics are placed
3 participants