1022880 - [Camera] Add focus state color to the continuous auto focus ring

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[Diego Marcos [:dmarcos]]

The focus ring should be green or red depending on the focus state: failed or focused

Comment 1

[Diego Marcos [:dmarcos]]

Attachment: Pull Request

Comment 2

[Diego Marcos [:dmarcos]]

The continuous auto focus ring also displays red or green color depending if the focus succeeds of fails. 

It also changes the focus ring animation as requested by Amy

Comment 3

[Amy Lee [:amylee] UX]

(In reply to Diego Marcos [:dmarcos] from comment #2)
> The continuous auto focus ring also displays red or green color depending if
> the focus succeeds of fails. 
> 
> It also changes the focus ring animation as requested by Amy

Hi Diego, 

Just a few things:

1. The rotation should be clock-wise
2. The animation feels rigid and slow. When you tap to focus, the ring appears and hangs briefly before it rotates. The animation should feel quick and responsive.

I had worked on the AF animation previously with Justin and we had got the animation to feel pretty good. I would keep to that original animation and just remove the snapback from the rotation. Justin, do you know where I can find that bug/patch?

Comment 4

[Diego Marcos [:dmarcos]]

The animation is exactly the same that the one Justin implemented but rotating counter clockwise and with no snapback. The original animation always felt sluggish to me. The total duration of the animation is shorter in this patch than in Justin's original implementation.

Focusing is not an instantaneous process and the time variates depending on the scene. We cannot just play an animation of fixed duration. For continuous auto focus it hangs for a little because it's only triggered when the camera finishes focusing. As soon as the camera start focusing we display the ring. We still don't know if the camera will focus or not and when it will finish. Once the camera focuses we play the rotation and we change the color to red or green. It's important to display the ring as soon the camera starts focusing to indicate the user that she has to hold the camera still.

In the case of touch to focus we need to immediately display the ring to provide immediate feedback that the touch has triggered the focus process. The animation again only plays when the camera finishes focusing.

Comment 5

[Diego Marcos [:dmarcos]]

I changed the animation to always play before focusing. The rotation still feels choppy.

Comment 6

[Amy Lee [:amylee] UX]

(In reply to Diego Marcos [:dmarcos] from comment #4)
> The animation is exactly the same that the one Justin implemented but
> rotating counter clockwise and with no snapback. The original animation
> always felt sluggish to me. The total duration of the animation is shorter
> in this patch than in Justin's original implementation.
> 
> Focusing is not an instantaneous process and the time variates depending on
> the scene. We cannot just play an animation of fixed duration. For
> continuous auto focus it hangs for a little because it's only triggered when
> the camera finishes focusing. As soon as the camera start focusing we
> display the ring. We still don't know if the camera will focus or not and
> when it will finish. Once the camera focuses we play the rotation and we
> change the color to red or green. It's important to display the ring as soon
> the camera starts focusing to indicate the user that she has to hold the
> camera still.
> 
> In the case of touch to focus we need to immediately display the ring to
> provide immediate feedback that the touch has triggered the focus process.
> The animation again only plays when the camera finishes focusing.

Hi Diego

Can you make the white focus ring 40% bigger so when it rotates it should shink down to the focus state. Also pleaes make the rotation clock-wise. Is there another bug to implement the same animation for AF? I would like to see both CF and AF in action if possible. Thanks!

Comment 7

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8441069 [details] [review]
Pull Request

Need to update tests to reflect changes made by this patch (failed on Travis/Try).

Comment 8

[Diego Marcos [:dmarcos]]

Comment on attachment 8441069 [details] [review]
Pull Request

I fixed the unit tests and addressed your comments

Comment 9

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8441069 [details] [review]
Pull Request

Looks good. Tests are passing on Gaia-Try.

Comment 10

[Amy Lee [:amylee] UX]

Hi, 

I just loaded the build and have some feedback for AF and CF: 

1. The indicator currently grows and shrinks. It shouldn't grow. It should appear and shrink to focus.
2. There is still a snap back affect happening. Please remove. The indicator should appear and rotate 45 degrees. 

Thanks!

Comment 11

[Diego Marcos [:dmarcos]]

The animation is exactly the same that Justin already implemented in the past. I just copied the code back that Amy had approved. Anyways I agree that it can be improved and I'm happy to work on that with Amy. I created a follow up bug 1030380 just to polish the animation. This bug was about adding the focus state color to the continuous focus ring so I'm going to close it to keep us focused.

Comment 12

[Diego Marcos [:dmarcos]]

Landed in master:

https://github.com/mozilla-b2g/gaia/commit/c694b6fb9e0ce18e24c6b057079052767f9aa95b

Comment 13

[Diego Marcos [:dmarcos]]

This patch needs to be uplifted to 2.0. It landed a few weeks ago on master but post 2.0 branching.

Comment 14

[Hema Koka [:hema]]

Only critical issues are being uplifted to 2.0 at this point