Fix #317084: SVG honors z-order of staff-lines and includes Staff Typ… #7443
Conversation
worldwideweary
changed the title
worldwideweary
force-pushed
the
317084-SVGHonorZStackingOfStaves
branch
from
fc9e97e to
74a701e
Compare
worldwideweary
force-pushed
the
317084-SVGHonorZStackingOfStaves
branch
from
74a701e to
8a5e6f1
Compare
worldwideweary
changed the title
|
Just built this branch on my Windows 10 for MinGW, and just reviewed the code changes. So this change in style is just a change in style, not an actual efficiency gain? I am not familiar with this style change you mention, nor do I know what "banner" style means. I cannot find a style guide online. Can you point me there or explain it here please? |
Guideline should be easy to find via search engine: https://musescore.org/en/handbook/developers-handbook/finding-your-way-around/musescore-coding-rules under "Coding Style" etc. Certainly, indentation and bracket positionings hold no efficiency quality/gain whatsoever. Lines 3032-3037 were braced not in accord with the coding guideline (and had 4-space indent instead of 6) Not important from substantial code point of view, just figured since while I was fixing the errors of staff z-order and not taking into account Staff Type Changes, I might as well get the code in accord with the guideline (3.x... master branch follows different rules) |
|
OK, I'm misunderstanding. I was talking about the actual changes you made to the code, not formatting. Do the changes you made to rearrange the structure of the code result in any practical gains, or is it just a preferred "style" of coding? The code seems to be about the same size. I'm not complaining, I'm just asking. The code looks fine to me. I am not quite able to run it yet... |
|
OK, now I get it. You integrated the stafflines code into the elements loop. Sorry it took me so long to understand that that was the structural nature of your change. I can understand the motivation for that. Though it might hurt performance, I assume it will be negligible, and the code has a clarity of style to it. |
|
Cool. Just so that you are aware, and I think I mentioned back at the .org forums (this didn't exist back a year ago from what I remember): you can just download artifact builds so you don't have to locally build a PR. It's faster that way to test and potentially less taxing on your local system . For example, if you click:
It's a nice addition to their build system. P.S. Hopefully having the one-pass system shouldn't hurt performance, I can't really see why it would. One thing that might slow it down (but we're talking milliseconds probably) is the additional checking for the Staff Type Change. I made it so it checks measures entirely and not just the start because of it being possible for STC in the future to function within a measure rather than as "on" the measure. On my older system I didn't notice any time difference during export tests. |
|
Thanks for the tip. I'm not in a mode of building MuseScore a lot these days, and I had other issues getting this branch up and running. Yea, this is a File menu Export operation, so even if it takes a whole second or two longer, it's not a big deal. The separate loop for stafflines dates back to version 2. I believe my assumption was that capturing/handling staff lines merited separation because of the extremely low ratio of stafflines to other elements. I was focused on large scores that perform the worst, and the previous code was very different. But enough said. I don't think it matters much, if at all, and if this works, then great. It's nice to have someone else around to understand and help out with this code. |
|
Definitely. If it weren't for potential elements below the staff's z-order, it very much makes logical sense to draw staves within a separate pass to aid per-system drawing. Your mentioning to draw below-staff-z-order elements first also would've worked just fine but I had this already almost finished and figured I'd finish it as it was going. |
|
OK, I have found an issue, but have yet to debug it. I have attached two .mscz files and their corresponding one page of .svg files in a zip. The issue is that duplicate Stafflines elements are drawn, and the quantity of duplicates is a multiple of the number of staves. So in svgtesty2, with two staves, there are 2 of every staff line (1 copy). In svgtesty3, with 3 staves, there are 3 of every staff line. I have a personal score with 4 staves and there are 4 of every staff line. You can count the number of Stafflines elements in the file, they are repeated in order, not adjacent copies. So with 2 staves and 2 systems there should be 20 staff lines: 5 per staff * 2 staves * 2 systems. Instead there are 40. |
|
@sidewayss, Okay, thanks for testing this out. |
|
Once again, I appreciate your involvement. The problem appears to be that I assumed that the element list was some how so well ordered that systems were linearly involved, but that wasn't the case. For instance, some staff elements are on one system, and then upon another later down the list, but then of the previous system again afterwards. It's only ordered by z-order. I had only checked if the current system was equivalent to the most recent system when traversing the score's element list rather than verifying more fully. I include an extra commit to alleviate that problem. Should be fixed! Any further testing of course is welcome, but I include here a zip with mscz+output file that has four pages and each checked out okay without duplicates: |
|
Sounds like it might have been simpler to leave the staff lines in a separate loop :-). I'll download, build and test it tomorrow some time. |
Yeah. It wasn't so bad, as I learned a thing about the element list provided. At first I thought my continue statements were possibly in the wrong bracket section or something because it read nice and logical and it didn't make any sense until i output some system pointer information and saw the back and forth action. If I had to start all over again, I would have more than likely just done a 3-pass approach |
|
After sleeping on it I realized that you should probably to go back to that 3 pass approach. Open the svg files in a text editor. Note that the Stafflines are down the page. SVG z-order is based on the file order of the elements, top to bottom. Not that I'm not seeing elements that might actually be a problem, like note heads or stems, but the staff lines should really be first in the file, after your image(s). A few years from now someone with colored note heads or something will complain that the staff lines are on top. This might have been part of my reasoning for structuring it that way. It was 5 years ago - sorry I didn't remember it before you started coding. This is why it's best to change as little as possible in a large open source code base like this unless you are interested in learning all the details of how it works... |
|
Never did it, so couldn't go "back" to it :) From my testing, though, It's okay. Color the noteheads, export, and the result is fine. Screenshots of SVG output: Here's first page which was per-measure staff (maybe that was why the staffs are down the page?): And here page 4 where all staves were drawn per system: No z-order problem manifests is demonstrated in viewing the resulting image of the svg file. I also changed bar lines to be behind and above with different colors and the results were fine. May your observations been due to the staff lines being drawn per-measure within the first two svg files in my provided zip? Of course the staves would be down the page since they follow the z-order arrangement from the sort function. If you can though, please demonstrate an erroneous z-order if you find something, like a picture or mscx file. If it can be demonstrated, I'll be sure to change it since the whole point here is to have appropriate z-order in viewing an SVG file (+ the staff type changes). In other words, I don't want to redo the code and build again unless I know it's necessary due to some demonstrated error. |
|
Tie segments are an example of a class of element that appears prior to staff lines in the attached, über-simple 3 staff example. Note that the red tie segment is underneath the staff line. I was using note heads as a hypothetical example that might come up in the future. IMO it's far better to be safe than sorry in this kind of circumstance. |
|
I'm not understanding what you mean by safer than sorry here. The tie shows that the Z-order is 1100 in the actual Musescore file, which is default when I make a new file also. The SVG file is merely honoring the actual z-order MuseScore is providing. That's to say, if that's a problem, this should be resolved in the main MuseScore code and not be accommodated strictly in SVG output. Staves seem to be 1200 z-order (or between 1200-1300 i haven't verified 100%). Hell, maybe we should do a PR for changing the z-order of ties and whatever else there is that's below 1200 when it should be like 1300? |
|
The result is not the same as viewing the file in MuseScore, thus it is incorrect. I would bet that the z-order of that measure has the stafflines under the tie. It is only because the code consolidates the elements by type that the tie segment appears before the staff lines in the .svg file. This is an error and should not be allowed. That's the most important point here. Stafflines should always be first in the z-order except in the exceptional circumstance your issue raises. |
|
Unless I'm misunderstanding something, the result is the same on my machine: both MS and SVG show as being behind the staff with the tie at z-order 1100 as with a new score doing the same thing. What are the results on your machine? P.S. Even if i were to do a first pass for all elements that were below staff z-order of 1200 in a 3-pass system, draw them, and then draw staff lines, the tie would be below the staves since the tie is 1100 and the staff is [~1200-1300] I don't agree that SVG should be not in accord with MuseScore's main output. MuseScore itself should be updated if it is generally conceived that a particular element should be at a certain z-level by default. Your MS tie: Your SVG tie: your "thus it is incorrect" statement doesn't seem to be true |
|
OK, I had to rebuild my musescore exe in order to view this in MuseScore again. After zooming in multiple times it does seem that the tie is underneath the staff line in MuseScore too. That must be new in v3 or 4 somewhere. You are correct that it should be a separate issue in the MuseScore issue tracker. Barlines are below the stafflines too, along with measure numbers and several other things. I just did a test in v2.1 and it's very hard to tell, but it looks like the tie is under the staffline inside MuseScore. Certainly it is in the resulting SVG file. I think this is an interesting debate, whether the existing SVG Export functionality should be maintained or whether it should be updated to reflect MuseScore's current state, even if that state might be in error. I don't think it's as clear cut as you make it out to be, and I tend to weigh in on the side of correct output in the export, especially since it is a zero change for the functionality in question. I'm going to inquire a bit about the general z-order on the Telegram Developer's chat. Join in if you like. |
|
Ok cool. The thing is that it's not a zero-change. As mentioned earlier, the three-pass system would still result in a tie for example (z:1100) be behind the staff on export because the first pass would find all elements below the staff's z-order and print them first. It shouldn't only take into account PNG images or SVG images to be drawn first like with my image, but anything below staff z-order. So I think that mainly the MuseScore code should be updated if it's agreed that some of these elements should by default always be on top of the staff: i.e. greater than staff z-order. |
|
Yet it does not. The Slur Segment, as it was called in 2.1, appears after the Stafflines in this .svg file exported from 2.1. Though again, I'm not sure yet if that is because of the Export code or because something changed in MuseScore z-order since then. Hopefully someone can answer that so that neither of us has to investigate the history of this mess... |
|
...and the check for elements below the stafflines in z-order could limit itself to particular element types that belong below the stafflines, like images. |
|
The code shouldn't check for only specific elements. What if the user wants something to be below staff not by default but by manual z-order change? I hold the position that SVG should honor those customizations just like in the main program. The problem seems to me to have the default z-orders potentially re-imagined if it's a problem for any "corner case" users like you :) (and I) |
…e Changes Duplicate of musescore#7443, part 1
Duplicate of musescore#7443, part 2
…e Changes Duplicate of musescore#7443, part 1
Duplicate of musescore#7443, part 2
…e Changes Duplicate of musescore#7443, part 1
Duplicate of musescore#7443, part 2
…e Changes Duplicate of musescore#7443, part 1
Duplicate of musescore#7443, part 2
…e Changes Duplicate of musescore#7443, part 1
Duplicate of musescore#7443, part 2
…e Changes Duplicate of musescore#7443, part 1
Duplicate of musescore#7443, part 2
…e Changes Duplicate of musescore#7443, part 1
Duplicate of musescore#7443, part 2
…e Changes Duplicate of musescore#7443, part 1
Duplicate of musescore#7443, part 2
…e Changes Duplicate of musescore#7443, part 1
Duplicate of musescore#7443, part 2
…e Changes Duplicate of musescore#7443, part 1
Duplicate of musescore#7443, part 2
…e Changes Duplicate of musescore#7443, part 1
Duplicate of musescore#7443, part 2
…e Changes Duplicate of musescore#7443, part 1
Duplicate of musescore#7443, part 2
|
3.x is closed for any changes |
…e Changes Duplicate of musescore#7443, part 1
Duplicate of musescore#7443, part 2
3 participants
…e Changes
Resolves: https://musescore.org/en/node/317084
Staff lines have a z-stack order which weren't being honored during SVG output (PDF/PNG/Main program are fine). So for instance, if an image is given lower z-order below staves, it would end up stacking on top of the staves in exported SVG files. This PR fixes this by not drawing staff-lines before all other elements but having them conjoined in the process while being careful not to draw extra ones since there's custom code from @sidewayss, but also in the process, it was realized that because staff lines were being drawn per system before Staff Type Changes were implemented, those also were not taken into consideration. This also updates so they are taken care of, but there may be a better way to do so. For now, at least this gets the ball rolling. Any comments are appreciated
In the meantime, a quick fix up of some non-conformity to the "banner" style in 3.x was cleaned up. For all main purposes, the code is practically the same as it was even though it shows huge portions deleted. it was merely reformed within a one-pass system, and I've had no problems so far,but I would much appreciate some extra testing to be sure!
Update: So far working fine with a lo-z image + Staff Type Changes have passed testing (along with intermixed horizontal frames to be sure related with STCs)