Adding Tremolo functions for issue #87

[craigvear]

Adds tremolo funcionality as a class for individual Chrodrest or between two Chordrests.

[craigvear]

Andrew - this is first draft. I've put the PR in for a reality check. If I get the master design of this right, it would be easy to follow up with other ornamentation and note articulation. My main question is sending Tremolo Chordrests rather than PositionedObjecs. There is probably a nice way round this, but would need a hint.

[craigvear]

Used a class method, but realise it’s probably not what you had in mind.

[ajyoon]

I haven't looked, but a classmethod is what i was thinking for the convenience make-a-trem-for-a-chordrest constructor

-------- Original Message -------- On Jan 23, 2023, 3:10 PM, Craig Vear wrote:

Used a class method, but realise it’s probably not what you had in mind.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because your review was requested.Message ID: @.***>

[craigvear]

Ok. Not sure it’s right, it’s feels a bit of a hack to call the main class from this constructor. I still have to finish of the testing and properties, but thought. I’d check in with this first.

[ajyoon]

just took a peek - the broad 'strokes' look good to me!

I'm seeing now that SMuFL provides trem glyphs, and they recommend using them for single-note tremolos. In that case I think it makes more sense to, at least for now, explicitly keep multi-note trems out of scope for this class, and if users really need multi-note trems they can still roll their own using Beam. Maybe in this case instead of taking a strokes: int, it could take a indication: Union[int, str] and say if given an int it will use plain trem glyphs, and if a string it will use it as a SMuFL glyphname. This way we actually could support penderecki and other trems out of the box quite easily.

[craigvear]

Thanks Andrew, and a genius suggestion. And agreed about double note support. In the example script for tremolo I achieved it by simply moving the start pos along and down a bit, hey presto, it worked. Once I’ve completed the tweak mentioned above, I’ll write the test script and sort doc strings out.

[craigvear]

Nearly all done. There is an error in the final test that I cannot fathom assert tremolo.tremolo_position == Point( self.staff.unit(0.067), self.staff.unit(2.996) ) once we get to the bottom of this, its ready for PR

[craigvear]

Thank you Andrew. Your guidance is always warmly appreciated. And your code still amazes me. What a great environment to add to. As always its clean, clear and efficient. As it should be :)

[craigvear]

So the test all work on my machine (MacOSX) but seem to be failing on the ubuntu test env in GH. Should I implement a different type of test in the code, to avoid this OS conflict, but that still retains the accuracy needed?

Although this is perplexing if I run the final test to 3 decimal places (because they look like the same number to me :) ).

E AssertionError: StaffUnit(0.067) and StaffUnit(0.067) not equal within 3 Unit decimal places. E Both as StaffUnit: StaffUnit(0.067) vs StaffUnit(0.067) E Both as StaffUnit: StaffUnit(0.067) vs StaffUnit(0.067)

[ajyoon]

It's fine to reduce the assert_almost_equal precision to 2. The tolerance is relative to base units, not staff units, so the printout it kind of confusing

[ajyoon]

0.01 base units is 0.004 millimeters, so that's plenty precise for most purposes!

[craigvear]

That is what I thought too. The error report from the ubuntu test is rather odd, almost like its is flipping x and y positions round with the test e.g.

def test_position_of_tremolo_attachment_point(self):
    cr = Chordrest.tremolo_attachment_point(self.cr2)

  assert_almost_equal(cr.x, self.staff.unit(0.067), 2)

tests/test_western/test_tremolo.py:49:

---

tests/helpers.py:27: in assert_almost_equal _assert_units_almost_equal(left, right, places, epsilon)

---

left = StaffUnit(0.291), right = StaffUnit(0.067), places = 2, epsilon = None

def _assert_units_almost_equal(left, right, places, epsilon):
    if epsilon is not None:
        eq = abs(left.base_value - right.base_value) < epsilon
    else:
        eq = round(left.base_value - right.base_value, places) == 0
    if not eq:
        left_type = type(left)
        right_type = type(right)
        raise AssertionError(
            "{} and {} not equal within {} Unit decimal places.\n"
            "Both as {}: {} vs {}\n"
            "Both as {}: {} vs {}".format(
                left,
                right,
                places,
                left_type.__name__,
                left,
                left_type(right),
                right_type.__name__,
                right_type(left),

              right,

) ) E AssertionError: StaffUnit(0.291) and StaffUnit(0.067) not equal within 2 Unit decimal places. E Both as StaffUnit: StaffUnit(0.291) vs StaffUnit(0.067) E Both as StaffUnit: StaffUnit(0.291) vs StaffUnit(0.067)

[ajyoon]

i think i know the issue, working on a fix now

[ajyoon]

Ok, I think I found a good solution. While I was in there I also made a number of other improvements. Please take a look over my changes here.

The test failures on Linux were caused by a known issue (temporarily forgotten..) that text bounding rects have slightly different shapes on different platforms. To work around this issue we filter tests depending on them so they only run on linux.

If the new changeset looks good to you let me know and I'll do a final check and merge