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 transformation handling in DataInspector #3002

Merged
merged 22 commits into from Jul 3, 2023

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Jun 12, 2023

Description

Fixes #2662

Working with translation via model matrix / translate!:

  • lines
  • scatter
  • heatmap
  • image
  • mesh
  • meshscatter
  • surface
  • barplot
  • arrows
  • contourf
  • volumeslices
  • band

Working with transform_func (halfing each coordinates):

  • lines
  • scatter
  • heatmap
  • mesh (due to boundingbox() ignoring transform_func)
  • meshscatter
  • surface (triangles can't be found in some regions after trans_func application...?)
  • image
  • barplot
  • arrows (tooltip content wrong, but arrows transform weirdly anyway so I'm skipping this)
  • contourf
  • volumeslices
  • band

Note that this pulls changes relevant to DataInspector from #2746 and makes some further changes, so there will be conflicts there. (One thing being the transform application being moved to get_position.)

Since this pulls changes from #2746 these also apply here:

  • fixed error related to getindex_sv(::Range, idx)
  • fixed blinking in image inspection
  • fixed typos in arrows (closes callback error in arrows data inspection #2764) & lines(egments)
  • fixed incorrect values when inspecting volumeslices
  • fixed incorrect indicator placement in lines and added workaround to avoid multiple indicators

Type of change

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

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.

@MakieBot
Copy link
Collaborator

MakieBot commented Jun 12, 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 11.56s (11.42, 11.86) 0.15+- 1.02s (1.01, 1.03) 0.01+- 768.33ms (758.22, 810.78) 18.83+- 10.28ms (10.03, 10.43) 0.14+- 86.63ms (86.02, 87.13) 0.37+-
master 11.52s (11.45, 11.60) 0.06+- 1.02s (1.02, 1.03) 0.00+- 756.77ms (741.24, 762.81) 7.26+- 10.34ms (10.18, 10.40) 0.08+- 87.24ms (86.57, 87.86) 0.47+-
evaluation +0.30%, 0.03s invariant (0.30d, 0.59p, 0.11std) -0.71%, -0.01s faster ✓ (-1.43d, 0.02p, 0.01std) +1.50%, 11.56ms invariant (0.81d, 0.17p, 13.05std) -0.54%, -0.06ms invariant (-0.49d, 0.38p, 0.11std) -0.71%, -0.61ms faster ✓ (-1.44d, 0.02p, 0.42std)
CairoMakie 12.38s (12.08, 12.60) 0.18+- 1.41s (1.34, 1.46) 0.04+- 313.63ms (303.24, 332.51) 10.03+- 12.36ms (12.14, 12.87) 0.27+- 6.89ms (6.74, 7.04) 0.12+-
master 12.36s (12.21, 12.59) 0.15+- 1.40s (1.38, 1.42) 0.02+- 297.69ms (291.06, 305.63) 5.71+- 12.45ms (12.20, 12.85) 0.22+- 7.00ms (6.76, 7.21) 0.16+-
evaluation +0.12%, 0.01s invariant (0.09d, 0.87p, 0.17std) +0.72%, 0.01s invariant (0.33d, 0.56p, 0.03std) +5.08%, 15.94ms slower❌ (1.95d, 0.00p, 7.87std) -0.75%, -0.09ms invariant (-0.38d, 0.49p, 0.24std) -1.59%, -0.11ms invariant (-0.77d, 0.18p, 0.14std)
WGLMakie 14.54s (14.35, 14.72) 0.14+- 1.57s (1.52, 1.64) 0.05+- 14.27s (14.17, 14.44) 0.09+- 19.08ms (17.25, 25.59) 2.92+- 1026.48ms (958.94, 1240.53) 98.52+-
master 14.66s (14.41, 14.82) 0.14+- 1.60s (1.58, 1.66) 0.03+- 14.31s (14.19, 14.57) 0.15+- 18.19ms (17.42, 18.93) 0.66+- 1017.34ms (994.98, 1043.78) 19.07+-
evaluation -0.84%, -0.12s invariant (-0.87d, 0.13p, 0.14std) -2.08%, -0.03s invariant (-0.84d, 0.15p, 0.04std) -0.29%, -0.04s invariant (-0.33d, 0.54p, 0.12std) +4.69%, 0.9ms invariant (0.42d, 0.46p, 1.79std) +0.89%, 9.14ms invariant (0.13d, 0.82p, 58.79std)

@ffreyer ffreyer mentioned this pull request Jun 14, 2023
@ffreyer ffreyer marked this pull request as ready for review June 14, 2023 20:39
@ffreyer
Copy link
Collaborator Author

ffreyer commented Jun 15, 2023

Should we export the position_on_plot functions? They should be a useful alternative for mouseposition() in 3D

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jun 15, 2023

Small example for ray_assisted_pick:

using GLMakie, FileIO
m = FileIO.load(Makie.assetpath("brain.stl"))
N = length(Makie.coordinates(m))

f, a, p = mesh(m, color = 1:N, colormap = :viridis)

ls = Observable([Point3f(NaN), Point3f(NaN)])
p  = Observable(Point3f(NaN))
scatterlines!(a, ls, color = [:red, :orange], depth_shift = -1f-3)
scatter!(a, p, color = :black, depth_shift = -1f-3)

on(events(a).keyboardbutton) do e
    if e.key == Keyboard.space && e.action == Keyboard.release
        ray = Makie.ray_at_cursor(a)
        plot, idx, pos = Makie.ray_assisted_pick(a)
        p[] = pos
        ls[] = [pos - 100 * ray.direction, pos + 200 * ray.direction]
        # ls[] = [ray.origin, pos + 200 * ray.direction]
        # ls[] = [ray.origin, ray.origin + 500 * ray.direction]
    end
end

f

This picks where you cursor is when you press space and draws the ray and the picked position. After rotating a bit:

Screenshot from 2023-06-15 15-13-43

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jun 27, 2023

I added some tests for the ray intersections. I might try to add some more for the position_on_plot functions, but those would be rather cumbersome and fragile since they rely on camera positioning and involve simulating pick()...

bbox = boundingbox(plot)
# Manual boundingbox including transfunc
bbox = let
points = point_iterator(plot)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we also defined this in GeoMakie, maybe time to give this its own function!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea but I don't want to do that here. I started working on #3013 and wanted to do #2881 at some point after that.

else
tt.text[] = color2text(name, x, y, z)
i, j, z = _pixelated_getindex(plot[1][], plot[2][], plot[3][], pos, edge_based)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nearest_getindex? Also why the _?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to add a _ prefix to functions that seem very specific/internal to me. For this and _interpolated_getindex I thought they don't really make sense outside the specific context of show_data/DataInspector

@SimonDanisch SimonDanisch merged commit ea54d0f into master Jul 3, 2023
14 checks passed
@SimonDanisch SimonDanisch deleted the ff/DataInspector_transform branch July 3, 2023 14:05
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.

callback error in arrows data inspection DataInspector tooltips don't move with transformed elements
3 participants