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

Try to fix marker size #3073

Merged
merged 5 commits into from
Jul 20, 2023
Merged

Try to fix marker size #3073

merged 5 commits into from
Jul 20, 2023

Conversation

SimonDanisch
Copy link
Member

#2990 introduced a problem in the exact marker size:

marker-alignment

@MakieBot
Copy link
Collaborator

MakieBot commented Jul 18, 2023

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running:

using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(display(fig))
using create display create display
GLMakie 12.29s (12.05, 12.58) 0.21+- 1.20s (1.17, 1.26) 0.03+- 782.65ms (749.78, 868.00) 40.46+- 14.95ms (14.79, 15.24) 0.16+- 114.48ms (113.08, 117.35) 1.60+-
master 12.50s (12.34, 12.65) 0.11+- 1.18s (1.10, 1.24) 0.05+- 767.10ms (738.97, 798.97) 20.21+- 14.40ms (14.20, 14.72) 0.19+- 116.76ms (112.59, 126.72) 4.56+-
evaluation -1.66%, -0.2s faster ✓ (-1.21d, 0.05p, 0.16std) +2.32%, 0.03s invariant (0.69d, 0.23p, 0.04std) +1.99%, 15.55ms invariant (0.49d, 0.39p, 30.34std) +3.64%, 0.54ms slower X (3.13d, 0.00p, 0.17std) -1.99%, -2.28ms invariant (-0.67d, 0.25p, 3.08std)
CairoMakie 10.41s (10.30, 10.51) 0.07+- 1.05s (1.05, 1.07) 0.01+- 215.66ms (213.05, 217.89) 1.87+- 10.39ms (10.31, 10.49) 0.06+- 6.20ms (6.11, 6.27) 0.06+-
master 10.47s (10.31, 10.67) 0.12+- 1.06s (1.03, 1.10) 0.02+- 227.22ms (223.97, 234.28) 3.61+- 10.32ms (10.16, 10.41) 0.08+- 6.13ms (5.97, 6.31) 0.11+-
evaluation -0.63%, -0.07s invariant (-0.65d, 0.25p, 0.10std) -0.90%, -0.01s invariant (-0.58d, 0.31p, 0.01std) -5.36%, -11.56ms faster✅ (-4.03d, 0.00p, 2.74std) +0.68%, 0.07ms invariant (0.99d, 0.09p, 0.07std) +1.13%, 0.07ms invariant (0.77d, 0.18p, 0.09std)
WGLMakie 17.61s (17.05, 18.36) 0.48+- 1.67s (1.58, 1.74) 0.06+- 15.88s (15.60, 16.38) 0.26+- 22.85ms (19.67, 37.02) 6.28+- 1.61s (1.57, 1.66) 0.04+-
master 17.63s (17.12, 17.96) 0.28+- 1.66s (1.60, 1.75) 0.05+- 15.92s (15.49, 16.31) 0.28+- 19.16ms (18.10, 20.02) 0.77+- 1.61s (1.54, 1.67) 0.05+-
evaluation -0.12%, -0.02s invariant (-0.05d, 0.92p, 0.38std) +0.94%, 0.02s invariant (0.28d, 0.61p, 0.06std) -0.25%, -0.04s invariant (-0.14d, 0.79p, 0.27std) +16.14%, 3.69ms noisy🤷‍♀️ (0.82d, 0.17p, 3.52std) +0.01%, 0.0s invariant (0.00d, 0.99p, 0.04std)

@SimonDanisch
Copy link
Member Author

Hmpf, this does fix the above issue, but circle polys are now cropped:
image

image

@ffreyer
Copy link
Collaborator

ffreyer commented Jul 18, 2023

Yea that's kind of what I expected. You probably want even dividers for of the pixelsize for the aspect ratio.

Maybe we can clean up the rounding error at the other end when we calculate markersize and offset?

@ffreyer
Copy link
Collaborator

ffreyer commented Jul 18, 2023

I tried counteracting the distortion from rounding to full pixels by scaling widths(bb) (origin is fine regardless) in the offset and scale calculations but didn't really get anywhere...
From just experimenting with factors directly Vec2f(1.01838, 1) seems to be about where it needs to be for hex markers. That doesn't match up well with anything I can get out of ceil.(Int, x * aspect) ./ (x * aspect) where x is some pixel size between 10 and 256*64.

@ffreyer
Copy link
Collaborator

ffreyer commented Jul 19, 2023

I don't understand what I did yet but I think I got it working...
I mostly played around with the scaling and eventually found the factor from above in minimum(pad_scale) ./ pad_scale = (0.8824, 1) = (1.018, 1) .* (0.866, 1) and then continued to try stuff till it worked.

Screenshot from 2023-07-19 14-29-40
Bezier markers with bboxes:
Screenshot from 2023-07-19 14-28-14

@ffreyer
Copy link
Collaborator

ffreyer commented Jul 19, 2023

I reformulated/simplified the math here and added a comment that explains what's happening. I think this should be good to merge now. (Checked the two tests from above and a triangle with large offset)

@SimonDanisch
Copy link
Member Author

When I run using ReferenceUpdater; ReferenceUpdater.serve_update_page(pr=3073) I still see some moving around...
Interestingly, the polygon marker alignment test doesn't move at all:
image

@ffreyer
Copy link
Collaborator

ffreyer commented Jul 19, 2023

I think the reason for the scaling factor being so large (0.018 rather than 1/256 = 0.0039 or even 1/64 = 0.015625) is that the integer rounding errors compound during downsampling. So I made another change that effectively adds padding to avoid this. I don't really see things moving though, it just removes a bit of blur on hline (compared to last commit) or vline (compared to refimgs).

Refimg

BezierPath markers

No pixelsize matching (before commit)

BazierPath markers new

With pixelsize matching (after commit)

BazierPath markers new fractional size

@ffreyer
Copy link
Collaborator

ffreyer commented Jul 19, 2023

Oh yea, latest commit will need a texture atlas update

@SimonDanisch
Copy link
Member Author

Thanks, looks good! :)
There's still a bit of movement in the reference image tests, but at this point it's hard to tell what's even more correct.
Can't approve my own PR, but I think this is ready?

@SimonDanisch SimonDanisch merged commit f7635a5 into master Jul 20, 2023
13 checks passed
@SimonDanisch SimonDanisch deleted the sd/hexbin-markersize branch July 20, 2023 13:02
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