search

tomforwood / kerbal-impact

MIT License
8 stars 4 forks source link

After-impacts overwrite initial impacts. #10

Closed WazWaz closed 7 years ago

WazWaz commented 7 years ago

Often crashing a vehicle will create more than one "impact" in rapid succession. eg. a 3GJ impact followed immediately by one or two 500MJ impacts, as KSP explodes and crashes each part in the stack. This gets recorded by the bangometer as a 500MJ impact.

Impacts within a very short period should be added together. This is also physically correct - each atom on the crashing craft is in theory a separate tiny low-energy impact.

Kerbas-ad-astra commented 7 years ago

I caught that issue some time ago and made a fix for it (Kerbas-ad-astra/kerbal-impact@9a0192e1f3acf0c86f9ae35c2cb2b68d45114c1f), but it fell off my radar to see if it was still an issue and if my fix actually worked.

WazWaz commented 7 years ago

Hmm, I'm using Impacts 1.6.0, so that change should be in it, if I understand it, but it's definitely not working. It prints 2 or 3 lines to the top-right of the screen each time, and the bottom-most one has always been the lowest MJ, and even though the one above it is sufficient for the contract, transmitting from the Bangometer doesn't fulfill the contract (even though it has in the past when sufficiently high single-impacts occurred).

I'm happy to test anything you'd like me to.

Kerbas-ad-astra commented 7 years ago

No, I haven't made a pull request to Tom's repo with that commit, so it's not in the release. I'll get my repo up to date and get you a DLL to test.

tomforwood commented 7 years ago

There is code that is supposed to stop impacts bring overridden by smaller ones and it has always worked for me in testing.

Kerbal and Astra's changes contain extra checks that should eliminate any possible issue though

if you do create a pull request I'll do another release and maybe even do the internationalisation I promised months ago

Kerbas-ad-astra commented 7 years ago

I've updated and made the PR, but it might be good to have WazWaz test it before merging and releasing: kerbal-impact-test-no2ndary.zip

WazWaz commented 7 years ago

Works for me - I tested the exact case that was a problem before, and it worked correctly.

WazWaz commented 7 years ago

BTW, I still find it questionable that the close values aren't added. It's entirely an implementation detail that sometimes some parts of the craft "crash" before others.

tomforwood commented 7 years ago

The energy of the initial impact is the total mass of the craft times it's velocity squared. You could argue that secondary impacts should be subtracted from that because any energy they have is energy that wasn't put into making the body ring and instead of one nice clear impact you have smaller messier impacts that could obscure fine details.

WazWaz commented 7 years ago

Ah, I thought it was breaking the craft up, then calculating the separate parts; it's fine then, yes.