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

Fixes #5360 - lead calculation changes #5417

Merged
merged 1 commit into from Nov 24, 2022

Conversation

azieba
Copy link
Contributor

@azieba azieba commented Nov 7, 2022

  1. Removal of lead calculation for beam weapons. They become deadly!
  2. I tried to improve the lead calculation a bit. I have a save file where I have 0 speed relative to a trader ship that is 2.3 km away and unable to hit it because the target is accelerating. Adjustment for target acceleration helps with this.
  3. There was this FIXME for doubled lead angle. The problem was that the Fire function was rotating shooting direction that was already rotated giving twice the needed angle..I think. Anyway the additional transformation in the Fire function was unnecessary.

@impaktor
Copy link
Member

impaktor commented Nov 7, 2022

Why do you need to merge with master? If your branch needs updating, due to conflict, it's better to rebase, me thinks.

@azieba
Copy link
Contributor Author

azieba commented Nov 7, 2022

Why do you need to merge with master? If your branch needs updating, due to conflict, it's better to rebase, me thinks.

I had branch with the same name that I deleted. When I created it again it was outdated. I'm not very proficient in git magic. I'll try to remember that I can rebase. Thanks!
Or I can still do it - delete the merge and do rebase?
EDIT: Yes I rebased.

@azieba azieba force-pushed the guns branch 2 times, most recently from 1abc1b5 to 54f8fc9 Compare November 7, 2022 21:48
@impaktor
Copy link
Member

impaktor commented Nov 7, 2022

Sometimes when I'm in that situation, I make note of the hash of the commit I want to save, and git reset --hard the branch to latest master, then git cherry-pick the commit I want, (it's still in reflog, and not lost by the git reset --hard). Although, reset --hard, should be used by caution, as anything not checked in will be lost.

Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I like these changes, and I definitely welcome an exact solution to the lead problem rather than the approximation we had before! I've mentioned a few things that probably should be cleaned up before merge, but other than those the PR is very clean.

src/FixedGuns.cpp Outdated Show resolved Hide resolved
Comment on lines +295 to +293
//This is an exact solution as opposed to 2 step aproximation used before
//It does not improve the accuracy as expected though.
//If the target is accelerating and is far enough then this aim assist will
//actually make sure that it is mpossible to hit..
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be interested in a bit more data on the perceptual or actual measured increase in accuracy between the two methods, as the comment here seems to imply both have about the same effective accuracy when considered without the acceleration term.

Copy link
Contributor Author

@azieba azieba Nov 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I tried to solve this without approximation, lets say, for fun. Theoretically it should be better but in practice there was no noticeable difference. This is why I left the approximation method to decide during review which to choose. I do not think that the exact method is heavier computationalylly so maybe it could stay.

The adjustment for acceleration made noticeable difference. If a target flies away from you and is accelerating then you will get reliable hits. The good thing about the exact method is that it produced time to use for the acc adjustment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing that stands out to me as slightly more expensive would be the sqrt() call, but it should be a negligible difference in practice. If the new method is both an analytic solution and provides the travel time term "for free" then I'm all for it.

In performance terms, I'd only recommend to avoid division where possible, as the general rule of thumb is that division is much more expensive than multiplication by a pre-calculated reciprocal of the term. E.g. instead of dividing by 2, prefer multiplying by 0.5. If my elementary math knowledge is correct, I also believe a term like value / l / l can also be simplified as value / (l * l) which replaces a division (which needs to "wait" for the previous division to finish) with a multiplication that can happen independently of the prior division.

For those interested, according to Intel's documentation on Skylake and newer chips a division operation (DIVSD) is between 3 to 8 times as expensive as a multiplication (MULSD) is on double values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the approximation method used sqrt also to calculate length of the distance vector twice, but I suppose if the compiler is not too dumb it would only be done once.
I'll try to get rid of those divisions then. Good to know about the difference in complexities.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, good eyes. I can only apologize for being very tired when I wrote that, this is indeed one sqrt() cheaper than the prior solution, and the current iteration of the code has the same number of divisions. I'm satisfied with the current state of the code; I don't think there's any further optimization gain here that would be worth the time investment to realize (if at all).

The overall performance cost of this code is quite small for a single iteration (I'd guess around 1-2 microseconds at most), but in practice I'd like to be able to run lead computations on AI turrets, fixed/gimballed ship weapons, etc. for at least 100 (if not closer to 1000) individual weapons per frame in the future, which requires the individual iteration cost to be quite small indeed. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI turrets, fixed/gimballed ship weapons, etc. for at least 100 (if not closer to 1000) individual weapons per frame in the future

Wow. Are we gonna have battleships in the game?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually stations will need self-defense weapons, and there has long been a plan for large (2km+) "bulk ships"... including military/naval variants of such. You can imagine how many guns would by design be present on such installations!

src/FixedGuns.cpp Show resolved Hide resolved
@azieba azieba marked this pull request as draft November 15, 2022 23:29
Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you have further changes you'd like to make to the code (or I'm missing some part that doesn't compile), I consider this PR to be in a good state to merge, pending some testing effort I'm unable to provide at this time.

As an aside, we (the "core" development team) generally only start a PR as a draft when it's in the work-in-progress stage and not anywhere near the final form; once a PR has matured enough that we're ready to review and consider it for merge we usually don't convert it back to draft unless there are major architectural or fundamental changes required that would essentially "send it back to the drawing board" and require a redesign or more effort than the author is able to contribute.

All that to say, this PR is in a very good state and doesn't need to be sent back to the draft folder at all!

@azieba
Copy link
Contributor Author

azieba commented Nov 16, 2022

I need to have a look at a behavior of lead indicator for landed or docked ships. There might be a problem with acceleration coming from force of gravity even though the ships are not moving. I think about adding if( flightstate = FLYING) for acceleration adjustment. That is why I switched to draft, to prevent merging until I will adjust it.
Maybe I could not hit just because landed ships have collisions turned off.

@Web-eWorks
Copy link
Member

Landed ships do indeed have collision switched off, as part of an optimization effort by @Gliese852 from the last update.

@Gliese852
Copy link
Contributor

@Web-eWorks

Landed ships do indeed have collision switched off, as part of an optimization effort by @Gliese852 from the last update.

It's been off since time immemorial, I almost didn't touch that code. Maybe I fixed some bug related, but I don’t remember if it got into the master.

@azieba azieba marked this pull request as ready for review November 16, 2022 20:58
@azieba
Copy link
Contributor Author

azieba commented Nov 16, 2022

I have checked. Landed ships have 0 force so no need to correct. The switched off collisions confused me I think.

I am also wondering if some averaging of the target acceleration would be a good idea.

I need to hide the lead indicator when there is not firing solution.

@Web-eWorks Web-eWorks merged commit 74f1044 into pioneerspacesim:master Nov 24, 2022
@azieba azieba deleted the guns branch November 24, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants