Small clean ups (#1717)

* remove overly eager optimizations (#1713)

* small clean ups

* clean up implementation a bit, use only one name for space

* address review

CairoMakie/src/primitives.jl

@@ -198,7 +198,7 @@ function draw_atomic(scene::Scene, screen::CairoScreen, primitive::Scatter)
         markerspace = to_value(get(primitive, :markerspace, :pixel))
         scale = project_scale(scene, markerspace, markersize, size_model)
         offset = project_scale(scene, markerspace, mo, size_model)
-                          
+
         space = to_value(get(primitive, :space, :data))
         pos = project_position(scene, space, point, model)
         isnan(pos) && return
@@ -275,7 +275,7 @@ function draw_marker(ctx, marker::Circle, pos, scale, strokecolor, strokewidth,
         Cairo.scale(ctx, scale[1], scale[2])
         Cairo.translate(ctx, pos[1]/scale[1], pos[2]/scale[2])
         Cairo.arc(ctx, 0, 0, 0.5, 0, 2*pi)
-    else 
+    else
         Cairo.arc(ctx, pos[1], pos[2], scale[1]/2, 0, 2*pi)
     end
 
@@ -323,7 +323,7 @@ function draw_atomic(scene::Scene, screen::CairoScreen, primitive::Text{<:Tuple{
     glyph_collection = to_value(primitive[1])
 
     draw_glyph_collection(
-        scene, ctx, position, glyph_collection, remove_billboard(rotation), 
+        scene, ctx, position, glyph_collection, remove_billboard(rotation),
         model, space, markerspace, offset
     )
 
@@ -332,12 +332,12 @@ end
 
 
 function draw_glyph_collection(
-        scene, ctx, positions, glyph_collections::AbstractArray, rotation, 
+        scene, ctx, positions, glyph_collections::AbstractArray, rotation,
         model::SMatrix, space, markerspace, offset
     )
 
     # TODO: why is the Ref around model necessary? doesn't broadcast_foreach handle staticarrays matrices?
-    broadcast_foreach(positions, glyph_collections, rotation, Ref(model), space, 
+    broadcast_foreach(positions, glyph_collections, rotation, Ref(model), space,
         markerspace, offset) do pos, glayout, ro, mo, sp, msp, off
 
         draw_glyph_collection(scene, ctx, pos, glayout, ro, mo, sp, msp, off)
@@ -568,11 +568,11 @@ function draw_atomic(scene::Scene, screen::CairoScreen, primitive::Union{Heatmap
 
             # Rectangles and polygons that are directly adjacent usually show
             # white lines between them due to anti aliasing. To avoid this we
-            # increase their size slightly. 
+            # increase their size slightly.
 
             if alpha(colors[i, j]) == 1
-                # sign.(p - center) gives the direction in which we need to 
-                # extend the polygon. (Which may change due to rotations in the 
+                # sign.(p - center) gives the direction in which we need to
+                # extend the polygon. (Which may change due to rotations in the
                 # model matrix.) (i!=1) etc is used to avoid increasing the
                 # outer extent of the heatmap.
                 center = 0.25 * (p1 + p2 + p3 + p4)
@@ -673,14 +673,10 @@ function draw_mesh3D(
 
     model = primitive.model[]
     space = to_value(get(primitive, :space, :data))
+
     view = ifelse(is_data_space(space), scene.camera.view[], Mat4f(I))
-    projection = if is_data_space(space) scene.camera.projection[]
-    elseif is_pixel_space(space) scene.camera.pixel_space[]
-    elseif is_relative_space(space) Mat4f(2, 0, 0, 0, 0, 2, 0, 0, 0, 0, 1, 0, -1, -1, 0, 1)
-    elseif is_clip_space(space) Mat4f(I)
-    else error("Space $space not recognized. Must be one of $(spaces())") 
-    end
-    i = SOneTo(3)
+    projection = Makie.space_to_clip(scene.camera, space, false)
+    i = Vec(1, 2, 3)
     normalmatrix = transpose(inv(view[i, i] * model[i, i]))
 
     # Mesh data

GLMakie/src/drawing_primitives.jl

@@ -47,42 +47,35 @@ end
 make_context_current(screen::Screen) = GLFW.MakeContextCurrent(to_native(screen))
 
 function connect_camera!(gl_attributes, cam, space = gl_attributes[:space])
-    # (0, 1) x (0, 1) x (0, 1) space
-    rel = Mat4f(2, 0, 0, 0, 0, 2, 0, 0, 0, 0, 1, 0, -1, -1, 0, 1)
-    # (-1, 1) x (-1, 1) x (0, 1) space 
-    id = Mat4f(I)
-
-    for key in (:pixel_space, :view, :projection, :resolution, :eyeposition, :projectionview)
-        if !haskey(gl_attributes, key)
-            gl_attributes[key] = if key in (:projection, :projectionview)
-                map(getfield(cam, key), getfield(cam, :pixel_space), space) do data, pixel, space
-                    if     is_data_space(space)     return data
-                    elseif is_pixel_space(space)    return pixel
-                    elseif is_relative_space(space) return rel
-                    elseif is_clip_space(space)     return id
-                    else error("Space $space not recognized. Must be one of $(spaces())")
-                    end 
-                end
-            elseif key in (:view, )
-                map(getfield(cam, key), space) do view, space
-                    ifelse(is_data_space(space), view, id)
-                end
-            else # :pixel_space, :resolution, :eyepostion
-                getfield(cam, key)
-            end
+    for key in (:pixel_space, :resolution, :eyeposition)
+        get!(gl_attributes, key, getfield(cam, key))
+    end
+    get!(gl_attributes, :view) do
+        map(cam.view, space) do view, space
+            return is_data_space(space) ? view : Mat4f(I)
         end
     end
-
-    if !haskey(gl_attributes, :normalmatrix)
-        gl_attributes[:normalmatrix] = map(gl_attributes[:view], gl_attributes[:model]) do v, m
-            i = SOneTo(3)
+    get!(gl_attributes, :normalmatrix) do
+        return map(gl_attributes[:view], gl_attributes[:model]) do v, m
+            i = Vec(1, 2, 3)
             return transpose(inv(v[i, i] * m[i, i]))
         end
     end
 
-    haskey(gl_attributes, :space) && delete!(gl_attributes, :space)
-    haskey(gl_attributes, :markerspace) && delete!(gl_attributes, :markerspace)
+    get!(gl_attributes, :projection) do
+        return map(cam.projection, cam.pixel_space, space) do _, _, space
+            return Makie.space_to_clip(cam, space, true)
+        end
+    end
 
+    get!(gl_attributes, :projectionview) do
+        return map(cam.projectionview, cam.pixel_space, space) do _, _, space
+            Makie.space_to_clip(cam, space, true)
+        end
+    end
+
+    delete!(gl_attributes, :space)
+    delete!(gl_attributes, :markerspace)
     return nothing
 end
 
@@ -215,10 +208,10 @@ function draw_atomic(screen::GLScreen, scene::Scene, @nospecialize(x::Union{Scat
         # signals not supported for shading yet
         gl_attributes[:shading] = to_value(get(gl_attributes, :shading, true))
         marker = lift_convert(:marker, pop!(gl_attributes, :marker), x)
-        
+
         positions = handle_view(x[1], gl_attributes)
         positions = apply_transform(transform_func_obs(x), positions)
-        
+
         if isa(x, Scatter)
             space = get(gl_attributes, :space, :data)
             mspace = get(gl_attributes, :markerspace, :pixel)
@@ -328,7 +321,7 @@ function draw_atomic(screen::GLScreen, scene::Scene,
             # the actual, new value gets then taken in the below lift with to_value
             gcollection = Observable(glyphcollection)
         end
-       
+
         # calculate quad metrics
         glyph_data = map(pos, gcollection, offset, transfunc) do pos, gc, offset, transfunc
             Makie.text_quads(pos, to_value(gc), offset, transfunc)
@@ -346,7 +339,7 @@ function draw_atomic(screen::GLScreen, scene::Scene,
             !(k in (
                 :position, :space, :markerspace, :font,
                 :textsize, :rotation, :justification
-            )) # space, 
+            )) # space,
         end
 
         gl_attributes[:color] = lift(glyphcollection) do gc

GLMakie/test/runtests.jl

@@ -69,4 +69,3 @@ missing_refimages_glmakie, scores_glmakie = ReferenceTests.record_comparison(glm
         ReferenceTests.test_comparison(missing_refimages_glmakie, scores_glmakie; threshold = 0.01)
     end
 end
-

NEWS.md

@@ -3,9 +3,6 @@
 ## master
 
 - **Breaking** Added `space` as a generic attribute to switch between data, pixel, relative and clip space for positions. `space` in text has been renamed to `markerspace` because of this. `Pixel` and `SceneSpace` are no longer valid inputs for `space` or `markerspace`.
-
-## v0.16
-
 - **Breaking** Deprecated `mouse_selection(scene)` for `pick(scene)`.
 - **Breaking** Bumped `GridLayoutBase` version to `v0.7`, which introduced offset layouts. Now, indexing into row 0 doesn't create a new row 1, but a new row 0, so that all previous content positions stay the same. This makes building complex layouts order-independent [#1704](https://github.com/JuliaPlots/Makie.jl/pull/1704).
 

ReferenceTests/src/tests/text.jl

@@ -130,7 +130,7 @@ end
         rotation = pi/4:pi/2:7pi/4,
         align = (:left, :center),
         textsize = 30,
-        markerspace = :screen
+        markerspace = :pixel
     )
 
     wireframe!(scene, boundingbox(t2), color = (:red, 0.3))
@@ -160,7 +160,7 @@ end
             rotation = a,
             align = (:left, :center),
             textsize = 30,
-            markerspace = :screen
+            markerspace = :pixel
         )
 
         # these boundingboxes should be invisible because they only enclose the anchor
@@ -207,7 +207,7 @@ end
         position = positions,
         align = (:center, :center),
         textsize = 20,
-        markerspace = :screen,
+        markerspace = :pixel,
         overdraw=false)
     fig
 end

WGLMakie/src/serialization.jl

@@ -314,17 +314,9 @@ function serialize_three(scene::Scene, plot::AbstractPlot)
             return
         end
     end
-
-    mesh[:cam_space] = let 
-        key = haskey(plot, :markerspace) ? (:markerspace) : (:space)
-        space = to_value(get(plot, key, :data))
-        if     is_data_space(space);     :data
-        elseif is_pixel_space(space);    :pixel
-        elseif is_relative_space(space); :relative
-        elseif is_clip_space(space);     :clip
-        else error("Space $space not defined. Must be one of $(spaces())")
-        end
-    end
+
+    key = haskey(plot, :markerspace) ? (:markerspace) : (:space)
+    mesh[:cam_space] = to_value(get(plot, key, :data))
 
     return mesh
 end

docs/examples/plotting_functions/text.md

@@ -11,9 +11,9 @@
 - `transparency::Bool = false` adjusts how the plot deals with transparency. In GLMakie `transparency = true` results in using Order Independent Transparency.
 - `fxaa::Bool = false` adjusts whether the plot is rendered with fxaa (anti-aliasing). Note that text plots already include a different form of anti-aliasing.
 - `inspectable::Bool = true` sets whether this plot should be seen by `DataInspector`.
-- `depth_shift::Float32 = 0f0` adjusts the depth value of a plot after all other transformations, i.e. in clip space, where `0 <= depth <= 1`. This only applies to GLMakie and WGLMakie and can be used to adjust render order (like a tunable overdraw). 
+- `depth_shift::Float32 = 0f0` adjusts the depth value of a plot after all other transformations, i.e. in clip space, where `0 <= depth <= 1`. This only applies to GLMakie and WGLMakie and can be used to adjust render order (like a tunable overdraw).
 - `model::Makie.Mat4f` sets a model matrix for the plot. This replaces adjustments made with `translate!`, `rotate!` and `scale!`.
-- `color` sets the color of the plot. It can be given as a named color `Symbol` or a `Colors.Colorant`. Transparency can be included either directly as an alpha value in the `Colorant` or as an additional float in a tuple `(color, alpha)`. The color can also be set for each character by passing a `Vector` of colors. 
+- `color` sets the color of the plot. It can be given as a named color `Symbol` or a `Colors.Colorant`. Transparency can be included either directly as an alpha value in the `Colorant` or as an additional float in a tuple `(color, alpha)`. The color can also be set for each character by passing a `Vector` of colors.
 - `space::Symbol = :data` sets the transformation space for text positions. See `Makie.spaces()` for possible inputs.
 
 ### Other
@@ -24,16 +24,16 @@
 - `position::Union{Point2f, Point3f} = Point2f(0)` sets an anchor position for text. Can also be a `Vector` of positions.
 - `rotation::Union{Real, Quaternion}` rotates text around the given position.
 - `textsize::Union{Real, Vec2f}` sets the size of each character.
-- `markerspace::Symbol = :screen` sets the space in which `textsize` acts. See `Makie.spaces()` for possible inputs.
+- `markerspace::Symbol = :pixel` sets the space in which `textsize` acts. See `Makie.spaces()` for possible inputs.
 - `strokewidth::Real = 0` sets the width of the outline around a marker.
 - `strokecolor::Union{Symbol, <:Colorant} = :black` sets the color of the outline around a marker.
 - `glowwidth::Real = 0` sets the size of a glow effect around the marker.
 - `glowcolor::Union{Symbol, <:Colorant} = (:black, 0)` sets the color of the glow effect.
 
 
-## Screen space text
+## Pixel space text
 
-By default, text is drawn in screen space (`space = :screen`).
+By default, text is drawn in pixel space (`space = :pixel`).
 The text anchor is given in data coordinates, but the size of the glyphs is independent of data scaling.
 The boundingbox of the text will include every data point or every text anchor point.
 This also means that `autolimits!` might cut off your text, because the glyphs don't have a meaningful size in data coordinates (the size is independent of zoom level), and you have to take some care to manually place the text or set data limits such that it is fully visible.
@@ -143,7 +143,7 @@ scene
 ## Offset
 
 The offset attribute can be used to shift text away from its position.
-This is especially useful with `space = :screen`, for example to place text together with barplots.
+This is especially useful with `space = :pixel`, for example to place text together with barplots.
 You can specify the end of the barplots in data coordinates, and then offset the text a little bit to the left.
 
 \begin{examplefigure}{}

src/camera/projection_math.jl

@@ -311,44 +311,40 @@ function transform(model::Mat4, x::T) where T
 end
 
 # project between different coordinate systems/spaces
-
-function space_to_clip(cam::Camera, space::Symbol)
+function space_to_clip(cam::Camera, space::Symbol, projectionview::Bool=true)
     if is_data_space(space)
-        cam.projectionview[]
+        return projectionview ? cam.projectionview[] : cam.projection[]
     elseif is_pixel_space(space)
-        cam.pixel_space[]
+        return cam.pixel_space[]
     elseif is_relative_space(space)
-        Mat4f(2, 0, 0, 0, 0, 2, 0, 0, 0, 0, 1, 0, -1, -1, 0, 1)
+        return Mat4f(2, 0, 0, 0, 0, 2, 0, 0, 0, 0, 1, 0, -1, -1, 0, 1)
     elseif is_clip_space(space)
-        Mat4f(I)
+        return Mat4f(I)
     else
         error("Space $space not recognized. Must be one of $(spaces())")
     end
 end
 
 function clip_to_space(cam::Camera, space::Symbol)
     if is_data_space(space)
-        inv(cam.projectionview[])
+        return inv(cam.projectionview[])
     elseif is_pixel_space(space)
         w, h = cam.resolution[]
-        Mat4f(0.5w, 0, 0, 0, 0, 0.5h, 0, 0, 0, 0, -10_000, 0, 0.5w, 0.5h, 0, 1) # -10_000
+        return Mat4f(0.5w, 0, 0, 0, 0, 0.5h, 0, 0, 0, 0, -10_000, 0, 0.5w, 0.5h, 0, 1) # -10_000
     elseif is_relative_space(space)
-        Mat4f(0.5, 0, 0, 0, 0, 0.5, 0, 0, 0, 0, 1, 0, 0.5, 0.5, 0, 1)
+        return Mat4f(0.5, 0, 0, 0, 0, 0.5, 0, 0, 0, 0, 1, 0, 0.5, 0.5, 0, 1)
     elseif is_clip_space(space)
-        Mat4f(I)
+        return Mat4f(I)
     else
         error("Space $space not recognized. Must be one of $(spaces())")
     end
 end
 
-
 function project(cam::Camera, input_space::Symbol, output_space::Symbol, pos)
-    if space_matches(input_space, output_space)
-        return to_ndim(Point3f, pos, 0)
-    end
+    input_space === output_space && return to_ndim(Point3f, pos, 0)
     clip_from_input = space_to_clip(cam, input_space)
     output_from_clip = clip_to_space(cam, output_space)
     p4d = to_ndim(Point4f, to_ndim(Point3f, pos, 0), 1)
     transformed = output_from_clip * clip_from_input * p4d
-    return Point3f(transformed[SOneTo(3)] / transformed[4])
+    return Point3f(transformed[Vec(1, 2, 3)] ./ transformed[4])
 end

src/conversions.jl

@@ -740,16 +740,17 @@ convert_attribute(p, ::key"lowclip") = to_color(p)
 convert_attribute(p::Nothing, ::key"lowclip") = p
 convert_attribute(p, ::key"nan_color") = to_color(p)
 
-struct Palette{N}
-   colors::SArray{Tuple{N},RGBA{Float32},1,N}
-   i::Ref{UInt8}
-   Palette(colors) = new{length(colors)}(SVector{length(colors)}(to_color.(colors)), zero(UInt8))
+struct Palette
+   colors::Vector{RGBA{Float32}}
+   i::Ref{Int}
+   Palette(colors) = new(to_color.(colors), zero(Int))
 end
 Palette(name::Union{String, Symbol}, n = 8) = Palette(to_colormap(name, n))
 
-function convert_attribute(p::Palette{N}, ::key"color") where {N}
-    p.i[] = p.i[] == N ? one(UInt8) : p.i[] + one(UInt8)
-    p.colors[p.i[]]
+function convert_attribute(p::Palette, ::key"color")
+    N = length(p.colors)
+    p.i[] = p.i[] == N ? 1 : p.i[] + 1
+    return p.colors[p.i[]]
 end
 
 convert_attribute(c::Colorant, ::key"color") = convert(RGBA{Float32}, c)

src/scenes.jl

@@ -464,6 +464,7 @@ update_cam!(scene::Scene, bb::AbstractCamera, rect) = nothing
 function not_in_data_space(p)
     !is_data_space(to_value(get(p, :space, :data)))
 end
+
 function center!(scene::Scene, padding=0.01, exclude = not_in_data_space)
     bb = boundingbox(scene, exclude)
     bb = transformationmatrix(scene)[] * bb

src/units.jl

@@ -127,14 +127,8 @@ export px
 
 ########################################
 
-spaces() = (:data, :world, :pixel, :screen, :relative, :unit, :clip)
-is_data_space(space) = space in (:data, :world)
-is_pixel_space(space) = space in (:pixel, :screen)
-is_relative_space(space) = space in (:relative, :unit)
-is_clip_space(space) = space in (:clip, )
-function space_matches(a, b)
-    (is_data_space(a) && is_data_space(b)) ||
-    (is_pixel_space(a) && is_pixel_space(b)) ||
-    (is_relative_space(a) && is_relative_space(b)) ||
-    (is_clip_space(a) && is_clip_space(b))
-end
+spaces() = (:data, :pixel, :relative, :clip)
+is_data_space(space) = space === :data
+is_pixel_space(space) = space === :pixel
+is_relative_space(space) = space === :relative
+is_clip_space(space) = space === :clip