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.

Sign up for GitHub

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

Jump to bottom

many micro-optimizations #375

Merged
merged 34 commits into from
May 9, 2015
Merged

many micro-optimizations #375

merged 34 commits into from
May 9, 2015

Conversation

eroux
Copy link
Contributor

@eroux eroux commented May 6, 2015

  • draw porrectus bars with ambitus of one
  • redraw normal porrectus bars (narrower)
  • improve design of porrectus bars with ambitus of 5
  • improve design of porrectus auctus with second ambitus of one

@eroux
many macro-optimizations
f65c70e 
 * draw porrectus bars with ambitus of one
 * redraw normal porrectus bars (narrower)
 * improve design of porrectus bars with ambitus of 5
 * improve design of porrectus auctus with second ambitus of one
@eroux
Copy link
Contributor Author

eroux commented May 6, 2015

Please do not merge now, I'm going to do a few more things

@eroux eroux added this to the 4.0 milestone May 6, 2015
eroux added 2 commits May 6, 2015 20:27
@eroux
fixing horizontal episemus for fonts with porrectusam1
755e084 
also fixing an old bug with episemus too large on some torculus resupinus
@eroux
use newly added types and fix g_f_i
fcfeeff
@eroux
Copy link
Contributor Author

eroux commented May 7, 2015

This is ready to be merged, I'll make more changes but this is a complete working first step

eroux referenced this pull request in henryso/gregorio May 7, 2015
@henryso
Positioned horizontal episemas for the torculus resupinus flexus.
300a0f5
eroux referenced this pull request in henryso/gregorio May 7, 2015
@henryso
Known working checkpoint before sign changes in C.
33b38dc
@eroux eroux changed the title many macro-optimizations many micro-optimizations May 7, 2015
@henryso
Completed unrevert of "Implementation of fused torculus resupinus fle…
fb9f946 
…xus."

Worked out and created missing cases.
@henryso
Merge pull request #381 from henryso/microoptfonts
d938174 
Unrevert "Implementation of fused torculus resupinus flexus."
@eroux
Copy link
Contributor Author

eroux commented May 7, 2015

Thanks, I'll try to finish that tonight

@eroux
Copy link
Contributor Author

eroux commented May 7, 2015

Well, there are quite a lot of bugs, mainly because you seem to have removed the horizontal episemus for the two first notes of a porrectus (you even removed their numbers completely if I read correctly)... An extreme example showing the bug is g_b_c (it also has a misplaced bar)... I think it will be easier to fix for you than for me, as I'm not 100% sure I understand why you removed the case and how you wanted to replace them... What do you think?

@eroux
Copy link
Contributor Author

eroux commented May 7, 2015

Oh, could it be that you were relying on the episemus on the first and last note of a porrectus to be close enough to merge? That might work for some cases, but not all... I'll revert that to previous state

@henryso
Copy link
Contributor

henryso commented May 7, 2015

It's most likely typos on my part. I'll take a look.

@eroux
Copy link
Contributor Author

eroux commented May 7, 2015

The main problem is your handling of the H_EPISEMUS_FIRST_TWO in the C code, you treat it as if it was just an episemus on the first note, which it isn't... On small episemus with ambitus of 1 for instance, it's not noticeable if you have an episemus on the last note, but my previous example triggers problems...

@henryso
Copy link
Contributor

henryso commented May 7, 2015

Yes, it was a typo... Easy fix... Let me check all the cases.

@eroux
Copy link
Contributor Author

eroux commented May 7, 2015

Ok, thanx!

@eroux
Copy link
Contributor Author

eroux commented May 7, 2015

Ok yes, seems consistant... We have to revert 675fba5 for this. No you're right, no need to change anything! It will just look a little odd with deminutus (g_f_g~), but that certainly doesn't really exist...

@henryso
Copy link
Contributor

henryso commented May 8, 2015

@eroux Do you foresee many changes remaining for you in the horizontal episemus code for what you're working on here?

@eroux
Copy link
Contributor Author

eroux commented May 8, 2015

No, I'll just change the numbers corresponding to new cases in gregoriotex_find_sign_number, that's it, you can change everything else

@eroux
Copy link
Contributor Author

eroux commented May 8, 2015

@henryso I'm quite happy with the current state, I added a test in gregorio-test, and now gregoria4o looks good, so I think it's ready to merge... please review

@henryso
Copy link
Contributor

henryso commented May 8, 2015

I will review in detail tonight.

@henryso
Copy link
Contributor

henryso commented May 9, 2015

I'll admit that reviewing that was an eye-crossing experience, but I assuming the measurements are right, I didn't see anything glaringly out of place. Merging.

henryso added a commit that referenced this pull request May 9, 2015
@henryso
Merge pull request #375 from gregorio-project/microoptfonts
3f1e100 
many micro-optimizations
@henryso henryso merged commit 3f1e100 into develop May 9, 2015
@henryso henryso deleted the microoptfonts branch May 9, 2015 00:03
@henryso henryso mentioned this pull request May 9, 2015
Merged
@eroux
Copy link
Contributor Author

eroux commented May 9, 2015

Well, if you've tested with greciliae, I don't believe this branch introduces much difference (if at all), but with gregoria4o (which I consider finished now), it's noticeable

@henryso
Copy link
Contributor

henryso commented May 9, 2015

@eroux Are you referring to #391? Did you check that I used the correct glyphs in Gregoria? If you're referring to #375, then yes, I tested with greciliae, but I had to add the changes in #391 for some things to work.

@eroux
Copy link
Contributor Author

eroux commented May 9, 2015

I was just referring to your comment "I'll admit that reviewing that was an eye-crossing experience"

@henryso
Copy link
Contributor

henryso commented May 9, 2015

I see. Yes, it's fine more-or-less with greciliae.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement fonts
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@eroux @henryso