Skip to content

Transparent image overlap when using Overlap=0 in dzi #1470

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

Merged

Conversation

torbjorn-kvist
Copy link

@torbjorn-kvist torbjorn-kvist commented May 18, 2018

When using overlap=0 in dzi, we get a ~1px grid/border around each tile.
overlap_osd

Include a zip as a test, with no changes:
overlap_test.zip

Changing the clearReact size.x/y to -1 instead of -2 fixes the problem. There could be adverse effects that I'm not aware of.

@iangilman
Copy link
Member

This seems like a good change. It makes sense that trimming a pixel off of every side leaves overlap (which appears here as darker areas). By trimming off of just one side, we allow things to abut properly.

I guess one thing I'm curious about is whether the DZI tile overlap has any effect on this... This sample is done with overlap 0... What if we had overlap of 1 (and the same transparency)? It would be great if we could fix this so it would work for that scenario as well. @DVidan would you be up for giving that a try?

@avandecreme I'm curious if you have any thoughts.

@torbjorn-kvist
Copy link
Author

I can fix one with overlap 1 on monday, with the same image and transparency. I don't have access to the files at home. Anything else that would be good to have?

@torbjorn-kvist
Copy link
Author

Overlap 1:
example_overlap_1

With PR:
example_overlap_1_fixed

Zip contain example code and with both overlap 0 and 1.
overlap_test_0_and_1.zip

@avandecreme
Copy link
Member

Shouldn't the whole thing be:

             context.clearRect(
                 position.x + overlap,
                 position.y + overlap,
                 size.x - 2 * overlap,
                 size.y - 2 * overlap
             );

It would also be nice to check that it doesn't break with rotation and edge smoothing.

@iangilman
Copy link
Member

@avandecreme Yes, it probably should've been based on the overlap instead of just being 1 but size - 2 * overlap is the source of the problem we're trying to fix here; by doing that, we clear just the portion of the tile that is not overlapped, but not the portion that overlaps. If there's transparency in the overlap, then the overlap section gets darker because of the overlapping.

size - overlap fixes this so we're clearing the inner section and the overlap. In this case we clear just the left side overlap (for instance) and let the next tile clear our right side overlap (which its left side overlap). In this way each overlap is only cleared once.

That said, when I change the code to just size without subtracting anything for the overlap (and am therefore clearing each overlap twice) I can't detect any problem. The comment says we avoid clearing the overlap to avoid edge flickering, but I don't see it. That comment is at least 5 years old, and it's unclear to me exactly what problem it was solving at the time.

Good idea to check with edge smoothing and rotation. Edge smoothing seems fine (I did it with maxZoomPixelRatio: 10). Rotation, on the other hand, presents a challenge. Here I am rotating it 30 degrees with size - 1:

image

Note the white lines visible from the erasing.

With size - 2 the white lines are gone but the black lines are gone. With size - 1.5 we have flickering black lines but no white lines. It's possible we can't fix that entirely (unless we do all the tiling un-rotated and rotate afterwards). If we go with just size instead of size - 1, the white lines appear to be the same, by the way.

So I'm tempted to say we should just do:

context.clearRect(
   position.x,
   position.y,
   size.x,
   size.y
);

...and see how that goes. What do you think?

@avandecreme
Copy link
Member

avandecreme commented May 21, 2018 via email

@iangilman
Copy link
Member

Looks like it's one of the very early things @thatcher did:

f6ee93b

@iangilman iangilman closed this May 22, 2018
@iangilman iangilman reopened this May 22, 2018
@iangilman
Copy link
Member

@DVidan What do you think?

@torbjorn-kvist
Copy link
Author

@iangilman Looks good :) Want me to update the PR with the changes?
(Sorry for the late replay, somehow the notifications from github didn't show up.)

@iangilman
Copy link
Member

@DVidan Yes please; Let's give it a try :)

What browsers have you been able to test on, BTW?

@torbjorn-kvist
Copy link
Author

@iangilman Updated.

Tested in:
Chrome: 66.0.3359.139
Firefox: 60.0.1

@iangilman
Copy link
Member

Great, let's do this! Thank you both for thinking this through with me :-)

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.

3 participants