helge17 / tuxguitar

Improve TuxGuitar and provide builds
Other
424 stars 35 forks source link

TuxGuitar standard E Tuning is "E3 A3 D4 G4 B4 E5" - should be "E2 A2 D3 G3 B3 E4" #37

Closed helge17 closed 9 months ago

helge17 commented 1 year ago

I noticed that in the tuning settings all tunings are displayed one octave too high. While normally MIDI note 60 is equal to C4, TuxGuitar shows MIDI note 60 as C5, etc.

At least visually this can be fixed by subtracting 1 at the right place:

--- a/TuxGuitar/src/org/herac/tuxguitar/app/view/dialog/track/TGTrackTuningDialog.java
+++ b/TuxGuitar/src/org/herac/tuxguitar/app/view/dialog/track/TGTrackTuningDialog.java
@@ -694,9 +694,10 @@ public class TGTrackTuningDialog {
            sb.append(NOTE_NAMES[value % NOTE_NAMES.length]);

            if( octave ) {
-               sb.append(value / MAX_NOTES);
+               sb.append(value / MAX_NOTES - 1);
            }
        }
+
        return sb.toString();
    }

The MusicXML export has a similar problem as reported in https://sourceforge.net/p/tuxguitar/bugs/136/

Again, you can lower the octave by one in the following lines:

--- a/TuxGuitar-musicxml/src/org/herac/tuxguitar/io/musicxml/MusicXMLWriter.java
+++ b/TuxGuitar-musicxml/src/org/herac/tuxguitar/io/musicxml/MusicXMLWriter.java
@@ -194,7 +194,7 @@ public class MusicXMLWriter {
            Node stringNode = this.addNode(staffDetailsNode, "staff-tuning");
            this.addAttribute(stringNode, "line", Integer.toString( (track.stringCount() - string.getNumber()) + 1 ) );
            this.addNode(stringNode, "tuning-step", NOTE_NAMES[ NOTE_SHARPS[ (string.getValue() % 12) ] ] );
-           this.addNode(stringNode, "tuning-octave", Integer.toString(string.getValue() / 12) );
+           this.addNode(stringNode, "tuning-octave", Integer.toString(string.getValue() / 12 - 1) );
        }
    }

@@ -269,7 +269,7 @@ public class MusicXMLWriter {

                    Node pitchNode = this.addNode(noteNode,"pitch");
                    this.addNode(pitchNode,"step",NOTE_NAMES[ (ks <= 7 ? NOTE_SHARPS[value % 12] : NOTE_FLATS[value % 12] )]);
-                   this.addNode(pitchNode,"octave",Integer.toString(value / 12));
+                   this.addNode(pitchNode,"octave",Integer.toString(value / 12 - 1));
                    if(NOTE_ALTERATIONS[ value % 12 ]){
                        this.addNode(pitchNode,"alter", ( ks <= 7 ? "1" : "-1" ) );
                    }

There are a few other places in the code that do something like "value / 12" (Lilypond Export, Matrix Editor,...), probably you have to subtract 1 there too to get the right octave.

But maybe these are just workarounds and a real correction needs to be made elsewhere? Does anyone have a suggestion?

Matt-Osborn commented 1 year ago

From the code I looked at, it seems the system for handling note information is a bit of a mess; everything is done sort of haphazardly on the fly as needed.

Ideally there would be some sort of fundamental mapping between note names and note values that is consistent across the project -- i.e. there should be a table somewhere that says A4 == 440hz (in 12tet)== 2nd fret on Gstring in standard tuning == MIDI key x and so on. In addition there should be standard methods for manipulating, and translating between, all of those different note name and value data types.

This is not a trivial issue - really its fundamental to the design of something like Tuxguitar. I will go deeper into the code and try to write a little summary of how this is designed/implemented currently, with suggestions for improvement.

sugizo commented 11 months ago

already check on /Applications/tuxguitar.app/Contents/MacOS/share/tunings/tunings.xml

    <group name="Guitar">
        <group name="6-String">
            <tuning name="E Tuning" notes="64,59,55,50,45,40" />

MIDI note 64 is equal to E4 this one correct,

but in tuxguitar > track > properties > tuning > settings it show E5 this is incorrect, should show E4

best regards

guiv42 commented 10 months ago

Ideally there would be some sort of fundamental mapping between note names and note values that is consistent across the project

I fully agree. Note: there is already a centralized place to handle notes names: TGMusicKeyUtils.java. However, I do not understand at all how it is managed. It seems several tables of notes names are stored (for chord, scale, tuning, fretboard, matrix), but it looks they all are identical (?). I don't understand either why these tables are linked to language manager (and why TGMusicKeyNames reacts to language change events) since the final names are hard-coded in this file.

there should be a table somewhere that says A4 == 440hz (in 12tet)== 2nd fret on Gstring in standard tuning == MIDI key x and so on. In addition there should be standard methods for manipulating, and translating between, all of those different note name and value data types.

I still agree. Just take care, A4 = 440Hz = 5th fret on treble E string in standard tuning = midi note 69 Especially, a link between notes names and midi values (or should I say height?) is missing.

I will go deeper into the code and try to write a little summary of how this is designed/implemented currently, with suggestions for improvement.

@Matt-Osborn : any progress on this activity? I can suggest a simple approach: start by adding this correspondence between notes names and midi values, and use it in TGTrackTuningDialog and MusicXMLWriter classes (instead of re-computing association locally). Then progressively, other places where we find conversion between names and values could be redirected there.

Warning, I just noticed another bug in MusicXML export: tuning only exports notes names without alteration. In other words, tuning part of file will be identical if one string is tuned A or A#. So, as soon as a string is tuned in a non-natural note, all notes on this string have inconsistent values (note vs. fret)

guiv42 commented 9 months ago

I proposed a first correction with PR #164. Probably not as ambitious as proposed by @Matt-Osborn above, but I think it can be considered a first step in this direction. At least the issues mentioned in initial description should be fixed.

guiv42 commented 9 months ago

@helge17 : can we close this issue now?

helge17 commented 9 months ago

Thanks!