Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding dotted notes, = lengths and tie support to l directives #1

Open
exodustx0 opened this issue Nov 20, 2020 · 4 comments
Open

Adding dotted notes, = lengths and tie support to l directives #1

exodustx0 opened this issue Nov 20, 2020 · 4 comments
Labels
c++-side Involves the AddMusicK program itself in some way. enhancement New feature or request partially-resolved Something that partially resolves the issue (or part of the pull request itself) was merged.

Comments

@exodustx0
Copy link

I was redirected here after hearing you were working on this, except didn't want to touch tie support? I would have added it long ago if I had any idea how to get AMK compiling, but I can do you a pull request to add this in, already got the code.

Oh, and side-note, plus a reason why AMK really does need replacing (working on it): you seem to have the AMK 1.0.8 before it was moderated, meaning you haven't imported the actual released 1.0.8 (it was further changed before it passed moderation).

@KungFuFurby
Copy link
Owner

You're right! A few files were further modified afterwards. I'll get that pushed in.

No-go on ties due to a conflict with the actual tie command, which is a valid N-SPC command. I have the = length part implemented on my end, and I haven't gotten to the dotted notes yet. I can compare and contrast on the pull request with my own copy if you want.

KungFuFurby added a commit that referenced this issue Dec 24, 2020
This commit references #1. Tie support for l directive is not implemented here
due to a conflict with the actual tie command used by the sound driver itself.
KungFuFurby added a commit that referenced this issue Jan 7, 2021
This primairly includes the dotted note and if it were inside a triplet.
This commit references #1.
@KungFuFurby
Copy link
Owner

Although I merged the changes, I have not closed the issue yet, pending a confirmation to let the tie portion remain cut.

KungFuFurby pushed a commit that referenced this issue Jan 20, 2021
@KungFuFurby KungFuFurby added c++-side Involves the AddMusicK program itself in some way. enhancement New feature or request labels Mar 23, 2021
@KungFuFurby
Copy link
Owner

I'm adding sound effects to the scope of this issue, because I both never implemented them and there is way more to do there in addition to what this will do.

KungFuFurby added a commit that referenced this issue Oct 3, 2021
Triplets were previously de-facto ignored by the default note length, and
factoring them in causes them to be calculated twice by mistake. Because
triplets are temporary modifiers due to being defined as a section, unlike
dotted notes, they don't need to permanently modify the default note length.

This commit closes #214 and mentions #1 due to reverting a change that was
breaking existing cases.
@KungFuFurby
Copy link
Owner

KungFuFurby commented Dec 3, 2021

This is officially problematic because of the way substitution works. Namely, substitution only works at the start of each character prior to processing anything else and does not account for any characters in between. This results in a quirk where a rest followed by a define that is the same first character as a rest gets lost in the mix because it is combined. This quirk can work in reverse by failing to detect a character as the first character of a macro substitution. Unfortunately, adding the . character to parsing counts, and it actually has been done in an example case and thus must be kept to avoid song breakage.

The = character is also added as a parsing character, but is an illegal character to use in a define because of a conflict. Thus, it's safe, but only on a technicality.

This kind of problem is going to result in two things:

  • I need to check any parser modifications between AMK versions prior to my fork. The last version that Kipernal developed to my knowledge was 1.0.2, and I couldn't find any significant changes on the C++-side between 1.0.5 and 1.0.8. I don't have a copy of 1.0, and it is not extant on SMWCentral.
  • I either have to bump the parser version up to 4, or add a doReplacement call prior to checking the new characters in order to avoid this particular problem. There is no filtering of characters when doing macro replacements other than the quotation characters and the equals character.

@KungFuFurby KungFuFurby mentioned this issue Jan 25, 2022
17 tasks
@KungFuFurby KungFuFurby added the partially-resolved Something that partially resolves the issue (or part of the pull request itself) was merged. label Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++-side Involves the AddMusicK program itself in some way. enhancement New feature or request partially-resolved Something that partially resolves the issue (or part of the pull request itself) was merged.
Projects
No open projects
Development

No branches or pull requests

2 participants