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

Plot update refactor (prototype) #4550

Draft
wants to merge 58 commits into
base: master
Choose a base branch
from
Draft

Plot update refactor (prototype) #4550

wants to merge 58 commits into from

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Oct 30, 2024

Description

Prototype for #4360, implementing a mostly Observable-free way of propagating updates to backends. This is an attempt to implement scatter top to bottom in GLMakie.

In this Prototype updates go through a few stages:

  1. update!(plot, attributes...) adds all given attribute names to plot.updated_inputs, marking them as updated. Then it sets all the attribute values as given. Then it triggers an update of plot.updated_inputs for the backend to react to.
  2. resolve_updates!(plot) goes through all plot.updated_inputs to apply Makie level conversions and calculations. The results are written to plot.computed and marked in plot.updated_outputs. This step is called by the backend, allowing it to be push-based (on(plot.updated_inputs)) or pull based (call if needed in renderloop)
  3. The backend then does additional processing for values in plot.computed. Redundant updates can be avoided by checking plot.updated_outputs.

Problems: (need solution)

  • Need generic name for args to handle them generically (and thus do update!(plot; kwargs...) generically) We're going with update!(plot, arg1 = ..., arg2 = ...) for now.

Shortcuts taken: (probably rework)

  • Colorbar rebuilds effectively deprecated ColorMapping
  • Legend falls back onto using Observables
  • data_limits, boundingbox trigger full resolve_updates for marker metrics
  • plot.converted still contains Observables (these don't do anything though)
  • Attributes still converts everything to Observable, though with no (plot/robj based) listeners
  • FastPixel and normal markers are handled in the same function which is quite ugly/annoying

Notes (probably fine)

  • Observable attributes effectively register on(v -> update(plot, name = v), obs)
  • Backend uses plot.computed for caching some values (e.g. positions before GPU upload for reuse)

Benefits:

  • Cuts out ~60 of 120(?) Observables per scatter, without touching Attributes (37), transformations (5-10)
  • The plot update performance during rendering seems to be 2-3x faster (Frame time does not improve that much, because other parts (rendering, pollevents, SwapBuffers) also each up time) (these benchmarks maybe outdated now as I added a lot of temporary test fixes)
  • Allows for synchronous updates

Potential benefits:

  • It might be easier to disconnect the scene:
    • camera, lighting, etc can query parent_scene(plot), allowing the scene to be a runtime variable
    • scene can notify scene.plots of updates, allowing plots to a runtime variable
  • Caching is sort of built into the system, as update!(plot, ...) marks outdated inputs and resolve_updates!(plot) only updates outputs which have outdated inputs. To make this more usable we may need to register update functions per plot like in my earlier prototype

Type of change

Delete options that do not apply:

  • 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.

@asinghvi17
Copy link
Member

asinghvi17 commented Oct 30, 2024

If we're doing this - is it possible for the backend to know about the change and recreate a plot / scene when some setting (transform_func dimensionality, scene lighting, etc) changes?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Oct 30, 2024

Updates that affect shader compilation (e.g. NoShading vs FastShading vs MultiLightShading, converted dimensionality, ...) will continue to not work. We may even double down and use some of this information to pin down types for better performance.

If the update doesn't affect the shader I don't see why it shouldn't work. I.e. updates to scene.lights or changing transformation.transform_func.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Oct 30, 2024

This will probably make plots easier to detach so maybe we can do something like this in the future:

detach!(plot) # remove from parent scene, destroy renderobject, destroy update routine
plot.shading = MultiLightShading
attach!(scene, plot) # add to new scene, create new renderobject + update routine

@SimonDanisch
Copy link
Member

Yeah, the trend is to make the backend plot type stricter and less dynamic, while improving the tools for Makie itself to just re-create a plot for type changes.

@asinghvi17
Copy link
Member

Sweet, as long as that can be automated at some point in the future it sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Work in progress
Development

Successfully merging this pull request may close these issues.

3 participants