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

How should we handle transformations, projections and space? #4750

Open
ffreyer opened this issue Jan 24, 2025 · 3 comments
Open

How should we handle transformations, projections and space? #4750

ffreyer opened this issue Jan 24, 2025 · 3 comments
Labels
coordinate spaces i.e. space attribute, transformations between coordinate spaces, project Makie Backend independent issues (Makie core) planning For discussion and planning development transformation related to `Transformation` objects

Comments

@ffreyer
Copy link
Collaborator

ffreyer commented Jan 24, 2025

Current Siuation

Pipeline (From user data to pixels)

(Conversions don't really matter here, they are just included for completion sake.)

  1. conversion
    1. expand args - add automatic args, e.g. x, y for image
    2. dim_convert - extracting units (etc) annd/or merging with existing units
    3. convert_arguments - normalize data types and structure
  2. transformation
    1. transform_func - apply (arbitrary?) function to converted data
    2. * float32convert - make data Float32 compatible
    3. model - 4x4 transformation matrix: "model" to "world" space
  3. projection
    0. * float32convert - make data Float32 compatible
    1. view - 4x4 matrix:: "world" to "camera/eye/view" space (i.e. viewer centered coordinate system, applies translation, rotation, mirroring)
    2. projection - 4x4 matrix:: "camera/eye/view" to "clip" space (-1..1^3 box, applies scaling, perspective)
    3. viewport - transforms -1..1 space to px units (internal in W/GLMakie)

* float32convert is an odd one out:

  • 3.1 (+3.2) are derived from limits, and must be Float32 compatible. If they are not, the float-unsafe part gets extracted as a LinearScaling(scale, translation)
  • 2.3 must also be Float32 compatible to run on the GPU. Thus the linear scaling is permuted with model.
    • If this is not possible model is applied on the CPU after 2.1, then float32convert applies in 2.2 and identity is used on the GPU in 2.3
    • else float32convert applies after 2.1 and before model in 2.3
  • For CPU projections float32converts get merged back into view/projectionview (i.e. apply in 3.0)

Ownership:

  • (2.2, 3.1, 3.2, 3.3) float32convert, view, projection and viewport are owned by the scene/camera
  • (2.1, 2.3) transform_func and model are owned by the plot but inherited from the scene (which may get its transformations from an Axis (x/yscale) or Axis3 (model), or another Block)
  • (1.2) dim_converts are owned by the Axis and synchronized with plots
  • (1.1, 1.3) convert_arguments is handled by the plot

space Attribute

Originally space only affected projections:

  • :data uses pipeline with default view, projection of camera
  • :pixel use view, projection that map pixel units to clip space
  • :clip use constant view, projection = I
  • :relative use constant view, projection scaling 0..1 -> -1..1

Notes:

  • :data may sometimes refer to "world" space and sometimes to user space (i.e. between 1.3 and 2.1)
  • transform_func is usually only applied if space = :data. There is Code with the same restriction for model, but it maybe used less...

Related Issues/PRs/Problems/Requests

Proposal

I don't like the idea of adding space = :transformed anymore. Transformations can make sense regardless of the projection we are using. For example translate!() makes just as much sense in pixel space as it does in "data" space. There may also be valid transform functions in other (projection) spaces. With that we would need various versions of :transformed, i.e. :data_transformed, :pixel_transformed, ... I don't like that.

My preferred alternative would be to split transformations and projections. This means making transformations (not including float32convert) completely independent of space. They would always be applied. If that's not desired they would need to be adjusted to identity transforms. You can already overwrite the transformations of a plot to do that:

# only inherit model
t1 = Transformation(parent.transformation, transform_func = identity)
# only inherit transform_func
t2 = Transformation(parent.transformation.transform_func)
# inherit neither
t3 = Transformation()

plot!(..., transformation = t1)

We could push this more to the foreground, i.e. document it more and advertise it as the way to handle space = :transformed. Maybe also add more convenience here, e.g. something like transformation = :inherit/:inherit_func/:inherit_model/:identity?

With that project() could look like this:

project(plot, ...)  = project(parent_scene(plot).camera, parent_scene(plot).float32convert, plot.transformation, ...)
project(scene, ...)  = project(scene.camera, scene.float32convert, scene.transformation, ...)

function project(camera, float32convert, transformations, output_space, data, TargetType = Point2f)
    mat = projectionmatrix(camera, output_space) * # projection
        to_matrix(float32convert, output_space) * # not valid for pixel etc
        transformation.model # transformation 
    
    transformed = apply_transform_func(transform_func, data) # transformation
    
    return map(transformd) do point
        p4d = mat * Point4d(point)
        TargetType(p4d / p4d[4])
    end
end

which conceptually is a projection * (transformation * data). To keep the implementation simple and clean I think a inverse_project() = inv(transform) * (inv(projection) * data) would be good. Some helper function for individual steps also seem useful, i.e.:

  • apply_transform(transform_func, data) - only transform function
  • apply_transform(transformation, data) - transform function and model
  • apply_matrix(mat, data[, TargetType]) - convert to Point4, apply 4x4 Matrix, convert back to TargetType

Mixed spaces would get simplified in some sense, because separating transformations from projections/space gives us a pass to ignore them. It would be on the user to make sure that the transformations are compatible with mixed spaces (i.e. not mix dimensions)

High level functions would still have to handle what kind of transformations they apply themselves. Some may always or never want to apply transformations. Some may want a kwarg to choose. ray_assisted_pick/position_on_plot already has an apply_transform::Bool option, which we could use elsewhere.

I also want to rename space = :data because it's unclear what that means. But it's also not clear to me what to call it. Maybe :camera as it uses projections based on the camera? :world seems weird since we have model and view space. All of them might be confusing with how transformations are separate...

@ffreyer ffreyer added coordinate spaces i.e. space attribute, transformations between coordinate spaces, project Makie Backend independent issues (Makie core) planning For discussion and planning development transformation related to `Transformation` objects labels Jan 24, 2025
@asinghvi17
Copy link
Member

asinghvi17 commented Jan 24, 2025

From a user interface perspective I don't think that having the transform_func apply by default to every space makes sense - if I say space = :pixel, then the transformation should be identity by default.

Maybe a better idea is to have a default transform_func and transformation object for every space, that is propagated with the theme? Then it becomes a lot more predictable.

As an example, if I create a Scene with some geographic transform_func, and then add an annotation for map acknowledgment in the bottom left as a box in space = :relative, I don't expect the relative space to be warped by the transform_func of data space - that input doesn't make any sense.

I do like the idea of having quick symbols to refer to the transformation's inheritance behaviour. And I agree that if we have this behaviour, then we no longer need space = :transformed.

What are your thoughts on giving each space its own Transformation?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 24, 2025

Should've probably mentioned it - creating a plot with a space not compatible with the scene already inhibits transformation inheritance. So this, for example, ends up with a disconnected identity transformation:

f,a,p = scatter([1, 100], [100, 100], space = :pixel, axis = (xscale = log10, yscale = log10))

I'm not sure how much practical use there would be for one parent transform per space. Transform functions in other spaces seem very niche to me and model transformations are mostly just translations, which may conflict with input data.

@asinghvi17
Copy link
Member

asinghvi17 commented Jan 24, 2025

If that's the case then I have no objections - users can always assign a transformation as they see fit, which they would have had to do anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coordinate spaces i.e. space attribute, transformations between coordinate spaces, project Makie Backend independent issues (Makie core) planning For discussion and planning development transformation related to `Transformation` objects
Projects
None yet
Development

No branches or pull requests

2 participants