-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Realize the basic functions of Jianpu #19421
Conversation
Add a font file (to prevent unknown errors, fonts are added based on Leland text characters), which is only the basic function of Jianpu. There are still some layout issues to be addressed, but I am a bit tired. I will handle them later. If anyone is interested or has time, please help improve it. One person's power is too small.(All changed codes can be found by searching for the keyword "jianpu") ss.gp3.2023-09-17.03-43-47.mp4 |
This comment was marked as resolved.
This comment was marked as resolved.
@cbjeukendrup |
@Auase Thanks! I'll re-review the changes as soon as possible, but I'm very busy these days so it might take a while. Sorry! |
@cbjeukendrup |
I'd be happy to take a look at this from a product design point of view. Is it ready for a review? |
Yes, thank you @Tantacrul |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments! This PR will probably require a few more iterations before it is ready to be merged, but don't worry about that; that's always the case with big new things. So, even though code review comments might sometimes come across as a bit strict, never forget that we appreciate your work very much!
share/locale/musescore_zh_CN.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editing these files is fine, but not very effective, since for release builds they are overwritten with the latest translations from Transifex. So, once this PR gets merged and the 4.2 strings are pushed to Transifex, you can add the translation there: https://app.transifex.com/musescore/musescore/dashboard/
fonts/jianpu/Jianpu.fcp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of file is this?
src/engraving/dom/lyrics.cpp
Outdated
@@ -171,6 +171,14 @@ void Lyrics::layout2(int nAbove) | |||
LayoutData* ldata = mutLayoutData(); | |||
double lh = lineSpacing() * style().styleD(Sid::lyricsLineHeight); | |||
|
|||
if (staffType() && staffType()->isJianpu()) { | |||
double yo = segment()->measure()->system()->staff(staffIdx())->bbox().height(); | |||
placeBelow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function call has no effect; placeBelow
does nothing, but only returns whether the placement should be "below". (But yes, the name is a bit confusing.)
You can just remove this line.
src/engraving/dom/staff.cpp
Outdated
@@ -104,6 +104,10 @@ staff_idx_t Staff::idx() const | |||
|
|||
void Staff::triggerLayout() const | |||
{ | |||
if (staffType() && staffType()->isJianpu()) { | |||
score()->style().set(Sid::MusicalSymbolFont, "Jianpu"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not good; you're changing a score-wide style setting here. During the layout process, no style settings should be changed.
Did you implement it this way because the Jianpu font is currently a copy of Leland with some symbols added? But that means that if you have a score that uses a different font, and you add a Jianpu staff, it will suddenly change to Leland (well, the Jianpu copy of Leland).
I think the Jianpu font should only contain Jianpu characters, and the rest of the score should keep using whatever font it was using; for example, the original Leland.
However, we'll need to think about a good way to keep Jianpu properly separated from SMuFL, while also being able to reuse MuseScore's existing symbol drawing logic. For example, the logic around SymId is indeed quite nice to use. Perhaps we can generalise EngravingFont a bit, so that it can have multiple "subfonts": one for SMuFL symbols, one for Jianpu symbols. Actually, a bit like you're already doing, but a bit more generalised and cleaned up.
@igorkorsukov You doubtlessly have some interesting thoughts about this; could you help us with some ideas here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cbjeukendrup I agree with you!
I think we need to add a new style parameter something like JianpuFont
And get the font implementation by Sid
or staff type
that is, we need to do something like this:
- generalise EngravingFont (and maybe add type to EngravingFontsProvider::addFont, see enum Font::Type)
- remove Score::m_engravingFont
- change
Score:engravingFont()
to two methods:Score:engravingFont(Sid )
andScore:engravingFont(const StaffType* )
- try compile, fix all code where use
Score:engravingFont
(changeLayoutConfiguration::engravingFont
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have `engravingFont(StaffType*)’. It's not that different staff types should use different fonts. It's that the different staff type uses different symbols at different codepoints, that are not part of the SMuFL spec. That's why we can't use SmuFL fonts for that so we need to combine a SMuFL font with a Jianpu font preferably in one structure (like EngravingFont) so that we can reuse our SymId and symBbox logic etc. The question is how exactly that should work though. Especially since would ideally want the user to be able to choose any text font as their Jianpu font, but for text fonts there is of course no metadata file with bbox info, so we need to compute that info ourselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, since Jianpu characters are just text, we should maybe just use plain strings to represent the symbols and FontMetrics to get the metrics. (Of course, we can use named constants for each Jianpu character instead of writing string literals everywhere.)
How do we feel about that idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your idea.
I'm thinking about how this task is related to the display of Tablatures, it also has its own font...
I also think that this task is rather infrastructural and has little to do with the current PR (that is, we need to improve the font system as general). Therefore, I would prefer that this PR be done as desired, the just working..., and then the system fonts need to be improved separately.
src/framework/fonts/fonts_Smufl.qrc
Outdated
@@ -3,5 +3,8 @@ | |||
<file alias="fonts/smufl/classes.json">../../../fonts/smufl/classes.json</file> | |||
<file alias="fonts/smufl/ranges.json">../../../fonts/smufl/ranges.json</file> | |||
<file alias="fonts/smufl/glyphnames.json">../../../fonts/smufl/glyphnames.json</file> | |||
|
|||
<!--Because MODULE_QRC cannot import multiple qrc, it can only be imported here.--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I don't see why using multiple qrc files in MODULE_QRC would not work; for MODULE_BIG_QRC
it apparently works too. Have you tried it, and what kind of problems did you get?
src/framework/fonts/fontsmodule.cpp
Outdated
@@ -34,7 +34,7 @@ static void init_fonts_qrc() | |||
Q_INIT_RESOURCE(fonts_FreeSans); | |||
Q_INIT_RESOURCE(fonts_FreeSerif); | |||
Q_INIT_RESOURCE(fonts_Gootville); | |||
Q_INIT_RESOURCE(fonts_Leland); | |||
Q_INIT_RESOURCE(fonts_Jianpu); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Note to other reviewers: Leland is already listed a few lines higher too)
src/engraving/dom/stafftype.h
Outdated
@@ -415,6 +415,9 @@ class StaffType | |||
mu::PointF chordStemPosBeam(const Chord*) const; | |||
double chordStemLength(const Chord*) const; | |||
|
|||
// Check lines() == 0 to prevent conflicts when there are multiple staff; | |||
bool isJianpu() const { return (name() == "Jianpu" && lines() == 0) || (xmlName() == "stdJianpu" && lines() == 0); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be possible to create a new StaffGroup::JIANPU
, so that we can implement this method more similar to isTabStaff()
and isDrumStaff()
? I think that would also allow us to not worry about properties that are not relevant to Jianpu like number of staff lines, so that we can just ignore the values of these properties.
double symWidth = item->symWidth(SymId::keysigJianpuDo); | ||
double symHeight = item->symHeight(SymId::keysigJianpuDo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In situations like this, it's more efficient to call symBbox
once, and then get the width and height from that rectangle. That's more efficient because symWidth
and symHeight
both call symBbox
internally, so if you call both of them separately, the bbox calculation has to be done twice, which is a bit expensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still quite some "magic numbers" in this file (and some other files), of which the meaning is unclear... at least adding comments and/or named constants would be good. But in some cases, you should perhaps create a special style setting dedicated to Jianpu. For example, you have double lw2 = item->beamWidth() / 6.0;
. In my opinion, taking 1/6 of the width of a normal beam is a somewhat random thing to do. I'd just create new style parameter for Jianpu beam width, instead of basing it on normal beam width while there is not a very direct relation between those two values.
double symHeight = item->symHeight(SymId::keysigJianpuDo); | ||
double lw2 = item->beamWidth() / 6.0; | ||
for (const BeamSegment* bs : item->beamSegments()) { | ||
double y1 = bs->line.y1() / 1.65 + symHeight / 2.55; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another one... if you have to divide by 2.55
, that looks to me like there should actually not be a direct relation between the height of that symbol and the value you want to calculate here. Instead, you could base the value on the overall size of items in the score (i.e. spatium
), or create a style setting for it.
(regarding style settings: I mean those parameters that can be accessed using keys from the Sid
enum. Not all of them are (currently) exposed as an actual style setting in the UI, but the Sid
system is useful for forcing yourself to use named constants, and makes it easy to make certain values still customisable in the future.)
@Auase - I've been looking over this and have a few comments. First, if I understand correctly, you have created a new font specifically for Jianpu? If so, think it would make more sense to put them in Leland, since there aren't that many unique symbols - and we'd then be able to gradually improve the engraving and not be left with a new font that works by different rules. In order for that to happen, you'll need to send your font to @oktophonie and me so that we can have a look over it. I would also like to ensure that there is some consistency in the quality of the engraving. Secondly, there appears to be quite a few issues with placement / clashing elements.
![]()
![]()
![]() I'm aware of a few variations for accidental placement with Jianpu but accidentals seem to either be centred: Or if they are positioned higher, they are larger for legibility: Here's the Guitar Pro version: Again, I think out engraving team, and myself should be helping out here.
|
Just to summarise, I'm very excited about the idea of supporting Jianpu and I'll be happy to help by incorporating the correct symbols into Leland / Leland Text (I'm one of the two creators of Leland by the way). I would also like to make sure that everything about it has a high level of quality, while @cbjeukendrup helps on the implementation side. To begin, I would like to know what your intended goal is for this first implementation? Is it to provide the same level of support as Guitar Pro 8.1? In other words, single-line Jianpu only? |
@yuki0035 - thanks for this! One thing I'm pretty certain about is that we shouldn't use the time signature numbers from Leland for this but should actually just use regular numeral unicodes, so any font can be chosen. Of course, we'll pick a house style so it looks really nice by default. |
@Tantacrul |
@Tantacrul |
My goal is to have basically the same level of support as Guitar Pro 8.1, but there is no need to support multiple notes at the same position and some complex playing techniques, because when you really use Jianpu, you don't need these, because these complex things are not Jianpu can be expressed. |
After @Tantacrul's comments I've started to doubt about whether we should put Jianpu characters in Leland or in a separate font. As far as I can see, these are the three options we have:
As far as I've seen so far, Jianpu only contains usual characters like letters and numbers, so creating a custom specification seems not sensible. So the choice is between option 1 and 2. I think that for the development of this pull request, we should follow this roadmap:
|
That's true, but I really don't want to add too much code with repetitive functionality. As for what decision to make, it can only be decided by you and I am powerless. |
@Tantacrul |
A few general comment:
![]()
|
Oh, I understand this and I think the work you've done so far has been very promising. These things always take time! |
@yuki0035 |
@Tantacrul |
There's no rush with this. It's worth taking our time to get it right. |
My 2 cents: I think it'd be worth it to take the time to allow using plain text fonts for this. There's a lot of different score styles out there and one of the simplifications offered by Jianpu is that in essence "it's just text" and any word editor can be used to make it. Given that there is (to our knowledge) no current font standard for the notation system I'd think it be good to support as much variants as possible. Enforcing a new font standard might be a bit less effort, but feels as an unnecessary artificial restriction to me. I'm really happy to see this long standing request getting some love. Good job @Auase ! |
If there are codepoints, it is not difficult to enable users to choose to change font styles. |
@cbjeukendrup @Tantacrul |
@Auase We're still waiting for @oktophonie's opinion. But we'll let you know as soon as possible! In the meantime, you can already work on my other comments. My general message about layout/drawing code is: calculate things based on the |
Ok |
|
@Tantacrul |
The codepoints would then be the plain ASCII codepoints for digits; Which would open up any text font to be chosen. |
This is not an ASCII code point, so common text fonts cannot be used, but it is perfectly fine to add other font styles, but they will need to be added manually by the developer. |
Again @Auase - I just need to discuss with @oktophonie - although I'd be pretty sure we'll end up agreeing with @jeetee that for numbers, we should use basic ASCII unicodes so any font will work. There's no point putting sans serif numerals in Leland for this purpose. I definitely would not point to those unicodes in Bravura because they're almost certainly there for some other intended purpose. |
@Auase @Tantacrul @cbjeukendrup How about adding an option something like I'm afraid that here are a lot of changes regarding the layout, and we are now actively working on the layout, there may be conflicts or we may even lose this work... I think we need to disable this function from the interface. |
I don't mind this as long as there's no way for the user to discover this in the UI. Then we figure out next steps - start again or keep and refine. @cbjeukendrup ? |
From a code-review point of view, it's not convenient to merge this PR right now. The code will need some refinement before we can consider it "production ready". It is much easier to make these refinements while we have all code together in one Pull Request, where we can easily exchange comments about the code and see what has been addressed and what not. When we merge it right now, the code is scattered around the codebase, and we'll have to look manually which code was from this PR and which parts of it need refinement; and we'll have to manually write lists of code review comments. This makes nobody happy; not the reviewer but also not the contributor. What would make these inconveniences even bigger, is that this PR will most likely need a multiple rounds of reviews and refinements: for example, as long as we haven't taken a decision about how to handle fonts, it does not make much sense to look at the specific layout code, because that will probably change anyway when we choose a different way for font handling. All this back-and-forth communication is easier when it happens in one place, and if we can immediately see the updated version of the whole Jianpu code when the contributor makes refinements. However, Igor has certainly an important point: currently, we're making a lot of changes to the layout code. This can cause conflicts with this big PR. This is a bit inconvenient, but @Auase if you rebase your branch often, and watch Igor's PRs to see what he is doing, then it shouldn't be too much of a problem. Also, @Auase, we can still help you while the changes are not merged but only in a PR: we can namely also push changes to your branch. If we coordinate this well, this can lead to a very productive collaboration. Conclusion: let's not merge it now, and make some refinements to it first, until most of it is ready and the "to do list" has been reduced to a few concrete steps (that can easily be performed and verified one by one). Once we reach that point, we can merge it and do those few concrete steps in next PRs. (I've already discussed this with @igorkorsukov, by the way) |
I agree, and my opinion is to wait until the font solution is resolved before considering merging it. As for the changes to the main branch, I think there is no problem with the Jianpu branch following the changes to the main branch (the main branch had major changes before I submitted it), because I understand the entire logic. |
@cbjeukendrup |
I'm a little busy these days. I'll open it after I finish it in a while. |
Hey @Auase Apologies for the delay. Been pretty busy the last week. After a chat with @oktophonie, and some thoughts from @cbjeukendrup and @jeetee, I think we all feel that the best approach would be this.
I'll put together a recommendation about spacing / alignment of elements and share it here soon. This won't block the work. It's simply about making Jianpu look really nice and consistent with the spacing we've used in the score in general. How does this sound? We can obviously guide you though any uncertainties. We've done a lot of this in the past and we're confident that this approach will work nicely. |
That sounds great, thank you. But this solution may result in some more code with repeated functions. If you have any good ideas, please tell me here. I will follow the main branch to improve it. If I encounter a problem that I am unable to solve, I will ask everyone for advice when the time comes. But recently, there are some things in my company that I need to deal with, so please give me some time. |
Was the @Tantacrul video mentioning jianpu removed? |
Removing videos of mine is an offence punishable by death. |
I may be a little confused, but at one point I thought this PR was shipped in v4.2, and was super excited. I'm trying to adopt a violin piece for a fellow Erhu player of mine, and sincerely hope this get resolved soon 🙏 |
This confusion might have arisen from the fact that the Jianpu contribution was mentioned here: https://musescore.org/en/node/355107 |
Also, the video you mention, was that https://youtu.be/Eq3bUFgEcb4?feature=shared&t=4279 ? |
Ah, yes indeedy! |
I will continue and haven't given up, but I really don't have time recently. Sorry. |
We are looking forward to the implementation of this feature and we really need it. |
Resolves: #NNNNN