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

Rewrite of episema positioning #401

Merged
merged 11 commits into from
May 13, 2015
Merged

Conversation

henryso
Copy link
Contributor

@henryso henryso commented May 12, 2015

This pull request includes:

Ready to be pushed into https://github.com/gregorio-project/gregorio-test:

@eroux
Copy link
Contributor

eroux commented May 12, 2015

What about merging that in a fix-40 branch of the main repository on which we'll work together? Also, what about dividing the bits of h_episemus field of gregorio_note so that more things are possible?

@eroux
Copy link
Contributor

eroux commented May 12, 2015

Adding these in gregorio_note:

  • bool h_has_bottom
  • bool h_force_top
  • bool h_prevent_bridge
  • unsigned char h_short_type (H_NORMAL, H_S_LEFT, H_S_CENTER, H_S_RIGHT)

would make things a bit easier but would keep the current structure... But maybe it would be clearer with a pointer to a bigger h_episemus structure, with something like

  • bool h_force_top
  • bool h_prevent_bridges
  • unsigned char h_type (H_NORMAL, H_S_LEFT, H_S_CENTER, H_S_RIGHT)
  • unsigned char h_top_note
  • bool h_has_bottom
  • bool h_bottom_prevent_bridges
  • unsigned char h_bottom_type (H_NORMAL, H_S_LEFT, H_S_CENTER, H_S_RIGHT)
  • unsigned char h_bottom_lowest_note

?

@henryso
Copy link
Contributor Author

henryso commented May 12, 2015

It sounds like you really want to support both top and bottom episemae at the same time. Can you explain the use case?

@eroux
Copy link
Contributor

eroux commented May 12, 2015

The case you described in #40, (g_0_h_g_0_) actually happens (in this precise form) in some books. I remember seeing this a few years ago, I don't believe I can find any reference though... maybe @jakubjelinek would have a reference?

@eroux
Copy link
Contributor

eroux commented May 12, 2015

I admit I'm doubting a bit right now... I cannot find any case in gregobase...

@henryso
Copy link
Contributor Author

henryso commented May 12, 2015

No need for a reference; I'll do it. I just need to rethink the algorithm which figures out the episemae. It probably needs to figure out the top and bottom simultaneously.

@henryso
Copy link
Contributor Author

henryso commented May 12, 2015

The problem with (g_0_h_g_0_) is actually disambiguating this from an isolated episemus (which pushes itself onto the note on the left).

@eroux
Copy link
Contributor

eroux commented May 12, 2015

You know, it's very possible that I'm wrong: I'm a Gregorio developer, not a gregorianist... I admit I don't understand what you mean in your last comment though, can you explain a bit more?

@henryso
Copy link
Contributor Author

henryso commented May 12, 2015

I mean in the lexical analysis, gh__ is equivalent to g_h_.

As I've coded it (which is admittedly a little different than the present version), gh_0_1 is equivalent to g_1h_0. The second episema specification applied to the second-to-last note.

We need to decide on a way to allow the user to unambiguously specify both a bottom and a top specification for a note.

There are several options. The first which come to mind are:

  • Only allow a bare _ to apply backwards
  • (or) switching from auto to forced above/below switches logically to the next specification
  • (or) switching from auto to forced above/below or using a previously used above/below switches logically to the next specification

I think it can become very confusing to look at for a user. I am actually in favor of the first option because of this.

@eroux
Copy link
Contributor

eroux commented May 12, 2015

I prefer the first option too, __ is a shortcut for a common case (in fact, it was meant to be compatible with Gregoria at the beginning, and now it's kept for backward compatibility, but that was probably an error to add it in the first place). When things are getting more complex, I don't think users expect shortcuts to still work...

@henryso
Copy link
Contributor Author

henryso commented May 12, 2015

Do you still want me to create a branch or should I work on this a bit more before I do that?

@henryso
Copy link
Contributor Author

henryso commented May 12, 2015

Also, you (and I) would probably be surprised what users expect :)

@eroux
Copy link
Contributor

eroux commented May 12, 2015

For sure, users expectations are always surprising! :) If you feel like it, you can continue this PR, I just thought you'd be a bit discouraged and would need some help

@jakubjelinek
Copy link
Contributor

See the

TRi(d)bus(c) mi(d!ewf)rá(f_0gF'___ED/eed)cu(cd)lis(d.)

is a real-world example from Antiphonale Monasticum 1933, p. 296.
I've mentioned elsewhere.
Or

or(g_0hG'E____)

in the same book, page 291 (Antiphona Venit lumen tuum). Here the upper bar is across all 4 notes, and lower horizontal episema below the podatus, and ictus below the first virga inclinata.

@henryso henryso mentioned this pull request May 12, 2015
@henryso
Copy link
Contributor Author

henryso commented May 13, 2015

I got @jakubjelinek's cases working. I think now I can finally start the changes for #40.

@eroux
Copy link
Contributor

eroux commented May 13, 2015

Seems good to me! Thanks a lot!

@henryso
Copy link
Contributor Author

henryso commented May 13, 2015

The implementation of #40 is done. This is more-or-less ready to merge. Tests fail in an acceptable manner given these changes. I will commit the broken expectations when/if this is merged into develop. Let me know if I should change anything.

@henryso
Copy link
Contributor Author

henryso commented May 13, 2015

Note: I left the feature in TeX that can use two different heights if bridges are disabled, but the Gregorio code currently doesn't compute an additional height for when bridges are disabled with \RemoveHEpisemusBridges. If we want to keep this feature, please outline the rules underlying non-bridged heights, in a separate issue, and I will implement in a different pull request. I also still think it's worthwhile to symbolize the offset cases used for signs, to further decouple the C code from the TeX.

@eroux
Copy link
Contributor

eroux commented May 13, 2015

What do you think of merging, updating gregorio-test, and then I add a few tests? (also, I concur with the idea to merge gregorio-test into the main repo, but that can be for later)

@eroux
Copy link
Contributor

eroux commented May 13, 2015

Also, why not symbolize the offset cases, yes. And thank you very much for this work, I think (at least I hope) this finalizes the horizontal episemus code in Gregorio...

@henryso
Copy link
Contributor Author

henryso commented May 13, 2015

Yes, if you are OK with the last change, I will merge this.

@eroux
Copy link
Contributor

eroux commented May 13, 2015

Seems fine yes

eroux added a commit that referenced this pull request May 13, 2015
Rewrite of episema positioning
@eroux eroux merged commit c6bc822 into gregorio-project:develop May 13, 2015
@henryso henryso deleted the fix-40 branch May 13, 2015 14:22
@eroux
Copy link
Contributor

eroux commented May 13, 2015

I don't know where it's best to report that, but hg__gof__(hg__gof__) fails to bridge the two parts... I think that when there is a space between the neumes, the episemus should bridge only if the difference in the top_notes is <3... But I'm not sure... Should I open separate issues for this?

@eroux
Copy link
Contributor

eroux commented May 13, 2015

oops, my tests were wrong, sorry!

@eroux
Copy link
Contributor

eroux commented May 13, 2015

I admit I can't make sense of my tests... When I do something like

abc(f_13hf_15) gná(ghg___) gná(ghg___//e!f'g)

I get no episemus for the first two (even in the gtex file) and a misplaced episemus for the last one... Is there something wrong in my tests?

@eroux
Copy link
Contributor

eroux commented May 13, 2015

@henryso can you confirm/infirm the bugs?

@henryso
Copy link
Contributor Author

henryso commented May 13, 2015

I get...

image

What are you expecting?

@eroux
Copy link
Contributor

eroux commented May 13, 2015

You're getting a quite normal result... I can't understand where the bugs come from... are you sure you pushed everything? I get the correct results in dump, but in gregoriotex, I don't have any hepisemus for the first two syllables...

Anyway, there is still a "bug" ("a bad explanation from my side" would be more accurate I believe) in your result: the small episemus should be just above their corresponding notes, that's what makes the feature interesting... The feature was made for this figure (f_13hf_15) and also this one: (gvFE_), to have episemus closer to the note... Do you see what I mean? Sorry for my bad explanation...

Thank you!

@henryso
Copy link
Contributor Author

henryso commented May 13, 2015

I think everything is pushed... unless something was ignored. Did you rebuild your fonts? I will try a clean clone and see if there's a problem.

I understand what you are saying about that bug... There are a number of other tweaks I'm noticing as I try out different scores. Also the ambitus > 3 breaking a bridge between glyphs. I will work on these in the next few days. For now, keep throwing these examples at me so I can get them all working properly.

@henryso
Copy link
Contributor Author

henryso commented May 13, 2015

I compared a fresh clone with my normal build directory, and the source files are identical.

@eroux
Copy link
Contributor

eroux commented May 13, 2015

There was indeed something wrong with the fonts, but the produced .gtex is still wrong, there are no episemus for the second syllable, I really don't know where it can come from... I'll make a fresh clone, and debug on my side.

@henryso
Copy link
Contributor Author

henryso commented May 13, 2015

By the way, @jperon's suggested spacing change has broken (expectedly) the test expectations. I will update them later today.

@eroux
Copy link
Contributor

eroux commented May 13, 2015

There is indeed a problem on my side, as a fresh clone works fine... I've completely cleaned the src/ directory, rebuilt dozens of times... Still searching, but it's not in your code.

@eroux
Copy link
Contributor

eroux commented May 13, 2015

Well, it works again, no idea why... Anyway, the use cases I meant were: (gvFE_5) and (hvGF_5)

@henryso
Copy link
Contributor Author

henryso commented May 14, 2015

@eroux I found some issue if I compile with -O2. I'm not sure how to figure out what causes it to break, but I'll try to fix it.

@henryso
Copy link
Contributor Author

henryso commented May 14, 2015

Very weird. It is a bug in the code, a missing return statement. However, when compiling with -g alone, this somehow works. If you use any optimization level beyond that, you have the problems you describe above.

@jakubjelinek
Copy link
Contributor

Didn't the compiler warn about the missing return? Generally, I'm seeing tons of less severe warnings on gregorio code (e.g. -Wparentheses -Wswitch -Wunused-but-set-variable -Wunused-function -Wunused-variable warnings), would be nice to fix them all eventually (can help with that later).

@henryso
Copy link
Contributor Author

henryso commented May 14, 2015

No, it didn't. I think warnings are off. I had assumed they would be on because we have -Wall in configure.ac, but I don't see it in the gcc commands emitted by make.

@eroux
Copy link
Contributor

eroux commented May 14, 2015

the -Wall is just for automake, I certainly forgot to add it (along with -Wextra) when I added to fortify flags... I'll fix that

eroux added a commit that referenced this pull request May 14, 2015
Random set of fixes for problems stemming from #401.
@henryso henryso mentioned this pull request Sep 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants