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

Isolate space from transformations #4792

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

Isolate space from transformations #4792

wants to merge 30 commits into from

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Feb 14, 2025

Description

Closes #3796

This pr removes space from transformations and adds more convenience inputs to the transformation attribute, as suggested in #4750.

The goal of this is to give space a clear and well defined scope - that of "projections", i.e. coordinate spaces that could be handled by a camera. "Transformations", i.e. the model matrix and transform function (or scale in Axis) are completely cut off from space here. They would instead be handled through the transformation attribute which now accepts:

  • automatic, :auto, :automatic: inherit transformations from parent based on space compatibility
  • :inherit: inherit transformations regardless of space compatibility
  • :inherit_model: inherit just the model matrix (i.e. rotate!(), scale!(), translate!()) regardless of space compatibility
  • :inherit_transform_func, :inherit_transform_function: inherit just transform function regardless of space compatibility
  • nothing, identity, :nothing, :identity: no inheritance, create new identity transformation

Previous options such as an explicit Transformation() and arguments for transform() (e.g. :xy, (translate = (1,2,3),)) continue to work. The latter assumes automatic for the inheritance setting. (inheritance_option, transform_arg) is currently also allowed.

TODO:

  • test transformation inheritance options
  • rename space = :data to :world currently vetod by Simon
  • check/update/add space related tests
  • document/update space documentation
  • update conversion/transform/projection pipeline docs
    • clean up TODO in those docs
    • fix nested list
  • document transformation attribute
  • document transform!() functions
  • document spaces()
  • check/update other space related docstrings (e.g. boundingbox, project, ...?)
  • consider updating interface for functions other than plots (e.g. project(), mouseposition(), boundingbox(), position_on_plot(), ...) (this may be a lot of not strictly required work better suited for other prs)

Tangential changes:

  • added convenience methods for is_..._space() to work with Observable and plot inputs
  • cleaned up space handling to rely on is_..._space() more and assume plot.space exists when it should (i.e. for primitive plots mostly in backends)

Type of change

  • 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
Copy link
Collaborator

MakieBot commented Feb 14, 2025

Benchmark Results

SHA: e1b45fa1352c31dc0978cca2ec1e20f1217f6652

Warning

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

GLMakie
CairoMakie
WGLMakie

@ffreyer ffreyer mentioned this pull request Feb 17, 2025
7 tasks
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.

Document Makie.spaces() values
2 participants