DataInspector rework

[ffreyer]

Description

Related Issues:

• fixes LoadError: Failed to resolve inspector_label on 0.24 #5229
• fixes scatterlines.inspector_label is broken #4713
• closes Persistent Data Tips for Data Exploration and Comparison #962 by adding the feature (though the hotkeys may need some adjustments)
• closes make Checkboxes not inspectable #4706 by restricting DataInspector to one scene each
• closes access to undefined reference with child plots #5383 by avoiding the need to set inspector_label for child plots

Goals

This aims to:

• fix DataInspector not always considering inspector_label (etc) of the top level plot (Recipes do not react to keyword arguments properly #3263, Histogram is now missing the inspector_label attribute #4440, scatterlines.inspector_label is broken #4713)
• add persistent tooltips (Persistent Data Tips for Data Exploration and Comparison #962)
• make DataInspector per-scene (make Checkboxes not inspectable #4706)
• (probably) fix issues with labels being post-transform (DataInspector still shows transformed coordinated for log scale, hline #3526, DataInspector does not work for lines in log10 scale #3571, DataInspector for PolarAxis should display tuples of (r, theta) #4446)
• maybe make DataInspector easier to extend
• may drop indicators

I think I fixed #4866 too, which I should probably extract...

Problems

The previous version of DataInspector worked like this:

• pick() picks a primitive plot, like lines
• DataInspector steps to the root parent of the plot tree lines belongs to
• DataInspector steps back down, calling show_data at each step

As a consequence, inspector_label etc was only accessed for plots that implemented show_data, and would be given that plot. For example scatterlines did not implement a show_data method and thus its inspector_label was not considered. If it were simply passed down, it would be called with either the lines or scatter plot and a position and index relevant to that plot. This is problematic for more complex recipes where you may want the recipes attributes and an appropriate index to generate a label.

Implementing show_data() is probably rather difficult for people, as it does multiple things:

• it may map the (plot, idx) pair generated by pick to something appropriate for the given recipe plot
• it generates a screen/pixel space position (which needs to correctly transform and project)
• it may generate (and clean up) an indicator plot
• it generates a label
• it updates the tooltip

Persistent Tooltips have been difficult to implement because DataInspector worked in a global scene. That forced tooltips to be drawn in pixel space, either by transforming a data space position or by directly using pixel space mouse positions. A persistent tooltip should be stuck somewhere in data space, e.g. anchored to a scattered marker, which means it would need to connect to the camera and project frequently. With show_data doing everything a persistent tooltip would also need to extract relevant information from a dynamic tooltip.

Plan

Plan

DataInspector being a global object has been more painful than useful, so I'm planning to make it per-scene. This will fix the general annoyances with it picking irrelevant plots in other scenes. It will also allow tooltips to exist in data space, as they don't have to work with different cameras anymore, which should simplify persistent tooltips.

I'm also planning to break up show_data into multiple pieces and restructure the infrastructure around it. As I see it, the main tasks of show_data are:

• map pick() data to something appropriate to a high level plot (typically an index or interpolation between indices)
• extract a position from a plot using the processed pick data
• extract a label from a plot using the processed pick data (and position)
• do whatever processing DataInspector needs and update the tooltip
• (generate/update an indicator plot using the processed pick data?)

For the first point I'm planning to add a new PlotElement type generated by a pick_element() function. The PlotElement should encapsulate the indexing or interpolation so that you can get whatever plot attribute you want for the picked element. The pick_element function should generate a PlotElement from pick data, attempting the root parent plot first and progressing down the to the picked primitive like show_data. The resulting PlotElement should always be (considered to be) relevant to the root parent though. This may also be useful in general as a higher level pick(). For example:

# Lines are continuous, so the picked element should represent an
# interpolated point on the line. `element.color` should thus return 
# a float value between 1 and 10
f,a,p = lines(rand(10), color = 1:10, linewidth = 10)
element = pick_element(scene, mouseposition_on_line)
element.color # e.g. 3.7
element.linewidth # constant, 10

# Scatter is discrete, so the picked element should encapsulate an
# index, making element.color an integer
f,a,p = scatter(rand(10), color = 1:10, markersize = 10)
p.color # e.g. 4
p.markersize # constant, 10

With that, getting positions and labels can hopefully be rather simple and generic:

function get_position(element)
    name = get_converted_args_name(parent(element))
    return getproperty(element, name)
end

function get_tooltip_label(element, pos)
    label = element.inspector_label
    if label isa String
        return Format.format(label, pos...)
    elseif label isa Function
        return label(element)
    elseif label === automatic
        return default_label(element)
    end
end

For persistent tooltips my current idea is to allow passing a PlotElement array to tooltip(). The tooltip plot can then be saved in DataInspector and elements can be added or removed. Since different plots can have different transformations, there needs to be a tooltip plot for each parent plot. To allow tooltips to update with changes to plot attributes we need some sort of map!(..., parent_attributes, ...). I'm currently thinking of tracking which attributes are used when calling get_position() and get_tooltip_label() to collect the necessary attributes.

Changes

DataInspector is now per scene. This simplifies persistent tooltips and using pre-transform/projection data because DataInspector no longer has to deal with multiple scenes. This also stops DataInspector from picking plots outside its designated scene, which was generally more of a hassle.

The whole structure of how data inspector gets from a picked plot to a tooltip + indicators has been reworked. Previously there were two parts. First a generic part, which traced the picked plot up to its root parent and dispatched calls to show_data. If no fitting method exists, the function would try again with the next lower level plot. The second part is the show_data method which did everything else, including providing an extension interface for recipe developers.
The new structure breaks up show_data to better separate concerns and through that simplify extension (I hope). It also reworks the fallback logic in a way that the plot trace doesn't disappear, allowing each step to do fallbacks independently. The steps now include:

1. (internal) tracing of the picked plot to the root recipe
2. (extendable) get_accessor(plot, idx, trace) methods for creating a AbstractElementAccessor which acts as an index or interpolation information for accessing the picked element of a plot
3. (internal) creation of a PlotElement which bundles the trace with the accessor returned by get_accessor()
4. (internal) manual search for an applicable get_position(element) method which successively removes parent plots from the trace in the PlotElement. This is used to identify the plot from which the position is pulled so that the appropriate transformations and space can be copied.
5. (extendable) position = get_position(::PlotElement{PlotType}) which gets the picked position
6. (internal) get_tooltip_label(formatter, ::PlotElement{PlotType}, position) which produces the tooltip label string
   • (extendable) get_default_tooltip_label(formatter, ::PlotElement{PlotType}, position which produces the string if no attribute based string exists
     • (extendable) get_tooltip_data(::PlotElement{PlotType}, position) which returns data to be converted to a string (defaults to position) for the label
7. (internal) update_indicator_internal!(inspector, ::PlotElement{PlotType}, position) which updates transformations and space of an updated indicator
   • (extendable) update_indicator!(inspector, ::PlotElement{PlotType}, position) which sets the data of an indicator plot and makes it visible
   • (extendable) construct_indicator_plots!(inspector, PlotType) which produces a cached indicator plot for a given plot type. get_indicator_plot(inspector, PlotType) will call this function once to produce this plot and otherwise return the cached version.

Changes to tooltips:

• Mesh currently gives position information instead of boundingbox information
• formatting has been reworked (more consistent, probably shorter)

TODO

• add docstrings
• review online documentation
• switch tests to use new DataInspector
• add more specialized tests
• remove old DataInspector
• improve label construction interface (i.e. how formatting works, shorter function names?)
• re-add missing settings
• figure out how to efficiently and correctly trigger updates (e.g. when updates can be discarded with tick based updates)
• add refimg test for persistent tooltips

General feature testing - correct behavior with previously implemented tooltips, no hard errors in general

• all primtives work (excluding volume)
• all explicitly implemented plots from old DataInspector work (BarPlot, Arrows3D, Arrows2D, Contourf, VolumeSlices, Band, Spy, HeatmapShader, DataShader)
• no plot hard errors (this would crash the renderloop)

Notes

• There are a bunch of plots that may need further changes to their tooltips but I'm sure what those should be:
  • stairs, stephist, ecdfplot should maybe display step values
  • hspan, vspan should maybe display limits
  • qqnorm, qqplot, boxplot, crossbar should probably display some stats-based things
  • stem should probably be discrete and show max heights
  • streamplot should probably display local position + direction
  • dendrogram, triplot, voronoiplot ... not sure what's useful to show here
• barplot is messy and incomplete due to not yet being updated to the compute graph
• discrete heatmap/image picking could be simplified by per scene rendering + rendering indicators without picking (Refactor (GLMakie) render order (experimental) #4150 + Rework postprocessor handling in GLMakie #4689 + WGLMakie versions)
• refimg tests cover everything except
  • some plots I'm unsure about: boxplot, dendrogram, ecdfplot, qqplot, qqnorm, streamplot, triplot, voronoiplot
  • some plots that don't produce tooltips: annotation, bracket, textlabel, tooltip

Type of change

Likely breaking because show_data will most likely be removed in its current state

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• Added an entry in CHANGELOG.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]

Benchmark Results

SHA: dbfe3ff0b56ce62c359969cb11daa713b69ef296

Warning

These results are subject to substantial noise because GitHub's CI runs on shared machines that are not ideally suited for benchmarking.

[ffreyer]

I updated this to include some notes on dim converts. Example from a test failure:

  ArgumentError: 
  Conversion failed for Tooltip with args:
      Tuple{Vector{Makie.PlotElement{Scatter{Tuple{Vector{Point{2, Float32}}}}}}} 
  Got converted to:
      Tuple{Vector{Makie.PlotElement{Scatter{Tuple{Vector{Point{2, Float32}}}}}}}
  Tooltip requires to convert to argument types
      Tuple{AbstractVector{<:Union{NTuple{N, var"#s417"}, StaticArraysCore.StaticArray{Tuple{N}, var"#s417", 1}}} where {N, var"#s417"<:Real}, Union{Nothing, AbstractVector}},
  which convert_arguments didn't succeed in. To fix this overload convert_arguments(P, args...) for Type{<:Tooltip} or NoConversion() and return an object of the correct type.
  (Dim converts were not applied. This happens if `space = data` is not in data space or if the target type of the conversion is reachable without dim converts.)

[ffreyer]

I think I fixed one of two problems with WGLMakie, the one visible in tests from b117521. When multiple graphs are connected it can happen that update!() intercepts resolve!() and corrupts state because of how we lock graphs. With a Graph a connected to a Graph b, I believe the order of actions for this is:

1. resolve!() triggers, locking b
2. resolve!() recurses up, locking a
3. resolve!() evaluates edges in a, unlocking a once done
4. task switches to update!() in a, which locks (or waits for) a
5. update!() replaces the input in a and marks every dependent as dirty. This does not lock/wait for b
6. resolve!() continues, evaluating the remaining edges in b using old values, marking everything it resolves as clean/up-to-date/resolved and finally unlocks b

With this the nodes set in step 6 are marked as clean while they are outdated with respect to update happening in 5. I believe this is what happened in the test - WGLMakie started resolving visible for tooltip plots, DataInspector interjected an update of visible from the top and the state desynchronized.

I changed the locking logic of resolve to not unlock in step 3. Instead it now starts by locking every involved graph first, before doing any computation, and unlocks all of them together once the full resolve is done. This should fix the locking behavior, and locally fixed the leftover plots as far as I could tell.

---

The other issue here is that with WGLMakie some regions just refuse to generate a tooltip. As far as I can tell this happens when the cursor moves to an area that was previously occupied by a tooltip. The pick_sorted call in DataInspector then only sees the tooltip, even though it is set to visible = false. The wgl_renderobject also reports it as invisible and it is not rendered in this case. This happens repeatedly as it triggers on(events.tick). Interestingly calling pick_sorted in the terminal with the same parameters as DataInspector resolves the issue (until it is recreated).

I also checked the javascript objects that picking returns. They also have visible: false (though this is at the end of pick_sorted. I guess asynchronous things could happen between rendering and that point, though I would expect flickering and inconsistencies if visibility toggles and on and off)