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

Draw colored Background Layer #318

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

sschmidTU
Copy link
Contributor

@sschmidTU sschmidTU commented Aug 3, 2018

We now draw a colored rectangle in VexflowBackend.clear(), which we call in OpenSheetMusicDisplay.render().

This enables directly right-click copying or saving the rendered score when using the Canvas backend, which is pretty convenient. Now we don't have to take a screenshot or edit image transparency anymore.

Previously, the canvas image had a transparent background:
image

Now it's white by default (saved as engraving rule).

The commits displayed here aren't that relevant, just look at the File changed. I'll squash the messages.

@sschmidTU sschmidTU added the enhancement a (small) improvement that neither adds nor fixes a significant feature label Aug 3, 2018
@sschmidTU sschmidTU self-assigned this Aug 3, 2018
@sschmidTU sschmidTU requested a review from bneumann August 3, 2018 09:45
Copy link
Collaborator

@bneumann bneumann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also check the skybottomlinecalculator there is also a clear call but I can't remember which function it uses

@@ -161,8 +161,13 @@ export class OpenSheetMusicDisplay {
this.drawer.clear();
this.drawer.resize(width, height);
this.drawer.scale(this.zoom);

// clear and fill with background color
this.drawer.clear(0, 0, width, height);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 clears. Why?

Copy link
Contributor Author

@sschmidTU sschmidTU Aug 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the first clear actually only removes the SVG children in the SVG backend in this case. I left it in in case the resizing and scaling is slower if we don't remove the SVG children beforehand.

So, clear without arguments behaves like before, clear with position and dimensions makes a colored background (rectangle).

We could separate these methods, of course. But i don't think removing children from a list or drawing a single rectangle costs much performance.

// this somehow works, though.
public background_fillStyle: { fill: string };
public setBackgroundFillStyle(style: string): void;
public clear(x: number, y: number, width: number, height: number): void;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like the idea that I need to know my canvas size to clear it.

Copy link
Contributor Author

@sschmidTU sschmidTU Aug 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you only need to know it if you want to fill it with the background color though. with default arguments it behaves as before. Also, canvas.width and canvas.height is often wrong in the canvas backend, too small.

@sschmidTU
Copy link
Contributor Author

sschmidTU commented Aug 3, 2018

SkyBottomLineCalculator's clear calls will behave as before, because of default arguments.
canvascontext.clearRect() currently doesn't do anything in vexflow by the way.

@bneumann
Copy link
Collaborator

bneumann commented Aug 3, 2018

Alright rest looks good to me then

@sschmidTU
Copy link
Contributor Author

sschmidTU commented Aug 3, 2018

@bneumann so is there anything else i need to review? SkyBottomLineCalculator works as before.

@bneumann
Copy link
Collaborator

bneumann commented Aug 3, 2018

I can't think of anything else right now. That function is pretty low level but has not really been used before so should not be a biggie

@sschmidTU
Copy link
Contributor Author

@bneumann just asking because your review still requests changes. But i've noted your LGTM ;)

@bneumann
Copy link
Collaborator

bneumann commented Aug 3, 2018

Si señor, problem is I cannot reopen the review on my phone

@sschmidTU
Copy link
Contributor Author

Seems like i broke the cursor, so right now this should not be merged. I'm fixing it among other things.

@sschmidTU
Copy link
Contributor Author

The cursor was broken because it's drawn on z-layer -1 in HTML, behind everything else, so it needs transparency on the next layers to be visible. So right now, we can't have the cursor and a colored background.
I thought about drawing the background on a deeper z-layer similar to the cursor, but the problem is that the cursor does not appear on the image if you copy it because it's not on the same canvas, so the background will have the same problem.
The comments in the Cursor class do say that it should be implemented differently. But it won't be easy to draw it as a background layer like it is now.

We can actually have Cursor and background right now if we move the Cursor to the z=0-layer, but then the notes won't paint over the cursor anymore:
image
This is the current state of the PR, but i'm not sure we want to make this tradeoff right now.

@sschmidTU
Copy link
Contributor Author

Currently implemented is an optional background color:

osmd.setOptions({pageBackgroundColor: '#FFFFFF'})

this will make the cursor invisible.

This PR still has a better compromise, but always changes the look (see above).
(also see #312)

@matt-uib matt-uib closed this May 14, 2020
@matt-uib matt-uib deleted the fix/transparentToWhiteBackground branch May 14, 2020 08:48
@matt-uib matt-uib restored the fix/transparentToWhiteBackground branch May 14, 2020 09:45
@sschmidTU sschmidTU reopened this May 14, 2020
@sschmidTU sschmidTU force-pushed the develop branch 2 times, most recently from 9b4a2fa to a64b76e Compare March 30, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement a (small) improvement that neither adds nor fixes a significant feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants