Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring to reduce confusing terms #80

Open
dbookstaber opened this issue Jul 26, 2024 · 9 comments · May be fixed by #90
Open

Refactoring to reduce confusing terms #80

dbookstaber opened this issue Jul 26, 2024 · 9 comments · May be fixed by #90
Labels
enhancement New feature or request

Comments

@dbookstaber
Copy link
Contributor

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
Copy link
Contributor

gambon2010 commented Aug 2, 2024

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
Copy link
Contributor Author

dbookstaber commented Aug 4, 2024

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
Copy link
Contributor

gambon2010 commented Aug 5, 2024

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

@dbookstaber
Copy link
Contributor Author

dbookstaber commented Aug 5, 2024

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
Copy link
Contributor

gambon2010 commented Aug 5, 2024

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
Copy link
Contributor Author

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

@gambon2010
Copy link
Contributor

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
Copy link
Owner

@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
Copy link
Owner

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

Sync fork to master branch before making updates

@o-murphy o-murphy linked a pull request Sep 16, 2024 that will close this issue
@o-murphy o-murphy added the enhancement New feature or request label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants