Request: more consistency for angle units

[Diarandor]

Some functions like "sol.main.get_angle(x1, y1, x2, y2)" work in radians. However, the circle movements work in degrees (initial angle, angle speed, etc). This is too confusing. All of these functions should use the same angle unit, and I personally prefer radians, although other people (like kids) may prefer degrees.

The best option could probably be to add another parameter (an optional parameter if you prefer) on each of these functions to indicate if we are using degrees or radians as angle unit. That is, if "unit" is one of the strings in the list {"degrees", "radians"}, we would write:

sol.main.get_angle(x1, y1, x2, y2, unit)
circle_movement:get_angle_speed(unit)
circle_movement:set_angle_speed(angle_speed, unit)

etc. And the same for all functions in the Lua API that use angles or angle speeds.

[oclero]

Using only radians is the best solution. The conversion from degrees is really easy, it should not be considered as an issue.

[christopho]

The problem is that changing these functions to work in radians will break retrocompatibility.

[christopho]

Same for circle_movement:get/set_initial_angle().

[Diarandor]

Adding that parameter as optional would not break compatibility, because the default units can be the current ones.

[Morwenn]

Some libraries just provide separate functions. For example QTransform has rotate and rotateRadians. I don't think that it's a better solution, but it's another solution you might consider. I don't know much about Lua, but wouldn't it trigger an earlier error if the function name is spelled wrongly than if the string parameter is spelled wrongly?

[christopho]

@Diarandor But the default units would be the wrong ones. To fix the inconsistency, we have to break compatibility. @Morwenn Separate functions are a possibility but we still have sol.main.get_angle() that works in radians and circle_movement:get_angle() that works in degrees.

Breaking compatibility is the only way to fix this mess. But this is okay, all non-patch releases had incompatibilities bigger than that in the past.

[christopho]

Or maybe make a function sol.main.set_angle_unit(unit) that sets the unit preference to be used by all angle functions of the API. Better than passing an optional unit parameter to all angle functions. With 3 possible values: "radians", "degrees" or "legacy" ("legacy" being the mixed units of before 1.6). Maybe is would be also useful because some users prefer degrees.

But still, the default value would have to be the ugly "legacy" if we want strict retrocompatibility.

[oclero]

Giving the the unity of the parameter is bad programming practice. I would not recommend that at all.

[Diarandor]

The last option, with sol.main.set_angle_unit makes the notation shorter and simpler. However, it may be annoying/dangerous when importing scripts that use a different angle mode, making scripts less independent, since this is used globally.

[Morwenn]

If possible I'd avoid functions that mess with the global state: I've had problem with global state at work recently (everything worked because some random math module set every string to UTF-8, and everything stopped working at once when that module stopped doing that for some reason after an upgrade). The sad truth is that any third-party script/module/function might reset that global state without telling you, even when you discourage it :/

If you're to break compatibility, explicitly passing the angle unit as a constant (could be sol.degree and sol.radian FWIW) sounds like the best idea to me.

Having a legacy value also sounds interesting, but you risk people passing the unit parameter from a function that accepts it down to another function that accepts a unit parameter, and legacy having different meanings depending on the function it's passed to, it might cause subtle errors.

[christopho]

You are right, setting a global state is bad too.

[christopho]

Other idea: make the current functions deprecated and add new ones with different names working with radians. get/set_initial_angle() can be replaced by get/set_angle(), which would give the current angle (and by the way, this is more useful than the initial angle). For get/set_angle_speed() I am not sure :)

[vlag]

get/set_angle_variation() ?

IMHO I think working with radian everywhere is the best option too since there are already math.deg() and math.rad(), and handling retrocompatibility on the engine API implementation is always the last thing I try, I even prefer the quest conversion script option :)

[christopho]

get/set_angular_speed()

[Diarandor]

I agree with Chris that we should deprecate the old angle functions, and replace each one with 2 new functions including "deg" or "rad" in the name. For instance, instead of "get_angle" we should have "get_angle_rad" and "get_angle_deg". The deprecate message will warn devs to update their functions in v1.6.