magiclantern_simplified icon indicating copy to clipboard operation
magiclantern_simplified copied to clipboard

LUA: Add lens.continuous_af

Open wow0000 opened this issue 1 year ago • 2 comments

Changes

  • Added lua binding for the function is_continuous_af()

Why

This changes is useful to properly handle errors with the lens.focus() call. It avoids triggering an error and it enable proper error handling.

Testing

The code has been tested on a Canon 100D and does not generate any additional warnings or error. It should not impact current features or existing scripts.

wow0000 avatar Dec 17 '23 20:12 wow0000

This isn't an area of code I know well, so I need to ask some questions to determine what this change is supposed to do, and why it might be useful.

What error occurs on lens.focus() call without this change? What improvement in ML behaviour happens after the change?

This existing line suggests that continuous AF is not compatible with lens.focus(): if (is_continuous_af()) return luaL_error(L, "lens.focus() requires %s AF disabled.", is_movie_mode() ? "movie servo" : "continuous");

Is that true? Is it still true now, or does this change alter behaviour? If so, why?

Thanks

reticulatedpines avatar Dec 19 '23 11:12 reticulatedpines

I've spent some time trying to understand this code and something looks wrong here. You're adding a "continuous_af" field to the "lua_lens_fields" struct, but "continuous_af" isn't a property of the lens, it's a camera state. So it definitely shouldn't live in a struct that records information about the lens.

See "autofocusing": https://github.com/reticulatedpines/magiclantern_simplified/blob/04e752b9af67c5b06c2b81cf7b43f97b89a73a3d/modules/lua/lua_lens.c#L70

That's setting something based on camera state, and doesn't add it to the lens struct. Do you really need to add it there?

It would also help if you show me the Lua script you're having problems with. As is, I only have half the information.

reticulatedpines avatar Dec 19 '23 11:12 reticulatedpines