Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

New API for Plot Objects #725

Closed
jkrumbiegel opened this issue Oct 15, 2020 · 11 comments
Closed

New API for Plot Objects #725

jkrumbiegel opened this issue Oct 15, 2020 · 11 comments
Labels
Collection contains multiple issues enhancement Feature requests and enhancements Makie Backend independent issues (Makie core) moved Note for projects (Issue moved to Discussions) planning For discussion and planning development

Comments

@jkrumbiegel
Copy link
Member

Current problems

  • Combined and PlotSpec structure is confusing and probably too complex for what it does
  • Plot objects can't be instantiated without giving a scene
    • This causes problems when pre-assembling plots without a preexisting scene, which limits our API options because scenes have to be passed to everything
  • Plot objects can be too "entangled" with the scene they're in, for example if they lift on scene properties like errorbars does, because there is no API to disconnect those connections and reconnect them when moving the plot object to a different scene

API wishlist

# instantiate without scene, observable structure must internally be correctly created
# and just waiting for connection to scene
scatterobject = Scatter(x, y)

# add to scene and connect all relevant observables
push!(scene, scatterobject)

# remove from scene and disconnect all relevant observables
delete!(scene, scatterobject)

# re-add a removed plot object to a scene without problems
push!(scene, scatterobject)
delete!(scene, scatterobject)
push!(other_scene, scatterobject)

# remove a plot object from its parent without specifying it
push!(scene, scatterobject)
delete!(scatterobject)

# plot object can only have one parent, pushing to another disconnects from the first one
push!(scene, scatterobject)
push!(other_scene, scatterobject) # implicit delete!(scatterobject)

# it should not matter if a plot object's parent is a scene or another plot object
push!(my_plot_object, scatterobject)

Questions

  • What observables does every plot object need from a scene? All of them? How to most sensibly store, connect and disconnect them?
  • How do the backends factor into this, for example with GPU state etc?
  • How to unify Combined and PlotSpec into one?
@jkrumbiegel
Copy link
Member Author

So each scene has

  • events
  • camera
  • camera_controls
  • data_limits
  • transformation
  • Attributes

in terms of updateable fields that a plot object might care about. Which ones of these should be mirrored on the plot object side so that the internal logic can be set up without a scene being present?

@SimonDanisch
Copy link
Member

I should add to the wishlist:

  • we want to fix the attributes a plot can contain, to give better errors
  • we want an easy way to get an attribute as a plain value, converted, and as an observable
  • I think it was a mistake, to push Plots to other plots. What we want here is to have a separate plot type, that can combine plots - I guess we could call it PlotCollection, or CombinedPlot :P This would give this a much saner design, instead of everything being a combined plot!
  • it should be possible to return Plot objects from convert_arguments

If we disconnect the Scene more from the plots, we can also think about factoring out the events from scene.
That was a bit ill designed as well, since they're really just window events, and there should just be one per window. We can hook the events up with the plots, when a scene with plots is added to a scene!

@jkrumbiegel
Copy link
Member Author

Oh and another thing, in my Text redesign I needed a place to store computed layouts, and there was no obvious option for me to store it except in an attribute. Which is really ugly (it was even an _underscore attribute name)

@ffreyer
Copy link
Collaborator

ffreyer commented Oct 18, 2020

How do the backends factor into this, for example with GPU state etc?

SSAO attributes (not ssao=true) and backgroundcolor are strictly per Scene.

And now that Simon mentioned events - being able to lock/consume events would be nice. For example, if you have a slider next to a 3D plot and drag to far, you rotate the 3D plot. Or if you select a LMenu item over a slider, you may also adjust the slider.

@jkrumbiegel
Copy link
Member Author

jkrumbiegel commented Oct 18, 2020

if you have a slider next to a 3D plot and drag to far, you rotate the 3D plot. Or if you select a LMenu item over a slider, you may also adjust the slider

The slider thing is just bad dragging logic in the scene, my mousestatemachine code in MakieLayout guards against dragging in from the outside.

The LMenu thing with the slider is because right now, the slider accepts clicks in its whole subscene area, because the line is so thin that it's annoying to have to click it pixel perfect.. But then even if an LMenu covers part of the slider scene, the slider scene event doesn't know that and still triggers. It's not such an easy problem I think :)

@ffreyer
Copy link
Collaborator

ffreyer commented Oct 18, 2020

I'm following a game engine tutorial series on the side. The event implementation they have essentially goes as such:

  1. (GLFW) events create some event type, e.g. KeyPressedEvent and call handle!(application, event)
  2. handle!(application, event) processes some events, like a WindowShouldCloseEvent and WindowShouldResizeEvent. The remaining events are passed on to layers in reverse render order (last drawn gets events first) via handle(layer, event)
  3. Each layer can then implement handle!(layer, event) to process an event. If handle! return true, the event is consumed, otherwise it goes down to the next layer.

Every so often I wonder if that design would be better for Makie. You can more or less translate application to scene and layer to plot. This would immediately fix my second point, because the menu could consume the event. For the first point - I think it would be relatively easy to set up a gui_layer/gui_plots which take events first.

From the user perspective adding some behavior based on an event would then require implementing

function AbstractPlotting.handle!(plot, event::SpecificEventType)
    # do something
    return true # true = event consumed, false = event continues to be passed
end

@timholy
Copy link
Contributor

timholy commented Oct 18, 2020

I don't know the internals at all well (yet), but in reconsidering the API, I'll throw up a deliberately-exaggerated proposed mental model: that plot objects should be only minor elaborations of Expr.

In a bit more detail, what I mean is that scatter(x, y) should create an object that does nothing more than record three facts: it's built from x and y and should be used to create a 2d scatter plot object. The type used to encode this object should also allow for those customizations that you would legitimately supply as extra arguments (keyword or otherwise) of that scatter call, things like color, marker, etc. Things like data limits, transform, camera controls, and layout are foreign to this notion, because they would never be supplied as part of the same command. That's the sense in which plot-objects are just glorified Exprs that merely capture the arguments a user has supplied, while also permitting modifications at a later time.

Parallel to plot objects would be "views", which include a layout hierarchy, camera settings, and transforms, and "actions" (events and their handers). Plot objects could literally be stored as a flat (and completely unorganized) list in the figure in which they appear, though I suspect each should be assigned a uuid and therefore should probably be a Dict. (The uuid would also be stored as a field of the plot object and thus one could easily go back and forth between the "key" and the "value.") This would facilitate pushing a single plot object to multiple container views, where the linkage is probably made via the uuid, and for example allowing both a zoomed-out and zoomed-in view in side-by-side axes. As long as the renderer skips over stale linkages, you could also delete a uuid from the Dict and have it disappear from all panels in which it is viewed.

I think that's pretty consistent with what's in the API demo in #725 (comment). The key thing seems to be to think of scenes (or whatever replaces them) as containers that you push plot objects to, rather than an argument you supply to the object constructor. There would be no more scatter!(scene, x, y), there is only scatter(x, y). If you wanted scatter!(ax, x, y) to indicate it should live in ax, that would be fine, but its implementation would be obj = scatter(x, y); push!(ax, obj); return obj.

One apparent loss would be in interactivity: naively, scatter(x, y) won't produce a plot anymore. But I think you could make the show method replace the contents of a default/current axis container---in essence, show(obj) calls push!(currentaxis, obj); show(currentfigure).

Again, I'm a total newbie here, speaking without the long experience of Makie's designers.

@SimonDanisch
Copy link
Member

That sounds like a good plan, and I've been contemplating a lot to turn the plot object into something that is purely a spec of what should be plotted... It's just not 100% trivial to get there, under the constraints of usability, type inference, compile times and interactivity( with the event system).

@jkrumbiegel
Copy link
Member Author

I think it could be quite hard to have plot objects that just store their input arguments, as the connection of these arguments to some plottable outcome is often the heart of the plot object implementation. So I think it's fine that a scatter object can only really be displayed once it's pushed to a scene. But it's important that it's internal logic is not dependent on its "mother scene" which it was first pushed to.

Your idea with Expr like objects seems really well suited to a separate effort for a recipe system for Makie. But I'm not sure if the actual plot objects can be so reduced in scope.

@timholy
Copy link
Contributor

timholy commented Oct 19, 2020

Again, I don't have your experience. But I don't see any fundamental obstacle. Basically, I'm proposing that one make content orthogonal from layout orthogonal from rendering. That would suggest that you have some objects that represent each of these pillars. That doesn't prevent you from creating hybrid objects that combine one or more of each, plus any "glue" fields needed to make them interoperate.

AbstractPlotting and its internal MakieLayout seems fairly close to this already, so I don't mean to sell this as a more fundamental shift than it is. But my naive impression is that currently things are a bit mixed together. For example, the first call to LAxis(scene) has huge latency, because its poly! and line! calls end up triggering a whole cascade of inference. A dump of @NHDaly's new inference-trees (JuliaLang/julia#37749) up to depth 50 with AbstractTrees.jl yields a file with more than 30k lines in it. I don't know Makie well enough to be sure, but a lot of them seem to be about geometric operations and rendering (e.g., lots of NamedTuple{(:visible, :shading, :fxaa), T}, which kinda look like rendering parameters), and to me it's just not clear that declaring a container object should trigger big chunks of the rendering pipline. To see what I mean, try this (on a recent Julia nightly or source build):

using AbstractPlotting, AbstractTrees
using AbstractPlotting.MakieLayout
AbstractTrees.children(node::Core.Compiler.Timings.Timing) = node.children
AbstractTrees.printnode(io::IO, node::Core.Compiler.Timings.Timing) = print(io, node.time/10^6, ": ", node.mi_info.mi)
scene, layout = layoutscene()
function time_inference()
    Core.Compiler.Timings.reset_timings()
    Core.Compiler.__set_measure_typeinf(true)
    LAxis(scene)
    Core.Compiler.__set_measure_typeinf(false)
    Core.Compiler.Timings.close_current_timer()
    return Core.Compiler.Timings._timings[1]
end
tinf = time_inference()
open("/tmp/LAxis.log", "w") do io     # or wherever you like to store your temporary files
    print_tree(io, tinf, 50)
end

and then open up /tmp/LAxis.log in an editor. The floating-point number before each MethodInstance is the inference time, in milliseconds.

The way I think about it, it seems like it should be possible to define a heirarchy of axes, etc, independent of the actual geometry or rendering. You call that code only when you actually display an object. I'm not necessarily saying that it will end up saving much inference time (it might merely delay it), but the triggered cascade seems like a symptom of mingling among what I naively expect could be independent layers.

I don't mean to pick on MakieLayout, as it exhibits several traits that I think should become the default for Makie. And again, I don't have yours and @SimonDanisch's expertise. I'll shut up now and let people know who what they're talking about discuss this 😄.

@jkrumbiegel
Copy link
Member Author

No I think you're completely right to question these design decisions. I can only speak for the MakieLayout part, as I didn't write AbstractPlotting. But it grew very organically, and I tried to use the existing functionality to hack together an axis that could be put in a layout and that wouldn't zoom its axis labels together with the content. From there it just got more and more complex, and because no standalone plot objects were available at the time, the whole LAxis thing is written like a hybrid of plot recipe and GUI container object. I would love to make this more streamlined, don't get me wrong :)

At least GridLayoutBase got factored out in the process and is now kind of parallel to everything else. Which is nice, but sometimes people are also confused that when a layout contains an object this doesn't mean that the scene contains / renders it. So the separation of concerns is sometimes hard to enforce.

One other thing which complicates things is that the layout code depends on getting measurements of things like axis labels. These can be measured only relative to a containing scene. That's another reason why all labels are instantiated as plot objects in the LAxis constructor, so that I can set up the reactive layout for whenever they change in size.

@ffreyer ffreyer added enhancement Feature requests and enhancements planning For discussion and planning development Makie Backend independent issues (Makie core) Collection contains multiple issues labels Aug 22, 2024
@MakieOrg MakieOrg locked and limited conversation to collaborators Aug 22, 2024
@ffreyer ffreyer converted this issue into discussion #4189 Aug 22, 2024
@ffreyer ffreyer added the moved Note for projects (Issue moved to Discussions) label Aug 22, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
Collection contains multiple issues enhancement Feature requests and enhancements Makie Backend independent issues (Makie core) moved Note for projects (Issue moved to Discussions) planning For discussion and planning development
Projects
Status: To do
Development

No branches or pull requests

4 participants