Makie.jl icon indicating copy to clipboard operation
Makie.jl copied to clipboard

Rework projection code

Open ffreyer opened this issue 2 years ago • 3 comments

Since this pr is shaping out to be breaking I'm planning on handling #3013, #2881 and #1730 here.

Description

This pr has two main goals. The first is to reduce the amount of (more or less) redundant code dealing with projecting positions. The second is to add missing space options. The pipeline of coordinate spaces/transformations we have looks like this

spaces

where transformed, world and eye space are not accessible through space. (See also #2766) These are added here.

Something I'm not sure about yet is whether pixel and relative space should ignore Transformations. It might be a bit weird that things like scale!() or polar transforms don't work with them. If we do allow them to affect pixel and relative space it might make sense to separate them completely from space. Otherwise naming will get confusing I think. We should also disallow transform propagation if spaces don't match.

I'm also considering restarting #1730 here, as it may change how some things need to work.

Closes #3013 Closes #2766 since this also implements transformed space.

TODO

  • [x] Rework project to be more useful. It now includes model and transform_func depending on input space and can deal with vector inputs.
  • [x] Add projection_obs to simplify projecting data in observables
  • [x] add:transformed, :world and :eye space to make every coordinate space accessible
  • [x] Remove redundant code from Makie
  • [x] Update GLMakie
  • [x] Rework CairoMakies projection code
  • [x] Update WGLMakie
  • [ ] Update RPRMakie
  • [ ] fix axis scale problem (bracket causes error due to late update?)
  • [ ] figure out how to deal with transformations vs camera projections (both controlled via space or just projections)
  • [ ] split spaces
  • [ ] update bounding boxes with new project code
  • [ ] consider doing #2881 here
  • [ ] Documentation
    • new page
    • docstrings, e.g. hvlines, project functions, ...
  • [ ] Cleanup of old code and notes

Other changes

  • fixed duplicate update issues with projectionview
  • allow transform_func to be passed directly as a plot kwarg
  • remove transformation and transform_func from plot attributes on construction

Type of change

  • [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)

While project may or may not be internal, I think it is fairly likely to be used outside in similar situations as errorbars, h/vlines, h/vspan, bracket, etc. (I.e. whether part of or the full plot needs to be in pixel space.)

Checklist

  • [ ] Added an entry in NEWS.md (for new features and breaking changes)
  • [ ] Added or changed relevant sections in the documentation
  • [ ] Added unit tests for new algorithms, conversion methods, etc.
  • [ ] Added reference image tests for new plotting functions, recipes, visual options, etc.

ffreyer avatar Jul 02 '23 16:07 ffreyer

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 14.13s (13.86, 14.34) 0.15+- 1.44s (1.38, 1.47) 0.03+- 861.90ms (822.69, 911.27) 28.17+- 12.64ms (12.54, 12.73) 0.05+- 165.63ms (163.12, 166.49) 1.16+-
master 14.05s (13.80, 14.31) 0.17+- 1.49s (1.44, 1.55) 0.04+- 865.02ms (841.99, 899.06) 20.07+- 12.65ms (12.48, 12.91) 0.15+- 166.40ms (164.53, 169.52) 1.78+-
evaluation +0.59%, 0.08s invariant (0.52d, 0.35p, 0.16std) -3.12%, -0.04s faster ✓ (-1.19d, 0.05p, 0.04std) -0.36%, -3.12ms invariant (-0.13d, 0.82p, 24.12std) -0.12%, -0.01ms invariant (-0.13d, 0.82p, 0.10std) -0.46%, -0.77ms invariant (-0.51d, 0.36p, 1.47std)
CairoMakie 10.74s (10.61, 10.82) 0.07+- 1.26s (1.24, 1.27) 0.01+- 255.34ms (253.20, 256.87) 1.52+- 10.73ms (10.57, 10.93) 0.12+- 6.37ms (6.30, 6.46) 0.05+-
master 10.89s (10.79, 11.00) 0.06+- 1.25s (1.23, 1.26) 0.01+- 229.42ms (224.54, 232.97) 3.04+- 10.68ms (10.51, 10.80) 0.10+- 4.55ms (4.51, 4.58) 0.02+-
evaluation -1.41%, -0.15s faster ✓ (-2.31d, 0.00p, 0.07std) +0.70%, 0.01s invariant (0.88d, 0.13p, 0.01std) +10.15%, 25.92ms slower❌ (10.78d, 0.00p, 2.28std) +0.48%, 0.05ms invariant (0.47d, 0.39p, 0.11std) +28.59%, 1.82ms slower❌ (44.14d, 0.00p, 0.04std)
WGLMakie 16.42s (16.13, 16.65) 0.19+- 1.69s (1.63, 1.86) 0.08+- 14.66s (14.47, 14.95) 0.17+- 18.44ms (17.13, 20.58) 1.17+- 1.42s (1.38, 1.48) 0.03+-
master 16.35s (16.14, 16.56) 0.12+- 1.66s (1.58, 1.74) 0.05+- 14.76s (14.33, 15.28) 0.29+- 18.17ms (17.01, 19.49) 0.97+- 1.42s (1.37, 1.46) 0.04+-
evaluation +0.44%, 0.07s invariant (0.45d, 0.41p, 0.16std) +1.44%, 0.02s invariant (0.35d, 0.52p, 0.07std) -0.64%, -0.09s invariant (-0.40d, 0.47p, 0.23std) +1.44%, 0.27ms invariant (0.25d, 0.65p, 1.07std) +0.65%, 0.01s invariant (0.26d, 0.64p, 0.04std)

MakieBot avatar Jul 02 '23 17:07 MakieBot

I've been thinking about whether Transformations should be part of what space controls or not, and I'm leaning towards separating them.

Conceptually I see Transformation, i.e. the transform_func, translate!, rotate! and scale! (or the model matrix) as transformations of the input data, and transformations derived from :world, :eye, :pixel, :relative and :clip space as transformations of the "world". In other words Transformation describes how data is placed in the world and the latter group describes the coordinate system of the world (or how the world maps to something displayable).

In practice it seems reasonable to me that any plot can be scaled, translated, rotated or even have a nonllinear transform_func like a polar transform before being placed into a world, even if that world is in pixel space or clip space. (This is also the way it works if we use a different camera rather than adjusting space.)

Detaching space and Transformations would allow you to define a pixel space box for example, and use translate!() it to where it needs to be without directly updating the box. This might be useful when updating that box involves expensive computations. Another example would a polar transform in pixel or relative space. You could effectively change the units of the radius with this.

Changes I'm thinking about making:

  • add transform_func as a kwarg to plot! (which gets moved to Transformation, not attributes)
  • maybe model decomposition into rotation, translation, scale so that it can be absorbed into Transformation (only static model matrices)
  • transform = :full / :linear / :nonlinear / :none as an attribute for controlling transform_func and model application (transformation is used for Transformation)
  • maybe detach Transformation from parents if spaces do not match

ffreyer avatar Jul 07 '23 14:07 ffreyer

With higher test accuracy this fails https://github.com/MakieOrg/Makie.jl/blob/83fc003e775c9468aa362a15b45b9e8d675234c6/ReferenceTests/src/tests/examples3d.jl#L461-L472 because the axis limits have become a bit larger, changing the generated grid. I assume this happens because the data_limits of the contour plot changed to include the z-translation it has. Not sure what caused this specifically, but I think this is something we probably want anyway.

ffreyer avatar Jul 08 '23 12:07 ffreyer