o-murphy / py-ballisticcalc

LGPL library for small arms ballistic calculations based on point-mass (3 DoF) plus spin drift.
https://pypi.org/project/py-ballisticcalc
GNU Lesser General Public License v3.0
18 stars 8 forks source link

Refactoring to reduce confusing terms #80

Open dbookstaber opened 1 month ago

dbookstaber commented 1 month ago

As shown in this discussion, we should try to improve the clarity and accessibility between look_distance and horizontal distance.

Per this discussion we should either switch the signs on "adjustments" or rename them so that they are clearly describing the drop instead of the adjustment to correct for the drop.

gambon2010 commented 1 month ago

Regarding 'adjustments' personally i would change it to 'drop' as this would make the code clearer and, if i am not mistaken, would require no changes to the logic of the library. This would also avoid breaking apps that use the results as they are. (instead of drop_adj -> drop_angle)

Regarding the look angle question, would it not be possible to replace the 'distance' value with the 'true' distance by calculating it using the cos formula in the discussion? So when a distance of 300yards is accessed, you get the correct drop at a distance of 300 yards, or would this possibly cause confusions elsewhere? @dbookstaber

dbookstaber commented 1 month ago

So we could probably take care of this all in the class TrajectoryData simply by renaming class attributes as follows:

  1. distance >> x
  2. height >> y
  3. look_distance >> distance
  4. drop_adj >> drop_angle
  5. windage_adj >> windage_angle

Of course, this would break anything that relied on the old names. @o-murphy is there a way to do this without requiring a major release?

gambon2010 commented 1 month ago

@dbookstaber Would it not also make sense to rename 'windage' to 'z' to keep the same logical as distance -> x and height -> y?

dbookstaber commented 1 month ago

My thinking was that x and y are less-public attributes, for use by people who take the time to dig in enough to understand what they refer to (because some ballistic coordinate systems use y for windage and z for drop), and who understand the distinction between the horizontal coordinate system and the angles relative to the sight line. But every shooter understands and cares about drop and windage. Also, windage is independent of whether the sight line is horizontal, so while windage always equals y that is not true for the other two coordinates when there's a non-zero look_angle.

That said, it wouldn't hurt to add a z as a synonym for windage.

gambon2010 commented 1 month ago

That said, it wouldn't hurt to add a z as a synonym for windage.

This makes sense.

Also, about get_at_distance, should we change the logic of the function so it gets distance (previously look_distance) or should it keep looking for the same value, which would now be x (previously distance)?

My opinion would be to return the TrajectoryData at the new distance instead of x so that you get the correction of the real distance you are shooting at

dbookstaber commented 1 month ago

Yes, that makes sense and I can't find any dependencies that would break.

gambon2010 commented 1 month ago

Following previous logic, for: def fire(..., trajectory_step: [float, Distance] = 0, ...), trajectory_step would also be steps of the new distance instead of x. If all the previously mentioned changes are implemented I think this would drastically improve the ease of use of the library. I can probably write up a pull request this week. Feel free to make suggestions after I push the code, I most work with js/c# and a somewhat new to python.

o-murphy commented 1 month ago

@gambon2010

я більшого працюю з js/c# і дещо новим для python

I also created JS version of this library, but have not time to update it to v2.x.x, if you want you can contribute https://github.com/o-murphy/js-ballistics

o-murphy commented 3 weeks ago

Можливо, цього тижня я зможу написати запит на отримання

Sync fork to master branch before making updates