Issue 5345: Stop crash while merging ledger lines (issue 343270043 by d...@gnu.org)
LGTM. https://codereview.appspot.com/343270043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5344: Avoid repeated calculation of accepted contexts (issue 346080043 by nine.fierce.ball...@gmail.com)
On 2018/06/17 18:13:20, dak wrote: I must be getting senile. I didn't spot where you make the copy. In fairness, the copy was not as obvious in patch set 1, though that's not proof you're not getting senile. :D https://codereview.appspot.com/346080043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5344: Avoid repeated calculation of accepted contexts (issue 346080043 by nine.fierce.ball...@gmail.com)
On 2018/06/17 17:56:06, Dan Eble wrote: On 2018/06/17 17:51:34, Dan Eble wrote: > test that \with \denies ... has local effect Thanks for the clue. The new test passes with the code I've submitted and fails if Acceptance_set::assign_copy is modified not to make a copy. I must be getting senile. I didn't spot where you make the copy. https://codereview.appspot.com/346080043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5344: Avoid repeated calculation of accepted contexts (issue 346080043 by nine.fierce.ball...@gmail.com)
On 2018/06/17 17:51:34, Dan Eble wrote: test that \with \denies ... has local effect Thanks for the clue. The new test passes with the code I've submitted and fails if Acceptance_set::assign_copy is modified not to make a copy. https://codereview.appspot.com/346080043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5344: Avoid repeated calculation of accepted contexts (issue 346080043 by nine.fierce.ball...@gmail.com)
On 2018/06/17 13:27:39, Dan Eble wrote: On 2018/06/17 12:52:01, Dan Eble wrote: > Stop uniquifying \accepts; clarify copying I also spent time trying to find input that would "let \with blocks destructively change global context defs." I was surprised that I couldn't make it happen even after temporarily changing the Acceptance_set list copies to plain old assignments of SCM values. My approach was to instantiate two contexts, with the first context having a \with ... \denies ... and the next context using the defaults for its type. Whether the contexts began sequentially or simultaneously, the impact of \denies was limited to the first context. It's strange, but I'm reluctant to spend more time trying to discover why I can't force a failure even with code that I'm not actually submitting. For scm_delete_x or its Scheme equivalent of delete! to have an actually destructive effect on other list copies, the removed element must not be the first in the list. That makes constructing failing examples a bit more complex since you always have to add additional fluff before an effect may become apparent. https://codereview.appspot.com/346080043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5344: Avoid repeated calculation of accepted contexts (issue 346080043 by nine.fierce.ball...@gmail.com)
On 2018/06/17 12:52:01, Dan Eble wrote: Stop uniquifying \accepts; clarify copying I also spent time trying to find input that would "let \with blocks destructively change global context defs." I was surprised that I couldn't make it happen even after temporarily changing the Acceptance_set list copies to plain old assignments of SCM values. My approach was to instantiate two contexts, with the first context having a \with ... \denies ... and the next context using the defaults for its type. Whether the contexts began sequentially or simultaneously, the impact of \denies was limited to the first context. It's strange, but I'm reluctant to spend more time trying to discover why I can't force a failure even with code that I'm not actually submitting. https://codereview.appspot.com/346080043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add and use a Transform data type (issue 344970043 by d...@gnu.org)
On 2018/06/17 10:54:38, dak wrote: Right. I haven't rebased this patch yet, but I naturally will have to load up another version for review now that the fix is in master. I actually lean towards doing this in a manner similar to your original approach, namely just using the stencil rotation call. Basically I'd want to completely replace both sten and s with a rotated copy. This will make a difference for later uses of s, notably // we use the bounding box if there are no boxes // FIXME: Rotation? if (!boxes.size () && !buildings.size ()) boxes.push_back (Box (s->extent (X_AXIS), s->extent (Y_AXIS))); The question is whether using the rotated version here is supposedly an improvement, detrimental or "don't care". Regarding the "relative" implementation of the origin location, the bounding boxes resulting from empty extents are likely to be garbage (likely NaN) after rotation. https://codereview.appspot.com/344970043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add and use a Transform data type (issue 344970043 by d...@gnu.org)
Reviewers: Be-3, https://codereview.appspot.com/344970043/diff/1/lily/stencil-integral.cc File lily/stencil-integral.cc (right): https://codereview.appspot.com/344970043/diff/1/lily/stencil-integral.cc#newcode1099 lily/stencil-integral.cc:1099: Offset center (robust_scm2double (scm_cadr (rot), 0.0), robust_scm2double (scm_caddr (rot), 0.0)); On 2018/06/17 10:46:37, Be-3 wrote: + center[X_AXIS] = s->extent (X_AXIS).linear_combination (center[X_AXIS]); + center[Y_AXIS] = s->extent (Y_AXIS).linear_combination (center[Y_AXIS]); (or something similar) Here, we've got the rotation centre problem as in issue 5346. Standard regtests can't see it, but PNG based regtests show differences for skyline-grob-rotation.ly and stencil-color-rotation.ly skyline-boxes-ellipses.ly shows up, too, but there are only minimal deviations due to rounding issues. Right. I haven't rebased this patch yet, but I naturally will have to load up another version for review now that the fix is in master. Description: Add and use a Transform data type As of now, this is just used in stencil-integral. However, a Scheme level interface would likely make sense for compacting geometric transformations. The abstraction currently relies on PangoTransform matrices but could easily be moved to other implementations such as cairo_matrix_t. Contains commits: Add Transform data type stencil-integral.cc: use Transform stencil-integral: pass SCM as transform values Please review this at https://codereview.appspot.com/344970043/ Affected files (+314, -143 lines): A lily/include/transform.hh M lily/stencil-integral.cc A lily/transform.cc ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Add and use a Transform data type (issue 344970043 by d...@gnu.org)
https://codereview.appspot.com/344970043/diff/1/lily/stencil-integral.cc File lily/stencil-integral.cc (right): https://codereview.appspot.com/344970043/diff/1/lily/stencil-integral.cc#newcode1099 lily/stencil-integral.cc:1099: Offset center (robust_scm2double (scm_cadr (rot), 0.0), robust_scm2double (scm_caddr (rot), 0.0)); + center[X_AXIS] = s->extent (X_AXIS).linear_combination (center[X_AXIS]); + center[Y_AXIS] = s->extent (Y_AXIS).linear_combination (center[Y_AXIS]); (or something similar) Here, we've got the rotation centre problem as in issue 5346. Standard regtests can't see it, but PNG based regtests show differences for skyline-grob-rotation.ly and stencil-color-rotation.ly skyline-boxes-ellipses.ly shows up, too, but there are only minimal deviations due to rounding issues. https://codereview.appspot.com/344970043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
