search

kennyhml / pyinterception

Modern Python port & wrapper for the interception device driver
MIT License
125 stars 26 forks source link

move_to_curved function added (revised) #34

Closed Emre-Kahveci closed 2 months ago

kennyhml commented 2 months ago

There is still quite a few things bothering me here, I appreciate the attempt to contribute but I cannot accept the pr as it is;

  1. _beziercurve.py does not follow the same naming conventions as the rest of the projects, function names and several variables names are not written in snake_case as they should be.
  2. Most of the math essentially just looks gpt generated and pasted, it doesnt even attempt to explain what the purpose of it is or link to any article to read about the functionality.
  3. In both inputs.py and _beziercurve.py you added a MOVE_SPEED parameter, why is it full caps? That should be reserved for constants..
  4. I understand what you were trying by placing the math functions into the curvePoints function, but the correct way would have been to have them scoped to the modul prefixed with an underscore.
  5. Why do we need both _curve.py and _beziercurve.py? They should just be the same module
  6. With this design we expose zero control over the specific creation of the bezier curve to the user, the move_to_curved function should have an optional bezier curve object parameter and people should be able to create custom bezier curves.
kennyhml commented 2 months ago

Decided to have a go at adding this myself - implemented it by offering the functionality if pyclick is installed and exposing a dataclass with the curve parameters to the API that can be used to generate the curve.

IMO this is the best of both worlds, dont need to write up code someone else already has but also dont force the PyClick install on someone who isnt even going to use the curves.