LilyIssues / Issues / #3383 old-straight-flag + smaller Stem.thickness gives no output and huge over

Google Importer - 2013-05-31

Originally posted by: k-ohara5...@oco.net

This went wrong with the patch for issue 3161. I reduced the example to one stem and flag, and see that the problem is when the tip of the stem is exactly at a corner of the flag.
Probably something in an intersection-finding algorithm is wrong, in the code that merges the skylines. There are points with Y-coordinates of 128.000 in the merged skyline, hinting at a 0.0/0.0 division.
If the code before the issue 3161 patch avoided this, it seems it was by removing the just-one-point 'building' where the flag protrudes (by zero distance) from the tip of the stem.

\layout {
\context {
    \Staff
    \override TimeSignature #'stencil = ##f
    \override Clef #'stencil = ##f
    \override NoteHead #'stencil = #point-stencil
    \override StaffSymbol #'stencil = ##f
} \context {
    \Score
    \override Stem.thickness = #1.20000000
    \override Flag.stencil = #old-straight-flag
} }
{ d'8 }
#(ly:set-option 'debug-skylines)

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-05-31

Originally posted by: k-ohara5...@oco.net

It is not quite what I thought. The vertical sides of the flag are being stored in slope/intercept form, after enough processing to make them not perfectly vertical due to round-off. Stem.thickness affects the exact position of the left edge, and thus the exact near-infinite slope.

Limiting the slope to be below a (justifiable) magic value in Building::precompute() resolves the problem.

http://codereview.appspot.com/9890045

Labels: Patch-new
Owner: k-ohara5...@oco.net
Status: Started

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-01

Originally posted by: pkx1...@gmail.com

Patchy the autobot says: passes make, make test and a full make doc.

Labels: -Patch-new Patch-review

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-02

Originally posted by: pkx1...@gmail.com

Patch on countdown for June 6 - 06:00 GMT

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-03

Originally posted by: k-ohara5...@oco.net

(No comment was entered for this change.)

Labels: -Patch-countdown Patch-new

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-04

Originally posted by: dak@gnu.org

Patchy the autobot says: passes tests.

Labels: -Patch-new Patch-review

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-05

Originally posted by: pkx1...@gmail.com

David had some comments that made this look like (to me anyway) that it needed work still.

Labels: -Patch-review Patch-needs_work

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-06

Originally posted by: dak@gnu.org

Issue3383: old-straight-flag + smaller Stem.thickness gives no output and huge over

This replaces the numerically imprudent y_intercept_ in buildings with
start_height_ and adapts calculations.

The one thing I have no actual clue about is what
Skyline::set_minimum_height does and how. So the code in there might
be wrong. It is based on the assumption that a skyline starts at
minus infinity and has its first start_height_ equal to the previous
y_intercept_.

http://codereview.appspot.com/10086043

Labels: -Patch-needs_work Patch-new
Owner: dak@gnu.org

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-06

Originally posted by: dak@gnu.org

Patchy the autobot says: LilyPond memory use goes ballistic, like on size20.ly

Labels: -Patch-new Patch-needs_work

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-07

Originally posted by: dak@gnu.org

Try mimicking old behavior more slavishly

http://codereview.appspot.com/10086043

Labels: -Patch-needs_work Patch-new

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-07

Originally posted by: pkx1...@gmail.com

Passes Make, make check and full make doc.

Didn't check memory usage but didn't really look that hard (I have 4GB assigned to RAM on Patchy)

Reg test diffs

https://www.yousendit.com/download/WFJVK3BCbEFtUUhtcXRVag

Labels: -Patch-new Patch-review

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-07

Originally posted by: dak@gnu.org

I clamped memory usage down to 1G with three LilyPond processes, and got through. Memory stats also seemed not to differ much, visuals looked reasonable. For me, event-listener-output looked worse: if that's not some oscillating effect from elsewhere, it would seem to point to a problem.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-07

Originally posted by: dak@gnu.org

Color me annoyed. With some slight changes, I see the same regtests differences as you, in particular also the same change in event-listener-output regtest. However, compiling the regtest on its own, with or without -ddebug-skylines, does not seem to show the spacing problem. Huh. Does not exactly make it easier to track it down.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-08

Originally posted by: dak@gnu.org

Make a pitch for addressing Keith's concerns

http://codereview.appspot.com/10086043

Labels: -Patch-review Patch-new

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-08

Originally posted by: pkx1...@gmail.com

Passes Make, make check and full make doc.

Diffs at

https://www.yousendit.com/download/WFJWM25LbEp3NUlpR01UQw

Labels: -Patch-new Patch-review

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-08

Originally posted by: dak@gnu.org

When adjusting start_, need to adjust start_height_ as well

http://codereview.appspot.com/10086043

Labels: -Patch-review Patch-new

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-08

Originally posted by: dak@gnu.org

Patchy the autobot says: passes tests.

Labels: -Patch-new Patch-review

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-08

Originally posted by: dak@gnu.org

Replace b.height (b.start_) with b.start_height_ where applicable

http://codereview.appspot.com/10086043

Labels: -Patch-review Patch-new

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-08

Originally posted by: dak@gnu.org

Actually, I am not too enthused about the implicitly stored end height, either. That kind of stuff is a recipe for trouble. It's not going to fluctuate much since we are no longer extrapolating, but I don't like such code.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-08

Originally posted by: dak@gnu.org

Patchy the autobot says: passes tests.

Labels: -Patch-new Patch-review

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-12

Originally posted by: pkx1...@gmail.com

Patch on countdown for June 15 - 06:00 GMT

Labels: -Patch-review Patch-countdown

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-15

Originally posted by: pkx1...@gmail.com

Patch counted down please push.

Labels: -Patch-countdown Patch-push

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-15

Originally posted by: dak@gnu.org

Pushed to staging as
commit [r04ce84386dc022316c347ee0c5049c852eea3421]
Author: David Kastrup <dak@gnu.org>
Date:   Thu Jun 6 17:37:44 2013 +0200

    Issue3383: old-straight-flag + smaller Stem.thickness gives no output and huge over

    This replaces the numerically imprudent y_intercept_ in buildings with
    start_height_ and adapts calculations.

Labels: -Patch-push Fixed_2_17_21
Status: Fixed

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-06-30

Originally posted by: Elu...@gmail.com

(No comment was entered for this change.)

Status: Verified

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2013-07-11

Originally posted by: PhilEHol...@googlemail.com

This commit was reverted with Issue 3432.

Labels: -Fixed_2_17_21
Owner: ---
Status: Accepted

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

old-straight-flag + smaller Stem.thickness gives no output and huge over

Issue Tracker for LilyPond

Searches

Help

#3383 old-straight-flag + smaller Stem.thickness gives no output and huge over

Discussion