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

Refactor display code to be Screen centric #2306

Merged
merged 71 commits into from
Oct 12, 2022
Merged

Conversation

SimonDanisch
Copy link
Member

@SimonDanisch SimonDanisch commented Sep 27, 2022

Loosely a continuation of #2012, but really Rabbit holed into a refactor of the display code.
Screen is now the holder of all display related information like device_scaling_factor, desired output type and things like visibility

This PR adds:

  • saving/recording with backend keyword
  • better and consistent screen constructors between backends, with a clear purpose of what they do
  • cleaned up display code and more consistent use of display
  • removed backend_display + inline! since they're somewhat redundant at this point
  • CairoMakie opens plots in OS image viewer/browser

..And some more docs and clean ups to come

@MakieBot
Copy link
Collaborator

MakieBot commented Sep 27, 2022

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 29.79s (29.53, 30.17) 0.22+- 18.55s (18.46, 18.67) 0.07+- 16.53s (16.38, 16.67) 0.12+- 17.68ms (17.34, 17.88) 0.21+- 50.10ms (49.55, 50.49) 0.37+-
master 29.52s (29.27, 29.80) 0.18+- 18.54s (18.41, 18.73) 0.10+- 17.21s (17.07, 17.40) 0.13+- 16.42ms (15.96, 16.71) 0.25+- 142.63ms (141.12, 144.74) 1.44+-
evaluation +0.91%, 0.27s slower X (1.35d, 0.03p, 0.20std) +0.05%, 0.01s invariant (0.10d, 0.86p, 0.09std) -4.13%, -0.68s faster ✓ (-5.52d, 0.00p, 0.12std) +7.12%, 1.26ms slower❌ (5.41d, 0.00p, 0.23std) -184.68%, -92.53ms faster✅ (-87.84d, 0.00p, 0.91std)
CairoMakie 31.45s (31.28, 31.59) 0.09+- 22.78s (22.53, 23.09) 0.17+- 3.16s (3.11, 3.21) 0.03+- 22.50ms (22.03, 22.87) 0.27+- 27.65ms (27.27, 27.98) 0.27+-
master 30.78s (30.68, 30.88) 0.07+- 22.71s (22.51, 22.82) 0.12+- 3.66s (3.59, 3.71) 0.04+- 20.26ms (19.73, 20.71) 0.31+- 29.78ms (29.66, 29.92) 0.09+-
evaluation +2.11%, 0.66s slower X (7.98d, 0.00p, 0.08std) +0.32%, 0.07s invariant (0.49d, 0.38p, 0.15std) -15.98%, -0.5s faster✅ (-13.81d, 0.00p, 0.04std) +9.96%, 2.24ms slower❌ (7.68d, 0.00p, 0.29std) -7.72%, -2.13ms faster✅ (-10.48d, 0.00p, 0.18std)
WGLMakie 33.49s (32.61, 34.03) 0.45+- 30.38s (29.54, 30.89) 0.50+- 44.47s (43.88, 45.26) 0.48+- 26.04ms (23.44, 37.10) 4.91+- 83.45ms (73.12, 92.21) 7.60+-
master 33.46s (33.02, 33.78) 0.27+- 29.48s (29.13, 30.00) 0.31+- 45.32s (44.90, 45.86) 0.38+- 21.77ms (21.30, 22.11) 0.29+- 1.74s (1.70, 1.77) 0.03+-
evaluation +0.10%, 0.03s invariant (0.09d, 0.87p, 0.36std) +2.96%, 0.9s slower X (2.16d, 0.00p, 0.41std) -1.91%, -0.85s faster ✓ (-1.96d, 0.00p, 0.43std) +16.40%, 4.27ms noisy🤷‍♀️ (1.23d, 0.06p, 2.60std) -1979.27%, -1651.69ms faster✅ (-83.79d, 0.00p, 17.21std)

@github-actions
Copy link
Contributor

Missing reference images

Found 5 new images without existing references.
Upload new reference images before merging this PR.

@github-actions
Copy link
Contributor

Missing reference images

Found 1 new images without existing references.
Upload new reference images before merging this PR.

GLMakie/src/screen.jl Outdated Show resolved Hide resolved
@SimonDanisch
Copy link
Member Author

SimonDanisch commented Oct 12, 2022

Final summary of changes:

GLMakie Bug fixes

  • Fix Heisenbug for multiple windows, where shaders got compiled for the wrong window (OpenGL context), which works fine on most nvidia hardware but may crash other OpenGL implementations.
  • Due to the above fix, it became clear that the scene clean up after displaying on a screen wasn't happening correctly, which got fixed
  • SSAO/FXAA/transparency_weight_scale were a global, which was fine until we got multi window support. Now, ssao/fxaa/oit are screen options, which you can set via GLMakie.activate!(ssa=true) or per screen via GLMakie.Screen(ssa=true).

Saving + record

  • new backend argument, allowing one to save a file with a specific backend (save("plot.png", figure; backend=GLMakie)). Applies also to record, display and colorbuffer.
  • record, display, colorbuffer and save now support forwarding all Backend.Screen options, which enables you to use record(...; px_per_unit=2.0) for higher resolution videos with CairoMakie, or e.g. save(...; backend=GLMakie, ssao=true).
  • All backend screen types have been renamed to Backend.Screen, which was previously not the case. This shouldn't break too much code, since the only backend where one was interacting with Screen is GLMakie, which was already named Screen. In the future, it will be more common to interact with Screen directly for all backends. Use ?Backend.Screen to see all screen options.

Speed and allocation improvements for record

We gained some substantial speed improvements for record, by reusing more screen state and directly writing out to a file via ffmpeg #2231.

GLMakie

PR:     171.387 ms (57775 allocations: 4.93 MiB)
master: 322.244 ms (50714 allocations: 32.58 MiB)

CairoMakie

PR:     647.093 ms (3076666 allocations: 174.11 MiB)
master: 807.175 ms (3070484 allocations: 335.48 MiB)

WGLMakie

PR:     1.450 s (2200125 allocations: 189.61 MiB)
master: 1.628 s (2192751 allocations: 279.25 MiB)

Other

  • adding more tests for display / record
  • CairoMakie now opens a browser/os-image-viewer when plotpane is disabled or display(fig) is called explicitely.
  • deprecation of Makie.inline!(false/true) which was only disabling show, which isn't very helpful (can just be done by fig; or return nothing) and was instead making trouble accidantely disabling show.
  • deprecating GLMakie.set_window_config!(; screen_config...) in favor of GLMakie.activate!(; screen_config...).
  • deprecating internal function backend_display in favor of display.
  • cleaning up colorbuffer(figlike) and colorbuffer(screen), making it faster and more consistent across backends.
  • clean up of unused code and making code more consistent between backends
  • backend is now registered as module not it's own GLMakieBackend struct. Deprecated internal global current_backend in favor of CURRENT_BACKEND and renaming register_backend! to set_active_backend!. One can now query the current backend via Makie.current_backend()::Module
  • Backend.activate!(; screen_config...) now always accepts attributes for the backends screen config, and writes it to the theme. Alternatively, one can use set_theme!(Backend=(screen_config...,))
  • better warnings for wrong ffmpeg flags in record/Record

@SimonDanisch SimonDanisch merged commit 7b8e881 into master Oct 12, 2022
@SimonDanisch SimonDanisch deleted the sd/display-refactor branch October 12, 2022 11:07
@github-actions
Copy link
Contributor

Missing reference images

Found 1 new images without existing references.
Upload new reference images before merging this PR.

t-bltg pushed a commit to t-bltg/Makie.jl that referenced this pull request Dec 31, 2022
* Only rasterize if the backend is not itself raster

* Define width of CairoScreen

* Make colorbuffer use the size of the screen, not the scene

This allows arbitrary scaling in CairoMakie, remains constant in GLMakie, and probably breaks WGLMakie.

* Add GeometryBasics.widths for ThreeDisplay

* Add GeometryBasics.widths for WebDisplay

* Fix inadvertent bug

* Add news entry

* Decrease the number of evaljs_value calls

* broadcast in WGLMakie method

* Pass kwargs along to backend_show in backend_display for CairoMakie

* Pass arbitrary kwargs along

* convert Iterators.Pairs to namedtuple in backend_display

for CairoMakie

* Splat kwargs as a vector of pairs

* refactor display code

* new screen constructors

* get CairoMakie & GLMakie to work

* fix tests

* more context switching

* try thiss

* hm

* make sure we track context in shader compilation

* clean up ROBJ and context switching

* fix RPRMakie

* fix thy backends

* fix stepper and docs

* remove all inline! use

* fix GLMakie screen deregistering

* fix offset in Stepper for CairoMakie

* fix benchmarks + precompiles

* implement switching of mimes

* move display config to theme

* fix parse error

* bring back precompiles

* small fixes/improvements

* incorperate changes from MakieOrg#2231 and refactor scene

Co-authored-by: Robert Bennett <rltbennett@icloud.com>

* fix CI ?

* improved screen clean up

* clean up and tests

* add logging

* fix makie unit tests

* let CI fail gracefully

* fix import

* remove last GLMakie.Screen related globals

* small improvements for Pluto + friends

* add comment

* clean up docs

* move preferred mime machinery to CairoMakie, since its not needed anywhere else

* remove set_preferred_mime! from tests etc

* fix mime test

* last clean up & address review

* update docs

* initialize array

* deprecate set_window_config! properly

* forgot another constructor

* small clean up and doc fixes

* small doc fixes

Co-authored-by: Anshul Singhvi <anshulsinghvi@gmail.com>
Co-authored-by: Anshul Singhvi <asinghvi17@simons-rock.edu>
Co-authored-by: Robert Bennett <rltbennett@icloud.com>
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

5 participants