Skip to content

Conversation

@Korijn
Copy link
Contributor

@Korijn Korijn commented Mar 1, 2023

This PR:

  • throws out the current prototype code for the object API.
  • renames import pylinalg as pla to import pylinalg as la in the test suite

@almarklein I'd like to merge this before you do a pylinalg release (for pygfx/pygfx#457)

@Korijn Korijn mentioned this pull request Mar 1, 2023
@almarklein
Copy link
Member

I must admit that I'd feel a bit sad to see this part of the API go.

I now have some experience with the new (functional) pylinalg API in pygfx/pygfx#457. Overall the API is very clean and Pythonic and Numpy-ish (?), but it can also feel rather verbose.

For example, to apply two extra rotations to a rotation:

        # With the functional API
        rot2 = la.quaternion_multiply(
            r_azimuth, la.quaternion_multiply(rot1, r_elevation)
        )
        # With an object API, this could be
        rot2 = rot1 * r_azimuth * r_elevation

Or this example to transform a vector with two matrices.

    # With the functional API
    pos1 = la.vector_apply_matrix(
        la.vector_apply_matrix((screen_dist, 0, center[2]), camera_projection_inverse),
        camera_world,
    )
    # With an object API this could be
    pos1 = (
        Vector3(screen_dist, 0, center[2])
        .transform(camera_projection_inverse)
        .transform(camera_world)
    )
    # Or perhaps even
    pos1 = (camera_world * camera_projection_inverse).apply((screen_dist, 0, center[2]))

My point is that there is still value in an object API that exposes primitive linalg objects (points, directions, planes) and transformations (translation, rotation, scale). For one because it makes code easier to read, but also imagine that in an interactive session, you type "some_position.", and autocompletion shows you what you can do.

I think that the API in #25, which is rather specific to a transform object for the pygfx WorldObject, is not strictly a replacement for what is thrown away in this PR.

@almarklein
Copy link
Member

That said, building and maintaining an extra API might not be worth it, considering that the linalg code is restricted to cameras, controllers, the gizmo, and the WorldObject. (Also since for the latter we plan on a specific solution).

Copy link
Member

@almarklein almarklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a bit of pain, but I approve 😉

@Korijn
Copy link
Contributor Author

Korijn commented Mar 2, 2023

I must admit that I'd feel a bit sad to see this part of the API go.

I get that. We can reintroduce it later when we feel like it would be worthwhile.

        # With the functional API
        rot2 = la.quaternion_multiply(
            r_azimuth, la.quaternion_multiply(rot1, r_elevation)
        )
        # With an object API, this could be
        rot2 = rot1 * r_azimuth * r_elevation

Personally, I think the functional API is more readable here.

[..] For one because it makes code easier to read

So, I've come to think that's generally not really the case.

but also imagine that in an interactive session, you type "some_position.", and autocompletion shows you what you can do.

Well, yes, there's value in that. On the other hand, it's also limiting in the sense that it's not a plain numpy array anymore, even though that is probably an expectation some of our users will have.

I think that the API in #25, which is rather specific to a transform object for the pygfx WorldObject, is not strictly a replacement for what is thrown away in this PR.

I agree with that! I just feel like working with plain ndarrays, the functional API, and eventually the Transform object, is already going to lead to a worthwhile improvement in pygfx. I don't think the object API we are removing here makes it that much better. So I agree with your statement that it's not worthwhile right now.

Put another way, because we can work with plain ndarrays, we don't have to come up with a new abstraction, and that decreases code complexity and risk of "doing it wrong".

@Korijn Korijn merged commit 572d08d into main Mar 2, 2023
@Korijn Korijn deleted the reset-obj branch March 2, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants