-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Calculate rest vertical adjustment after cross staff stem layout #19828
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
miiizen
force-pushed
the
19598-cross-beam-rest
branch
from
November 1, 2023 17:17
2691dc6
to
49c94e3
Compare
miiizen
force-pushed
the
19598-cross-beam-rest
branch
from
November 8, 2023 12:02
49c94e3
to
61ca9de
Compare
oktophonie
approved these changes
Nov 8, 2023
cbjeukendrup
reviewed
Nov 8, 2023
// Pad rest shape by small amount so horizontal distance between rest and beam is more than 0 | ||
// A horizontal distance of 0 counts as no vertical collision | ||
RectF bbox = restShape.bbox(); | ||
restShape.addBBox(RectF(bbox.topRight(), PointF(bbox.bottomRight().x() + 1, bbox.bottomRight().y()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of just + 1
, might it be desirable to add some spatium-based value? Or does it really not matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miiizen this is a good thing to do, but:
- Like Casper said, please use a spatium-based value for this padding and give it a descriptive name.
- This implementation (adding a rectangle to the Shape that's slightly larger than the Shape itself) is quite hacky. It would be better (and probably also useful in general) to change the verticalClearance method to something like
Shape::verticalClearance(otherShape, minHorizontalDistance)
, where minHorizontalDistance default-initializes to zero so it doesn't change where it's already used. Then, inside verticalClearance, you can add minHorizontalDistance to the left/right edges as needed before checking intersections :)
cbjeukendrup
approved these changes
Nov 9, 2023
mike-spa
approved these changes
Nov 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves: #19598
Resolves: #18555
Resolves: (see image)
Enables our rest & beam collision avoidance for beams which are completely moved to another staff and for upstemmed beams where the first item is a rest.