Add zoom to orthographic example

[B-Janson]

Objective

Fixes #2580. Extends the orthogonal example to include zoom as suggested.

Solution

• Describe the solution used to achieve the objective above.

[DJMcNab]

Should this zoom maths be added as a method on OrthographicProjection instead?

To clarify, I'm thinking something like:

/// Center is relative to the screen plane
/// E.g. 0,0 is the bottom left corner
pub fn zoom_to(&mut self, center: Vec2, scale: f32);
pub fn scale_clipping(&mut self, scale: f32);

[alice-i-cecile]

Should this zoom maths be added as a method on OrthographicProjection instead?

I'm definitely in agreement with this :) Looks solid though!

[B-Janson]

I'm not sure I understand exactly. In the example, zooming is performed by changing the scale of the projection and moving the camera itself. But we don't have access to the camera translation from within the projection. I can only see how it'd be possible to zoom in and out of the centre of the screen plane.

[DJMcNab]

Hmm yeah, true.

The zoom method could return the relative transform based on the Vec2 as the fraction of the screen, then you'd update the camera's transform based on that.

[B-Janson]

WDYT of the following signature
pub fn zoom_to(&mut self, screen_center_normalized: Vec2, delta_scale: f32, from_translation: &mut Vec3)

[alice-i-cecile]

Initial proposal:

pub fn zoom_to(&mut self, screen_center_normalized: Vec2, delta_scale: f32, from_translation: &mut Vec3)

Counter-proposal:

pub fn zoom_to(&mut self, new_screen_center: Vec2, new_scale: f32, camera_transform: &mut Transform)

Changes:

• new_screen_center: better communicates intent. Clearly specify that this is in Transform's x, y units in the field docs.
• delta_scale -> new_scale: idempotency is nice, and users can easily reconstruct the other API out of this one.
• camera_transform: &mut Transform -> Somewhat more type-safe, uses the familiar Transform type and clearly specifies where the values are coming from.

[B-Janson]

Hi @alice-i-cecile and @DJMcNab , I have updated the code as per my interpretation of our previous discussion. Please take a look and let me know your thoughts. I've also added a rotation system to the example.

[DJMcNab]

This looks good for the happy path, but for many other cases this would be either badly wrong - or worse, subtly broken.

[DJMcNab]

This does not correctly handle the window_origin property of self.

[DJMcNab]

I think this ignores rotation of the camera, and moves it as if it were facing in the direction of the

I think the correct way to implement this would be to create a new Transform with the translation corresponding to something like new_screen_center/self.size() * self.scale() (taking into account the window_origin correctly). You'd then apply this new transform to the passed in Transform.

Correctly handling all this will lead to some gnarly code, but that's all the more reason to make this something we have built in.

(That is why I suggested the API which returns a Transform, because it would have forced you to implement it correctly - I still think we should have this API, but the method with the easier name should do the application itself)

[DJMcNab]

Sorry, having done some more reading I've realised I don't know enough about this to be sure. This might be correct here.

[DJMcNab]

That is, the thing to do here is to add some tests which show it correctly handling all the available modes, with different rotations, scales and the like

In theory, for a higher level test of this you should use #2731, although at the moment I think this makes similar assumptions

[B-Janson]

Sorry for the delay, I have been very busy finishing up the quarter with work! Back and able to contribute with a bit more consistency.
After thinking about this a little bit, I'm back to thinking that the original commit was the best solution. Just including a specific example for how to achieve rotation and zoom for a specific example. I'm not sure it is clean enough to define a zoom function within the projection itself just yet, especially if it wouldn't be consistent amongst all other current projections. What do you think?
cc @alice-i-cecile @DJMcNab

[alice-i-cecile]

I'm not sure it is clean enough to define a zoom function within the projection itself just yet, especially if it wouldn't be consistent amongst all other current projections. What do you think?

Hmm. I think that the smaller, scoped demo has clear value. Then, we can tackle a proper, general solution in another PR and update the example.

[alice-i-cecile]

@B-Janson I'm going to add the Adopt-Me label, but feel free to pick up this work again yourself :)

[NthTensor]

I want this but it seems stale. I recommend we close this an open a tracking issue for the example.

[alice-i-cecile]

Seconding that. Please close once an issue is up.

[tychedelia]

This would conflict with #11022, we should probably just request an example there.