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

fix: SVG export in dark mode with embedded bitmap image #4285

Merged

Conversation

zsviczian
Copy link
Collaborator

@zsviczian zsviczian commented Nov 20, 2021

Solves this problem:

2021-11-20.11-27-11.mp4

After fix:

2021-11-20.11-33-06.mp4

This reverts commit 8407268.
bitmap images embedded in SVG are inverted when exporting in dark mode.
to avoid impacting package.json of main project
@vercel
Copy link

vercel bot commented Nov 20, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/2QuLg67GkPctXhM5SX9TVU3RSnq7
✅ Preview: https://excalidraw-git-fork-zsviczian-svgexport-image-e2a984-excalidraw.vercel.app

@zsviczian zsviczian changed the title Svg export image symbol inversion fix fix: SVG export in dark mode with embedded bitmap image Nov 20, 2021
@dwelle
Copy link
Member

dwelle commented Nov 20, 2021

Nice catch!

It should go into:

case "image": {
const fileData =
isInitializedImageElement(element) && files[element.fileId];
if (fileData) {
const symbolId = `image-${fileData.id}`;
let symbol = svgRoot.querySelector(`#${symbolId}`);
if (!symbol) {
symbol = svgRoot.ownerDocument!.createElementNS(SVG_NS, "symbol");
symbol.id = symbolId;
const image = svgRoot.ownerDocument!.createElementNS(SVG_NS, "image");
image.setAttribute("width", "100%");
image.setAttribute("height", "100%");
image.setAttribute("href", fileData.dataURL);
symbol.appendChild(image);
svgRoot.prepend(symbol);
}
const use = svgRoot.ownerDocument!.createElementNS(SVG_NS, "use");
use.setAttribute("href", `#${symbolId}`);
use.setAttribute("width", `${Math.round(element.width)}`);
use.setAttribute("height", `${Math.round(element.height)}`);
use.setAttribute(
"transform",
`translate(${offsetX || 0} ${
offsetY || 0
}) rotate(${degree} ${cx} ${cy})`,
);
svgRoot.appendChild(use);
}
break;
}

Also, let's use this filter instead (gives better results — at least for canvas double invert, not sure about SVG):

// using a stronger invert (100% vs our regular 93%) and saturate
// as a temp hack to make images in dark theme look closer to original
// color scheme (it's still not quite there and the clors look slightly
// desaturing/black is not as black, but...)
context.filter = "invert(100%) hue-rotate(180deg) saturate(1.25)";

@zsviczian
Copy link
Collaborator Author

Why a separate declaration. Why not use THEME_FILTER and change the filter here instead?

$theme-filter: "invert(93%) hue-rotate(180deg)";

I am using THEME_FILTER because it needs to mirror this:

svgRoot.setAttribute("filter", THEME_FILTER);

@zsviczian
Copy link
Collaborator Author

Modifying renderElements led to a cleaner solution. Also, I don't need to hardcode filter, I simply use the filter on svgRoot, since that is the one I need to revert.

@dwelle
Copy link
Member

dwelle commented Nov 20, 2021

am using THEME_FILTER because it needs to mirror this:

Inverting something by 93% and then inverting it again by 93% doesn't give you the original color. That's why I'm using a different filter to more approximate the original colors, though it's not perfect (I also haven't done any exact math and it's not clear to me whether we could come up with a filter in one step to invert it precisely, since we can only invert by up to 100%).

Here's the examples, in order: 1) the same filter 2) the modified filter 3) ground truth

image
image
image

You can see that both the dark-mode images are less saturated, but when using the same filter (1) it's way less saturated than (2).

@zsviczian
Copy link
Collaborator Author

still, at least they are not inverted. Excalidraw is not photoshop. Yes, it is great if pictures look saturated, but if you are after perfect colors, work with photoshop, if you want to sketch, the current solution is fit for purpose.

So why not change
$theme-filter: "invert(93%) hue-rotate(180deg)";
to
$theme-filter: "invert(100%) hue-rotate(180deg) saturate(1.25)";?
for consistency?

@dwelle
Copy link
Member

dwelle commented Nov 20, 2021

So why not change
$theme-filter: "invert(93%) hue-rotate(180deg)";
to
$theme-filter: "invert(100%) hue-rotate(180deg) saturate(1.25)";?
for consistency?

Inverting white by 100% gives you pure black which isn't a good default for dark mode. IIRC the invert(93%) hue-rotate(180deg) comes from a Google styleguide. IOW, we're not gonna change our filter because it'd change how our canvas dark mode looks across the board.

still, at least they are not inverted. Excalidraw is not photoshop. Yes, it is great if pictures look saturated, but if you are after perfect colors, work with photoshop, if you want to sketch, the current solution is fit for purpose.

I'm not sure what you're arguing for here. You can see above that using the modified filter gives you outputs closer to ground truth, so why not use that? Is the only argument against this a code consistency?

@zsviczian
Copy link
Collaborator Author

Got you, so what you are asking is that instead of
const filter = svgRoot.getAttribute("filter");

we do:
const filter = svgRoot.getAttribute("filter") ? "invert(100%) hue-rotate(180deg) saturate(1.25)" : null;

?

@dwelle
Copy link
Member

dwelle commented Nov 20, 2021

I've refactored the code and instead of detecting dark mode from filter attribute, I'm passing the flag from upstream. Presence of filter doesn't have to mean presence of dark mode in the future.

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.

None yet

2 participants