stem.cc - issue 5303 - misplaced notehead

stem.cc - issue 5303 - misplaced notehead

In loop calculating the attachments
of chord node heads to stem,
calculate last used position by
_rounding_ to int rather than by
truncating to int.

This fixes issue 5303 (misplaced note head bug).

Also add regression test.

[lilypond-pkx, 2019-07-04 20:03:01]

Passes make, make check and a full make doc.

[Malte Meyn, 2019-07-06 08:06:43]

Maybe the snipped should be renamed also.

https://codereview.appspot.com/570830043/diff/578780043/input/regression/misp...
File input/regression/misplaced-note-head-bug.ly (right):

https://codereview.appspot.com/570830043/diff/578780043/input/regression/misp...
input/regression/misplaced-note-head-bug.ly:1: \version "2.19.16"
IIUC this should be the first version a feature is introduced or a bug is fixed,
i. e. 2.21.0

https://codereview.appspot.com/570830043/diff/578780043/input/regression/misp...
input/regression/misplaced-note-head-bug.ly:5: texidoc = "Misplaced note head
bug (issue 5303) should be fixed."
Maybe that comment can be more descriptive so that you don’t have to search for
that issue to understand it? Something like “Note heads are placed on the
correct side of the stem; layout-set-staff-size doesn’t change that placement.”

[Lukas-Fabian Moser, 2019-07-06 09:20:32]

On 2019/07/06 08:06:43, Malte Meyn wrote:

Thanks for the review!

> Maybe the snipped should be renamed also.

Done. 

>
https://codereview.appspot.com/570830043/diff/578780043/input/regression/misp...
> File input/regression/misplaced-note-head-bug.ly (right):
> 
>
https://codereview.appspot.com/570830043/diff/578780043/input/regression/misp...
> input/regression/misplaced-note-head-bug.ly:1: \version "2.19.16"
> IIUC this should be the first version a feature is introduced or a bug is
fixed,
> i. e. 2.21.0

Done.
 
>
https://codereview.appspot.com/570830043/diff/578780043/input/regression/misp...
> input/regression/misplaced-note-head-bug.ly:5: texidoc = "Misplaced note head
> bug (issue 5303) should be fixed."
> Maybe that comment can be more descriptive so that you don’t have to search
for
> that issue to understand it? Something like “Note heads are placed on the
> correct side of the stem; layout-set-staff-size doesn’t change that
placement.”

Done (using a slightly different wording).

[lilypond-pkx, 2019-07-07 16:31:47]

With changes from Lukas based on dev's comments

[Malte Meyn, 2019-07-07 16:37:26]

On 2019/07/07 16:31:47, lilypond-pkx wrote:
> With changes from Lukas based on dev's comments

The last patch reverts the rounding in stem.cc. stem.cc should be taken from the
first patch set.

[Lukas-Fabian Moser, 2019-07-07 17:32:35]

> The last patch reverts the rounding in stem.cc. stem.cc should be taken from
the
> first patch set.

No, this was intentional: I agreed with David K that keeping the calculations in
floats would be the most convincing solution. (It's not a revert: The original
state was an (int) cast, which I first replaced by rounding+int-cast and now
(finally?) by keeping the original float value.

[Malte Meyn, 2019-07-07 17:36:46]

On 2019/07/07 17:32:35, Lukas-Fabian Moser wrote:
> > The last patch reverts the rounding in stem.cc. stem.cc should be taken from
> the
> > first patch set.
> 
> No, this was intentional: I agreed with David K that keeping the calculations
in
> floats would be the most convincing solution. (It's not a revert: The original
> state was an (int) cast, which I first replaced by rounding+int-cast and now
> (finally?) by keeping the original float value.

Sorry, I hadn’t noticed that the cast was gone.

[Carl, 2019-07-07 19:43:39]

Perhaps this patch should also revert 87eb2f9fe1be3a532675fe4b7322bbba5a60ba5c

since that patch was a workaround, rather than a real fix, as demonstrated
during this troubleshooting thread.

Carl

[Malte Meyn, 2019-07-08 06:48:28]

On 2019/07/07 19:43:39, Carl wrote:
> Perhaps this patch should also revert 87eb2f9fe1be3a532675fe4b7322bbba5a60ba5c

See also:

https://sourceforge.net/p/testlilyissues/issues/5530/ (a bug introduced by that
same commit 87eb2f9f)

https://sourceforge.net/p/testlilyissues/issues/5522/ (another bug that might be
a duplicate of issue 5303; in that case one should test, if this fix for 5303
also fixes 5522)

[lilypond-pkx, 2019-07-09 17:46:51]

Lukas-Fabian, see the comments about reverting a previous commit within this
patch. I didn't see any reply so am leaving this patch on 'Review' until the
next countdown cycle on the 11th

[Lukas-Fabian Moser, 2019-07-11 15:18:02]

Thanks, Malte, for the pointers!
 
> https://sourceforge.net/p/testlilyissues/issues/5522/ (another bug that might
be
> a duplicate of issue 5303; in that case one should test, if this fix for 5303
> also fixes 5522)

I can confirm that this patch set also fixes 5522 (and hence that 5522 almost
certainly is a duplicate of 5303). Thanks for spotting this!

As for Carl's suggestion, I'm trying now to understand the relationship between
87eb2f9fe1be3a532675fe4b7322bbba5a60ba5c and the bugs/patches under
consideration.

Lukas

[c_sorensen, 2019-07-11 17:40:23]

On 7/11/19, 9:18 AM, "lilypond-devel on behalf of
lukasfabianmoser@googlemail.com"
<lilypond-devel-bounces+c_sorensen=byu.edu@gnu.org on behalf of
lukasfabianmoser@googlemail.com> wrote:

    
    As for Carl's suggestion, I'm trying now to understand the relationship
    between 87eb2f9fe1be3a532675fe4b7322bbba5a60ba5c and the bugs/patches
    under consideration.

I withdraw my suggestion to revert this patch.  It should not be reverted. 

Thanks,

Carl