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

Sf/vscode resizing #3044

Merged
merged 10 commits into from Jul 11, 2023
Merged

Sf/vscode resizing #3044

merged 10 commits into from Jul 11, 2023

Conversation

SimonDanisch
Copy link
Member

Continue #3042 and make it optional!

staticfloat and others added 3 commits July 3, 2023 15:18
This implements basic resizing support for WGLMakie figures.  This has
primarily been tested in the VSCode plot pane and the JSServe
auto-launched browser window (using Chrome and Safari).

The implementation creates a new `resize` event that the javascript
frontend can send back to the Julia backend, with a computed size (in
pixels) that the backend should resize to.  Additionally, the javascript
`throttle()` function is altered to no longer drop the last few events
when throttled, it instead delays the last event until the end of the
throttling period, which is important when resizing quickly so that the
final size is faithfully recorded.
@MakieBot
Copy link
Collaborator

MakieBot commented Jul 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 10.65s (10.60, 10.71) 0.05+- 1.03s (1.01, 1.05) 0.02+- 775.80ms (760.36, 820.24) 20.24+- 9.71ms (9.66, 9.81) 0.06+- 86.27ms (85.74, 86.74) 0.39+-
master 10.64s (10.58, 10.69) 0.04+- 1.05s (1.03, 1.07) 0.01+- 767.43ms (758.96, 775.59) 5.22+- 9.66ms (9.57, 9.70) 0.04+- 86.44ms (85.87, 87.17) 0.42+-
evaluation +0.12%, 0.01s invariant (0.29d, 0.60p, 0.04std) -1.40%, -0.01s invariant (-1.05d, 0.08p, 0.01std) +1.08%, 8.37ms invariant (0.57d, 0.33p, 12.73std) +0.60%, 0.06ms invariant (1.11d, 0.06p, 0.05std) -0.19%, -0.16ms invariant (-0.41d, 0.46p, 0.40std)
CairoMakie 10.11s (10.01, 10.24) 0.07+- 1.17s (1.16, 1.17) 0.00+- 232.70ms (230.59, 235.35) 1.74+- 10.36ms (10.19, 10.60) 0.13+- 5.80ms (5.73, 5.93) 0.07+-
master 10.13s (10.02, 10.22) 0.07+- 1.14s (1.12, 1.15) 0.01+- 221.56ms (219.77, 223.37) 1.08+- 10.38ms (10.26, 10.45) 0.07+- 5.92ms (5.84, 6.00) 0.06+-
evaluation -0.28%, -0.03s invariant (-0.39d, 0.48p, 0.07std) +2.48%, 0.03s slower X (3.76d, 0.00p, 0.01std) +4.79%, 11.13ms slower X (7.70d, 0.00p, 1.41std) -0.15%, -0.02ms invariant (-0.15d, 0.78p, 0.10std) -2.20%, -0.13ms faster ✓ (-1.92d, 0.00p, 0.07std)
WGLMakie 15.64s (15.28, 16.12) 0.29+- 1.66s (1.58, 1.70) 0.05+- 14.22s (13.67, 14.56) 0.27+- 18.55ms (17.60, 20.92) 1.15+- 1.48s (1.37, 1.81) 0.15+-
master 15.69s (15.55, 15.83) 0.11+- 1.71s (1.66, 1.77) 0.04+- 14.45s (14.21, 15.21) 0.34+- 18.64ms (16.91, 20.97) 1.52+- 1.45s (1.39, 1.51) 0.04+-
evaluation -0.38%, -0.06s invariant (-0.27d, 0.63p, 0.20std) -3.08%, -0.05s faster ✓ (-1.17d, 0.05p, 0.04std) -1.67%, -0.24s invariant (-0.77d, 0.18p, 0.31std) -0.52%, -0.1ms invariant (-0.07d, 0.90p, 1.34std) +1.83%, 0.03s invariant (0.24d, 0.66p, 0.10std)

SimonDanisch added a commit that referenced this pull request Jul 5, 2023
And test if CI hangs like in #3044
@SimonDanisch SimonDanisch mentioned this pull request Jul 5, 2023
SimonDanisch added a commit that referenced this pull request Jul 5, 2023
And test if CI hangs like in #3044
@staticfloat
Copy link
Contributor

FTR, I think this is approach is fine. I just put WGLMakie.activate!(;resize_to_body=true) at the top of my script, and it all works. Thanks!

@SimonDanisch
Copy link
Member Author

@kbarros, the aspect ratio for LScene should be fixed now! (btw, you need to do Makie.inline!(false) right now for GLMakie to open an interactive window).

@kbarros
Copy link

kbarros commented Jul 10, 2023

I can confirm the aspect ratio bug has been fixed and with this branch, resizing now works as intended!

If I may request a future feature: Can resizing take into account the width of the panel? See for example the two attached VSCode images. Changing the width of the panel will not enlarge or shrink the image size.

image image

This behavior is shared by GLMakie pop-up windows. In GLMakie, it's fine. However, "tall and narrow" panels seem to appear by default in VSCode. The image below shows the behaviors of a Plots.jl image that resizes according to the panel width inside VSCode.

image

Interestingly, the Plots.jl image ignores the panel height (so it's the converse of Makie). I wonder if an ideal behavior is to use something like min(width, height) so scroll bars don't appear? It seems like a tricky design decision.

@staticfloat
Copy link
Contributor

staticfloat commented Jul 10, 2023

I wonder if an ideal behavior is to use something like min(width, height) so scroll bars don't appear? It seems like a tricky design decision.

I agree that locking to the minimum dimension is likely the ideal solution, but should probably be implemented in a follow-on PR.

@kbarros
Copy link

kbarros commented Jul 10, 2023

Also good to keep in mind that Jupyter notebooks tend to have short wide panels -- so we can't only look at the panel width in that use case.

@SimonDanisch
Copy link
Member Author

Actually, I accidently achieved the plots.jl behaviour by using the wrappers div offsetWidth/offsetHeight attributes, instead of using the exact body dimensions.
If that's more desired, we could use that?
That would also simplify the resize_callback code a lot:

  function resize_callback() {
      comm.notify({ resize: [wrapper.offsetWidth, wrapper.offsetHeight] });
  }

@kbarros
Copy link

kbarros commented Jul 10, 2023

Interesting. I'd be happy to test that change in VSCode and Jupyter if you put it on a branch.

@SimonDanisch SimonDanisch mentioned this pull request Jul 10, 2023
@SimonDanisch
Copy link
Member Author

@kbarros: #3056

@SimonDanisch SimonDanisch merged commit 07496e9 into master Jul 11, 2023
14 checks passed
@SimonDanisch SimonDanisch deleted the sf/vscode_resizing branch July 11, 2023 13:30
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

4 participants