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 closing of GLMakie windows on Mac #2553

Merged
merged 3 commits into from
Jan 10, 2023
Merged

Conversation

jkrumbiegel
Copy link
Member

@jkrumbiegel jkrumbiegel commented Jan 4, 2023

  • actually close the window when it's supposed to be closed
  • add unrelated missing null check found in the course of debugging

Description

Fixes #393, fixes #482, fixes #658, fixes #682

I always suspected that windows not closing were an issue of glfw on Mac, given that it always seemed to work ok on Linux and Windows. There actually is a problem with glfw as it was explained to me here glfw/glfw#1766 (comment) but following down that trail I noticed that it is a separate issue from windows not disappearing at all. The stuck process, which is really what the glfw issue is about, is not so bad for user experience, because it doesn't get in the way (unlike stuck windows). So I once more followed the chain of events that happen when the x button is pressed on the window:

  • The glfw ShouldClose callback triggers
  • the scene.events.window_open observable is set to false
  • this triggers a function that removes everything from the scene but doesn't actually call close on it. Why this worked on Linux and Windows I don't know. close does the stuff that actually hides a window..
  • close actually also sets window_open[] = false. To avoid an infinite loop, I put this behind a check. Not sure if this would actually need to be removed, but I'm not sure when else close could be called so it seems better to leave it in.

Hopefully this doesn't conflict with Linux and Windows behavior and settles that annoying issue once and for all.

Type of change

Delete options that do not apply:

  • Bug fix (non-breaking change which fixes an issue)

@MakieBot
Copy link
Collaborator

MakieBot commented Jan 4, 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 31.67s (31.47, 31.81) 0.13+- 16.14s (16.07, 16.31) 0.08+- 14.93s (14.83, 15.26) 0.15+- 11.78ms (11.64, 11.95) 0.12+- 135.00ms (132.36, 136.38) 1.25+-
master 31.73s (31.44, 31.86) 0.15+- 16.13s (15.96, 16.28) 0.10+- 14.88s (14.75, 15.02) 0.09+- 11.72ms (11.52, 11.87) 0.11+- 133.94ms (131.21, 134.90) 1.29+-
evaluation -0.20%, -0.06s invariant (-0.45d, 0.41p, 0.14std) +0.03%, 0.01s invariant (0.06d, 0.92p, 0.09std) +0.36%, 0.05s invariant (0.43d, 0.44p, 0.12std) +0.52%, 0.06ms invariant (0.54d, 0.33p, 0.11std) +0.78%, 1.05ms invariant (0.83d, 0.15p, 1.27std)
CairoMakie 31.31s (30.29, 32.26) 0.64+- 19.00s (18.55, 19.49) 0.37+- 2.91s (2.77, 2.99) 0.07+- 13.07ms (12.46, 13.55) 0.39+- 5.17ms (4.88, 5.36) 0.18+-
master 31.42s (30.44, 32.24) 0.60+- 19.23s (18.78, 20.21) 0.50+- 2.93s (2.85, 3.09) 0.08+- 13.18ms (12.76, 13.90) 0.39+- 5.22ms (4.97, 5.41) 0.15+-
evaluation -0.34%, -0.11s invariant (-0.17d, 0.75p, 0.62std) -1.20%, -0.23s invariant (-0.52d, 0.35p, 0.44std) -0.64%, -0.02s invariant (-0.24d, 0.66p, 0.08std) -0.81%, -0.11ms invariant (-0.27d, 0.62p, 0.39std) -0.98%, -0.05ms invariant (-0.31d, 0.57p, 0.16std)
WGLMakie 43.43s (43.08, 44.09) 0.34+- 19.20s (18.78, 19.68) 0.38+- 29.93s (29.58, 30.36) 0.33+- 21.07ms (20.18, 22.23) 0.79+- 2.59s (2.32, 2.77) 0.15+-
master 43.45s (42.90, 45.15) 0.78+- 18.84s (18.56, 19.16) 0.21+- 29.65s (29.14, 30.05) 0.29+- 20.47ms (19.07, 21.71) 0.86+- 2.55s (2.38, 2.73) 0.12+-
evaluation -0.05%, -0.02s invariant (-0.03d, 0.95p, 0.56std) +1.88%, 0.36s invariant (1.19d, 0.05p, 0.29std) +0.93%, 0.28s invariant (0.88d, 0.12p, 0.31std) +2.87%, 0.6ms invariant (0.73d, 0.20p, 0.82std) +1.57%, 0.04s invariant (0.30d, 0.59p, 0.14std)

@jkrumbiegel jkrumbiegel merged commit 9116025 into master Jan 10, 2023
@jkrumbiegel jkrumbiegel deleted the jk/fix-closing-of-window branch January 10, 2023 08:50
jwahlstrand added a commit to JuliaGtk/Gtk4Makie.jl that referenced this pull request Jan 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants