search

post-kerbin-mining-corporation / SpaceDust

Adds atmospheric and exoatmospheric resource discovery and extraction to Kerbal Space Program.
9 stars 12 forks source link

Removed duplicate divisions by density #52

Closed arbsoup closed 3 weeks ago

arbsoup commented 2 months ago

SampleResource divides the resource quantity by its density, converting from the t/m^3 that the .cfg files specify to the u/m^3 displayed in the toolbar in the map screen. But the scanner and harvester modules divide by density again, meaning that for very low-density resources like LqdHydrogen and Antimatter they pull in thousands of times more than the map screen toolbar's value implies.

I've fixed the double division. The map screen toolbar and the numbers given by scanners and harvesters should line up now.

ChrisAdderley commented 3 weeks ago

After a bunch of thinking about this I've changed the UI paradigm a little bit and gone back to showing data in t/m3. The main reason for this is that it makes comparisons between resources a lot easier because units is fungible. So this PR is technically not needed as its part of some refactors already in the dev branch

arbsoup commented 3 weeks ago

There still seems to be some density-related display issue:

image

1.5E-20 t/m^3 * 2.1E+07 m^3/s = 3.13E-13 t/s, which is off from the displayed value by a factor of ~14,000. (This is the inverse of LqdHydrogen's density in the CRP configs.) I think this is because of these lines here.

https://github.com/post-kerbin-mining-corporation/SpaceDust/blob/e19de2b6cc663daad563cc0f0b5e0fac3ae470cc/Source/SpaceDust/Modules/ModuleSpaceDustHarvester.cs#L518-L524

resAmt is resourceSample * intakeVolume in tons divided by density (518) to convert from tons to units, then that number 4.57E-09, in units, is displayed next to a "t/s" indicator (521). The actual amount harvested is more consistent with 4.57E-09 u/s = 3.13E-13 t/s, as per line 524.

Atmospheric scoops are wrong in a different way, displaying the converted resAmt incorrectly as "t/s" but then harvesting even less, multiplying the u/s value of resAmt by density (a number usually much lower than 1) to request even fewer units.

image

https://github.com/post-kerbin-mining-corporation/SpaceDust/blob/e19de2b6cc663daad563cc0f0b5e0fac3ae470cc/Source/SpaceDust/Modules/ModuleSpaceDustHarvester.cs#L460-L463

I will submit another PR on this if asked, but otherwise I figure it should be an easy fix: ignore density when setting resAmt in tons, then divide by density at the end.

ChrisAdderley commented 3 weeks ago

Might help if I actually included the last commit :/. that's what it should be

Pushed it to dev, see if the latest commit resolves your issues?

arbsoup commented 3 weeks ago

Looks good. Thanks!