Add NonTraditional support to KeyData. by p-groarke · Pull Request #77 · webern/mx

p-groarke · 2020-03-28T18:29:11Z

Fixes #70

As far as I could tell, this should be enough to support non-traditional keys in api.

However, I couldn't for the life of me figure out your unit test framework :) Could you explain to me how to add some tests for this? ty

Of note

Using a vector of PitchData. Maybe you'd rather I add a new "Data" type which only contains step, alter and accidental? NonTraditionalData maybe?
Minor tweaks to cmake.
No tests yet! PR will stay Draft until I can add some good tests.

p-groarke · 2020-03-28T18:29:47Z


 # Mx Library
-add_library(${PROJECT_NAME} STATIC ${SRC_MX_API} ${SRC_MX_CORE} ${SRC_MX_IMPL} ${SRC_MX_PUGIXML} ${SRC_MX_UTILITY})
+add_library(${PROJECT_NAME} STATIC ${HEADERS_MX_API} ${SRC_MX_API} ${SRC_MX_CORE} ${SRC_MX_IMPL} ${SRC_MX_PUGIXML} ${SRC_MX_UTILITY})


So the headers show up in IDE. Makes it much easier to navigate for new devs.

This will probably conflict when #79 lands but just reapply this change.

Sounds good, I'll rebase once everything is settled for the cmake stuff.

p-groarke · 2020-03-28T18:30:05Z

    target_link_libraries(MxTest ${PROJECT_NAME})
    target_link_libraries(MxTest ${CMAKE_THREAD_LIBS_INIT})
    set_property(TARGET MxTest PROPERTY CXX_STANDARD 14)
+    set_property(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} PROPERTY VS_STARTUP_PROJECT MxTest)


Makes MxTest the default target in MSVC.

p-groarke · 2020-03-28T18:30:30Z

+            bool nonTraditionalKey;
+            // The non-tradional accidentals.
+            // Fill this with your custom accidentals and alters.
+            std::vector<PitchData> nonTraditionalPitchData;


Maybe create a new "Data" struct. "NonTraditionalData"?

I had started, or maybe completed this here https://github.com/webern/mx/master/develop...feature/custom-key-signatures

As a reference to how I was thinking about it. Or you could check it out, kick the tires. The main difference is...

Maybe create a new "Data" struct. "NonTraditionalData"?

Yes, I think that would be better than using PitchData.

Some xsd for reference:

<xs:complexType name="key"> <xs:annotation> <xs:documentation>The key type represents a key signature. Both traditional and non-traditional key signatures are supported. The optional number attribute refers to staff numbers. If absent, the key signature applies to all staves in the part. Key signatures appear at the start of each system unless the print-object attribute has been set to "no".</xs:documentation> </xs:annotation> <xs:sequence> <xs:choice> <xs:group ref="traditional-key"/> <xs:group ref="non-traditional-key" minOccurs="0" maxOccurs="unbounded"/> </xs:choice> <xs:element name="key-octave" type="key-octave" minOccurs="0" maxOccurs="unbounded"> <xs:annotation> <xs:documentation>The optional list of key-octave elements is used to specify in which octave each element of the key signature appears.</xs:documentation> </xs:annotation> </xs:element> </xs:sequence> <xs:attribute name="number" type="staff-number"/> <xs:attributeGroup ref="print-style"/> <xs:attributeGroup ref="print-object"/> </xs:complexType>

<xs:group name="non-traditional-key"> <xs:annotation> <xs:documentation>The non-traditional-key group represents a single alteration within a non-traditional key signature. A sequence of these groups makes up a non-traditional key signature</xs:documentation> </xs:annotation> <xs:sequence> <xs:element name="key-step" type="step"> <xs:annotation> <xs:documentation>Non-traditional key signatures can be represented using the Humdrum/Scot concept of a list of altered tones. The key-step element indicates the pitch step to be altered, represented using the same names as in the step element.</xs:documentation> </xs:annotation> </xs:element> <xs:element name="key-alter" type="semitones"> <xs:annotation> <xs:documentation>Non-traditional key signatures can be represented using the Humdrum/Scot concept of a list of altered tones. The key-alter element represents the alteration for a given pitch step, represented with semitones in the same manner as the alter element.</xs:documentation> </xs:annotation> </xs:element> <xs:element name="key-accidental" type="accidental-value" minOccurs="0"> <xs:annotation> <xs:documentation>Non-traditional key signatures can be represented using the Humdrum/Scot concept of a list of altered tones. The key-accidental element indicates the accidental to be displayed in the key signature, represented in the same manner as the accidental element. It is used for disambiguating microtonal accidentals.</xs:documentation> </xs:annotation> </xs:element> </xs:sequence> </xs:group>

p-groarke · 2020-03-28T18:30:40Z

+            g = 4,
+            a = 5,
+            b = 6,
+            count,


Fine, whatever. :)

p-groarke · 2020-03-28T18:31:11Z

        PropertiesWriter::PropertiesWriter( const core::PartwiseMeasurePtr& inPartwiseMeasure )
        : myProperties{ nullptr }
        , myPartwiseMeasure{ inPartwiseMeasure }
+        , myConverter{}


Added converter because its nice.

p-groarke · 2020-03-28T18:31:58Z

+                    core::NonTraditionalKeyPtr k = core::makeNonTraditionalKey();
+                    if (p.step == api::Step::count || p.step == api::Step::unspecified)
+                    {
+                        // Would you prefer throw? assert?


Not sure what is your standard in terms of "bad things reporting". Continue, throw, assert, format c:/ ?

I think just silently fall back to Step::c. If I remember right I only use exceptions/assertions for bugs in the library, not for bad input.

p-groarke · 2020-03-28T18:32:33Z

+                    }
+
+                    if (p.alter != 0) {
+                        k->setKeyAlter(std::make_shared<core::KeyAlter>(core::Semitones{ double(p.alter) }));


Drops support for float semitones :/ Maybe should use doubles in "NonTraditionalData"?

I think we need to adapt the int alter and double cents pattern wherever we convert to/from alter double. See #50
Probably what we should do is move the calculations that I did in #50 to the MxConverter thing and use it in both places.

And by using a custom data struct instead of pitchdata, I'll use a double so it doesn't conflict.

p-groarke · 2020-03-28T18:32:39Z



-        void PropertiesWriter::writeTime( const api::TimeSignatureData& value )
+		void PropertiesWriter::writeTime(const api::TimeSignatureData& value)


p-groarke · 2020-03-28T18:33:54Z

        measure->timeSignature.beatType = 4;
        measure->timeSignature.isImplicit = true;
        measure->staves.emplace_back( StaffData{} );
+        measure->keys.emplace_back( KeyData{} );


OK, I tried adding a test. But this doesn't do anything. I found 2 test files which test KeyData, this and ApiLy43eScoreData. I don't get what this tests.

Just add a new test for this api/KeyDataTest.cpp.

A lot of the really funky names you see in the tests are related to specific test files. ApiLy43eScoreData is probably related to a test file by that name from the Lilypond MusicXML test suite.

webern

Good job figuring out how things work!

webern · 2020-03-29T21:59:10Z

-                    const auto coreMode = traditionalKey.getMode()->getValue().getValue();
-                    if( coreMode == core::ModeEnum::major )
+
+                if (keyType == core::KeyChoice::Choice::traditionalKey) {


Suggested change

if (keyType == core::KeyChoice::Choice::traditionalKey) {

if( keyType == core::KeyChoice::Choice::traditionalKey )

{

Just a note on the existing style, for better or worse.

webern · 2020-03-29T22:02:01Z

-                        keyData.mode = api::KeyMode::unsupported;
+                        api::PitchData p;
+                        p.step = myConverter.convert(k->getKeyStep()->getValue());
+                        p.alter = int(k->getKeyAlter()->getValue().getValue());


Microtones will need to be handled in semitones (int) and cents (double).

webern · 2020-03-29T22:07:00Z

+                    core::NonTraditionalKeyPtr k = core::makeNonTraditionalKey();
+                    if (p.step == api::Step::count || p.step == api::Step::unspecified)
+                    {
+                        // Would you prefer throw? assert?


I think just silently fall back to Step::c. If I remember right I only use exceptions/assertions for bugs in the library, not for bad input.

webern · 2020-03-29T22:11:19Z

        measure->timeSignature.beatType = 4;
        measure->timeSignature.isImplicit = true;
        measure->staves.emplace_back( StaffData{} );
+        measure->keys.emplace_back( KeyData{} );


Just add a new test for this api/KeyDataTest.cpp.

A lot of the really funky names you see in the tests are related to specific test files. ApiLy43eScoreData is probably related to a test file by that name from the Lilypond MusicXML test suite.

webern · 2020-03-29T22:12:11Z

+                    }
+
+                    if (p.alter != 0) {
+                        k->setKeyAlter(std::make_shared<core::KeyAlter>(core::Semitones{ double(p.alter) }));


I think we need to adapt the int alter and double cents pattern wherever we convert to/from alter double. See #50
Probably what we should do is move the calculations that I did in #50 to the MxConverter thing and use it in both places.

webern · 2020-03-29T22:13:27Z

    target_link_libraries(MxTest ${PROJECT_NAME})
    target_link_libraries(MxTest ${CMAKE_THREAD_LIBS_INIT})
    set_property(TARGET MxTest PROPERTY CXX_STANDARD 14)
+    set_property(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} PROPERTY VS_STARTUP_PROJECT MxTest)


p-groarke · 2020-04-04T14:57:25Z

closed, fixed by #81

Add NonTraditional support to KeyData.

3361b05

p-groarke commented Mar 28, 2020

View reviewed changes

webern requested changes Mar 29, 2020

View reviewed changes

webern mentioned this pull request Apr 1, 2020

api: support non-traditional key signatures #81

Merged

p-groarke closed this Apr 4, 2020



		void PropertiesWriter::writeTime( const api::TimeSignatureData& value )
		void PropertiesWriter::writeTime(const api::TimeSignatureData& value)

	if (keyType == core::KeyChoice::Choice::traditionalKey) {
	if( keyType == core::KeyChoice::Choice::traditionalKey )
	{

Conversation

p-groarke commented Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webern Mar 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webern Mar 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-groarke Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-groarke Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webern Mar 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webern Mar 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-groarke commented Apr 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

p-groarke commented Mar 28, 2020 •

edited

Loading

webern Mar 29, 2020 •

edited

Loading

webern Mar 29, 2020 •

edited

Loading

p-groarke Mar 28, 2020 •

edited

Loading

p-groarke Mar 28, 2020 •

edited

Loading

webern Mar 29, 2020 •

edited

Loading

webern Mar 29, 2020 •

edited

Loading