Implement turntable camera roll programmatically and clarify transformation docstrings

[harripj]

As discussed in #2279 the docstrings of elevation, azimuth, and roll appear to be misleading for TurntableCamera. This PR aims to address this and also implements roll functionality. The TurntableCamera._get_rotation_tr() function has been updated so all three transformations represent consistent right-hand rotations around the respective axes.

This PR may be missing some nuance as to the way camera rotations are defined in VisPy, for example rotations of the camera around a static scene or vice-versa, or as to why roll was not implemented in the first place. regarding the former point,I have written this PR in the first sense.

Any help or guidance appreciated! Thanks!

[djhoese]

Looks good. Thanks for putting this together. I think this makes sense. I'm not really sure why the documentation was so wrong. I have a couple requests before merging this:

1. Could you remove your test script.
2. Could you add a proper test to the unit tests. Probably here: https://github.com/vispy/vispy/tree/main/vispy/scene/cameras/tests
3. Could you restructure the docstrings for the properties to be more PEP8 compliant where the first line is a single sentence with an imperative verb, then a blank line, then any additional description. For example:

def my_method(self, x, y, z):
    """Get some value by doing stuff.

    Further description.
    """
    ... code ...

[djhoese]

Beautiful! Thank you. One small change request for better test output on failure.

[harripj]

@djhoese thanks for the advice and quick review! I just undid some auto-formatting changes that were committed. This PR should be okay for a second look!

[djhoese]

🎉

@brisvag or @almarklein do you want to give one more review and then we can merge this.

[brisvag]

Looks good to me too, assuming tests pass!

[djhoese]

Looks like two tests failed in at least one of the environments:

 =================================== FAILURES ===================================
___________________________ test_perspective_render ____________________________
vispy/scene/cameras/tests/test_perspective.py:53: in test_perspective_render
    max_px_diff=200)
vispy/testing/image_tester.py:138: in assert_image_approved
    assert_image_match(image, std_image, **kwargs)
vispy/testing/image_tester.py:201: in assert_image_match
    assert mask.sum() <= px_count
E   AssertionError
----------------------------- Captured stdout call -----------------------------
Image comparison failed. Test result: (200, 120, 4) uint8   Expected result: (200, 120, 4) uint8
Uploaded to: 
http://data.vispy.org/data/scene/cameras/3cf5aa00cbb7b70110e2afb592671d73472eca89/perspective_test.png
_____________________________ test_rotation_angle ______________________________
vispy/visuals/tests/test_axis.py:53: in test_rotation_angle
    assert_allclose(axis2._rotation_angle, -30)
E   AssertionError: 
E   Not equal to tolerance rtol=1e-07, atol=0
E   
E   Mismatched elements: 1 / 1 (100%)
E   Max absolute difference: 59.99999836
E   Max relative difference: 1.99999995
E    x: array(29.999998)
E    y: array(-30)

The last one I suppose could just be a precision error brought on by the additional multiplications with handling roll. Allowing more variance in the atol is probably OK. The other one I'm not so sure what's going on. Best case the test is actually incorrect and roll was being tested and wasn't actually doing anything and now it is. 🤷‍♂️

[djhoese]

Ah the tests upload images of the result so it looks like things are being shifted:

[harripj]

Should be able to make the tests pass by adding the - sign back here:

vispy/vispy/scene/cameras/turntable.py

Line 142 in fcc0139

transforms.rotate(self.elevation, -right),

although I believe this means that the rotation around the x-axis (elevation) is in the opposite sense to the other two rotations (roll and azimuth) around their respective axes. What do you think?

Alternatively all transformations could be performed in the same sense as elevation, ie. -ve, this doesn't cause the test to fail in my testing.

[djhoese]

I have no experience with the 3D cameras so I'm not sure I should make a decision. @almarklein @brisvag ?

[brisvag]

I guess we should strive to have everything rotate with the same handedness; we have to becareful changing this stuff though, cause it's gonna break for everyone using it :/ Is there a way we can somehow deprecate it and let people "opt in"?

[harripj]

Is there a way we can somehow deprecate it and let people "opt in"?

Would a UserWarning informing people of the future change on instantiating TurntableCamera and a use_updated_transforms flag (default is False) serve this purpose?

[brisvag]

So for one release we do that, and for the next one we have to deprecate use_updated_transforms and then remove it?

[harripj]

I think that would be a safe way to implement the change and would give users warning about it. Happy to implement this if you like the idea!

[djhoese]

I'm curious if @rougier or @almarklein know why the camera's elevation, azimuth, and roll aren't all rotating following the same rules. Maybe there is some other common way of thinking about it where this makes sense?

[almarklein]

I'm not sure. If this is only for how a mouse move rotates the camera, than one might view it more as a UX problem than inherent mathematical way of working, and a difference would be fine, I suppose?

[brisvag]

@harripj do you have a quick code example to test out exactly what the difference entails in UX terms?

[harripj]

You can test out the UX differences by running this script (based on the failing test @djhoese mentioned above: #2352 (comment)):

from types import MethodType

from vispy import io, scene
from vispy.util import transforms


def test_perspective_render():
    canvas = scene.SceneCanvas(size=(120, 200), show=True)

    grid = canvas.central_widget.add_grid()
    imdata = io.load_crate().astype('float32') / 255

    for i in range(2):
        v = grid.add_view(row=i, col=0, border_color='white')
        v.camera = 'turntable'
        v.camera.fov = 50
        v.camera.distance = 30
        v.camera.elevation = 30
        v.camera.roll = 0
        v.camera.azimuth = 40

        if not i:
            def _old_rotation_tr(self):
                """Return a rotation matrix based on camera parameters"""
                up, forward, right = self._get_dim_vectors()
                matrix = (
                    transforms.rotate(self.elevation, -right)
                    .dot(transforms.rotate(self.azimuth, up))
                    .dot(transforms.rotate(self.roll, forward))
                )
                return matrix

            v.camera._get_rotation_tr = MethodType(_old_rotation_tr, v.camera)

        else:
            def _new_rotation_tr(self):
                """Return a rotation matrix based on camera parameters"""
                up, forward, right = self._get_dim_vectors()
                matrix = (
                    transforms.rotate(self.elevation, right)
                    .dot(transforms.rotate(self.azimuth, up))
                    .dot(transforms.rotate(self.roll, forward))
                )
                
                return matrix

            v.camera._get_rotation_tr = MethodType(_new_rotation_tr, v.camera)


        image = scene.visuals.Image(imdata, grid=(4, 4))
        image.transform = scene.STTransform(translate=(-12.8, -12.8),
                                            scale=(0.1, 0.1))
        v.add(image)

    return canvas

if __name__ == "__main__":
    canvas = test_perspective_render()
    canvas.app.run()

After running this I see why the transform matrix is constructed as it is (with -elevation): the elevation and azimuth interaction both give the impression of rotating the object rather than the camera. In which case these interactions are consistent in this sense and should be left as they were, if everyone agrees?

Further, the roll interaction is not fully implemented in the UX in this PR, so perhaps the title is misleading, but this PR does allow the roll interaction to be implemented programmatically. I think that this UX is suitable for the purpose of TurntableCamera but allows the user further flexibility if they want to change roll manually. It would be great to get your opinions on this!

[brisvag]

Sounds great! Thanks for the example, very clear. Indeed it seems we want to keep the old implementation (and maybe add a comment or docstring to explain why and point to this PR for future reference).

[brisvag]

@harripj do you have time to push over the line? The remaining test failure should be fixable as you said by increasing the tolerance a bit!

[harripj]

@brisvag sorry for the delay, I have been busy lately. I will take another look at this over the weekend!

[harripj]

@brisvag I have had another look over this and updated the tests. Effectively this PR clarifies the TurntableCamera docstrings to clarify the interaction and allows programmatic adjustment of the roll parameter (this parameter is not implemented in the UI, which I think makes sense).

The UI interaction is such that azimuth always rotated around the scene Z axis, whereas elevation rotates not around the scene X axis, but around the 'effective' scene X (horizontal) axis after the azimuth transformation has been performed. I have tried to describe this in the docstring, but realise that a better description may be possible using VisPy reference frames if they are documented somewhere?

[djhoese]

Looks good to me from the small amount I understand of the conversation here. I had two comments about the docstrings.

[djhoese]

Can/should you mention what the camera "sees" when azimuth is at 0 degrees as it did in the old docstring?

[harripj]

@djhoese thanks for reviewing, I have updated the docstrings with your suggestions. Hopefully the information comes across more clearly now.

[djhoese]

This looks good to me. Let's merge it. The build website failing is a known issue and I doubt your PR is causing any issues. Thanks again for this @harripj!