On 2020/03/17 21:27:52, hanwenn wrote: > https://codereview.appspot.com/553740043/diff/547780043/input/regression/tie-single-manual.ly > File input/regression/tie-single-manual.ly (right): > > https://codereview.appspot.com/553740043/diff/547780043/input/regression/tie-single-manual.ly#newcode19 > input/regression/tie-single-manual.ly:19: \override Tie.staff-position = #-7.4 > On 2020/03/17 21:06:42, hahnjo wrote: > > I'm not really happy about tweaking this test like this. It relies on rounding > > halfway always up (my_round) instead of away from zero (std::round). I think > > this is a coincidence, but if we want to retain this I need to go through all > > replacements of my_round -> std::round and check if the argument could be > > negative... > > couldn't you inline my_round into the call site that needs this and add a > comment? The change otherwise risks breaking user tweaks, which we should avoid > if we can.
Yes, that was my thought. The problem is that I don't know in which call sites could exhibit negative values, possibly all of them? On 2020/03/17 21:28:35, lemzwerg wrote: > https://codereview.appspot.com/553740043/diff/547780043/input/regression/tie-single-manual.ly > File input/regression/tie-single-manual.ly (right): > > https://codereview.appspot.com/553740043/diff/547780043/input/regression/tie-single-manual.ly#newcode19 > input/regression/tie-single-manual.ly:19: \override Tie.staff-position = #-7.4 > > I'm not really happy about tweaking this test like this. > > It relies on rounding halfway always up (my_round) > > instead of away from zero (std::round). I think this is > > a coincidence, but if we want to retain this I need to > > go through all replacements of my_round -> std::round > > and check if the argument could be negative... > > Hmm. Are you talking about rounding to integers? If `staff-position` is a > floating point value, LilyPond takes it as an exact value, see > `has_manual_delta_y`. So there shouldn't be any rounding here... Yes. I hope I've marked the correct rounding that I found out to be responsible for the difference. https://codereview.appspot.com/553740043/diff/547780043/lily/tie-formatting-problem.cc File lily/tie-formatting-problem.cc (right): https://codereview.appspot.com/553740043/diff/547780043/lily/tie-formatting-problem.cc#newcode908 lily/tie-formatting-problem.cc:908: conf.position_ = (int) std::round (specifications_[i].manual_position_); IIRC this is the call site that influences the regression test. https://codereview.appspot.com/553740043/
