ScanManager improvements and ship properties fixes

[Max5377]

UPDATE
As suggested by @Web-eWorks, first solution changed to wrap totalCargo and usedCargo around this check (ex. for totalCargo) in function CargoManager:Constructor(ship):

if not self.ship:hasprop("totalCargo") then
	ship:setprop("totalCargo", self:GetTotalSpace())
end

so this properties will not be reinitialized by CargoManager's constructor, because after saving the game, this properties will be already stored in the ship and will be unserialized during loading the game.
self.ship:UpdateEquipStats() was not needed since it's called internally from C++.

While testing more, another bug was found, maxHyperspaceRange wasn't properly initialized after loading the game.
This happened, because in this line in void Ship::UpdateLuaStats():

	if (hyperclass) {
		std::tie(m_stats.hyperspace_range_max, m_stats.hyperspace_range) =
			LuaObject<Ship>::CallMethod<double, double>(this, "GetHyperspaceRange");
	}

m_stats.hyperspace_range_max, m_stats.hyperspace_range are mixed up since GetHyperspaceRange returns range and range_max, range is zero at this time, but will be initialized later.
Switched them up so maxHyperspaceRange will be properly initialized.

Incorrect display of cargo space used after loading the game

Fixes #5557 fixes #5534
(Maybe more)

The Ship's properties were not assigned new CargoManager property values after it was loaded from save.
The most evident case of this is incorrect display of cargo space used in personal info after loading the save.
This properties is used in a bunch of other places that I listed here.
One of this places is calculating probability of pirates attacking player, so I guess expect more pirate attacks?!
I also want to clarify that this was happening to every ship present during save loading.
To fix it, added this lines in CargoManager:Unserialize:

self.ship:setprop("totalCargo", self:GetTotalSpace())
self.ship:setprop("usedCargo", self.usedCargoSpace)

self.ship:UpdateEquipStats()

To assign new values to Ship's properties during CargoManager unserialization.

Still don't know if this line is needed:

self.ship:UpdateEquipStats()

but I leave it for now, just in case, like in CargoManager:OnShipTypeChanged().

Scanner not starting previous scan after loading the game

Fixes #5577

ScanManager: StartScanCallback had no way to determine if the current ScanManager was loaded from save when it needed to set up a callback, because field activeCallback of ScanManager is saved and during unserialization ScanManager:StartScanCallback is called which has this check:

if updateRate == self.activeCallback then

which basically thinks that callback is already setted up for current ScanManager, which in reality is not.

Added additional argument to ScanManager:StartScanCallback force and changed this check to:

if updateRate == self.activeCallback and not force then

to force through this check if ScanManager:StartScanCallback was called from ScanManager:Unserialize.

Orbital scanning stops despite meeting maximum altitude requirements

This was due to an oversight in the ScanManagers code, when it did not take into account that player might be too far from the planet, when his frame of reference (frameBody) was changing, immediately stopping the scanning process.
Added additional checks when current scan is orbital (ex. immediatly returning from onFrameChanged when current scan is orbital) and branching body choice:

self.activeScan.orbital and self.activeScan.bodyPath:GetSystemBody().body or self.ship.frameBody

Update: a small revision of code.
Update2: added additional assertions with messages to better describe what happened.

[Max5377]

Probably going to do more fixes to scanning mission. I have a orbital scanner, which says that maximum scanning altitude is 5.71Mm, but as soon as my frame body changes (~2.3Mm altitude) I can't scan planet. Unless I misunderstood something.
Edit: Oberon, Sol
Edit2: Normal edit.
ScanManager right know is too dependent on Ship's frame body or frame of reference. Just imagine, if you're on Neptune and your frame of reference changes to Sol. Maximum scanning altitude will be no longer valid because, it will try to calculate all parameters (ex. altitude) for Sol, immediatly ending scan, because it will be too far from you ship.
The preliminary decision will be to get body you need to scan at the start of mission and setup timer that will check distance between ship and body that needs scanning.

[Max5377]

I guess this is how it meant to be:

OrbitalScan.mp4

[Max5377]

Had an occasion where I need to do two scans from the same planet, but I can do only one scan. Oh well.
Edit: write to me if you want this to be changed. Maybe we can explain that, because one sensor can only do one scan, for example or maybe you have other ideas about it. Personally, I am for convenience so I can just change code to support multiple active scans, but want to hear your opinion first.

[Web-eWorks]

Personally I think the constraint of one active scan is acceptable for multiple reasons:

• The probability of getting multiple "compatible" scans of the same body from the same station is intended to be extremely low;
• The point of the scan gameplay loop is to incentivize the player to manually adjust their orbit to the maximum permissible for the contract, to optimize the amount of "real time" they spend scanning the body. With simultaneous scans with differing maximum orbits, the gains from smart maneuvering and manual flight mean less than brute-forcing time acceleration.
• It avoids a pathological "min-maxing" behavior of only taking scan contracts for bodies where you have multiple scans at once. The payout vs. time spent scanning should be the metric that scan missions are balanced around, and that goes completely out-of-control if you can accomplish 2 or 3 scans in the same timeframe it takes to do one (exploiting).

The point of the work being done on Pioneer's economy is to provide consistent rewards to the player for their time investment no matter which gameplay loop they want to try, not have most activities be a pointless grind for pennies and a select few exploits/activities trivialize the rest of the game, as in Elite Dangerous.

[Max5377]

Thank you for your opinion. I agree with you on exploiting and min-maxing part. I can also add that performing multiple scans with just one scanner when you have missions that require scanning in different conditions sounds like nonsense in itself. How is this even supposed to be possible?
P.S.: after I wrote this, I had the thought "do multiple scans that require multiple scanners", but this will probably also force player to choose one good strategy or to min-max.

[Web-eWorks]

Of the changes here in the PR, I've left two comments here where I'd like to see the solutions amended to better address the underlying issues. Otherwise, this seems fairly sensible and non-invasive; thank you for tackling these issues!

[Web-eWorks]

Also, if you're willing, I will push a commit to this PR once you've addressed the above PR feedback that amends the time acceleration limits in Game.cpp to allow higher "default" time acceleration while near bodies, without needing to Ctrl+Click and override time acceleration.

[Web-eWorks]

As a minor nitpick for future contributions, we prefer the use of either hyphen-delimited-lower-case or PascalCase branch names without the use of any special punctuation characters. Adding spaces or punctuation characters makes your branches more difficult to work with in a terminal while performing a review.

[Max5377]

I think disabling Ctrl+Click can be added as an option in setting's menu like "Always force time acceleration". I can add that if you want, since I already done some changes to game.lua and settings-window.lua like pause menu now remembers last speed (you probably saw it on IRC channel) and I moved Input.EnableBindings and SetTimeAcceleration from settings-window.lua, so game.lua is the only responsible now for setting this and just tracks when optionsWindow is closed. It looks like this:

ui.registerHandler('game', function(delta_t)
...
       if currentView == "world" and ui.escapeKeyReleased(true) then
	       if not ui.optionsWindow.isOpen then
	       lastTimeAcceleration = Game.GetTimeAcceleration() ~= Game.GetRequestedTimeAcceleration() and        Game.GetRequestedTimeAcceleration() or Game.GetTimeAcceleration()
	Game.SetTimeAcceleration("paused")
		ui.optionsWindow:open()
		Input.EnableBindings(false)
		settingsMenuWasOpened = true
		else
			ui.optionsWindow:close()
		end
	end

	callModules('modal')
		
	if settingsMenuWasOpened and not ui.optionsWindow.isOpen then
		settingsMenuWasOpened = false
		local forceTimeAcceleration = ui.ctrlHeld() and (lastTimeAcceleration ~= "paused")
		Game.SetTimeAcceleration((lastTimeAcceleration == "paused") and "1x" or lastTimeAcceleration, forceTimeAcceleration)
		Input.EnableBindings()
	end
...
end

ui.ctrlHeld() for forceTimeAcceleration doesn't work right now since if you press CTRL and then ESC, it just opens Start Menu on Windows, but will work if you press Close button instead.
Also, spotted another bug while testing UpdateShipEquipment, maxHyperspaceRange returns zero after save loading for player, but GetHyperspaceRange returns normal values. Seems like, maxHyperspaceRange is used in displaying ship's info (Hyperspace range max will be zero) and in DeliverPackage module:

local onCreateBB = function (station)
       ...
       local canHyperspace = Game.player.maxHyperspaceRange > 0
       ...
end

Edit: aside from that, feel free to make any changes you deem necessary.

[Web-eWorks]

I think disabling Ctrl+Click can be added as an option in setting's menu like "Always force time acceleration".

I would strongly prefer not to do this.

The Ctrl+Click is intended to override the game's limits as to what's considered a "safe" time acceleration both in-world and from a developer perspective. I'm willing to amend those limits slightly given the optimizations and improvements that have been made since Pioneer first introduced them, however the distinction between forcing a time acceleration setting and requesting one is an important one to have. If clicking on the buttons always forces time acceleration, the player will experience many unintended deaths and other program errors. For an example, in a low orbit around a Titan-sized body at 10,000x time acceleration your orbit will shift very drastically due to numerical error.

EDIT: the goal should be to improve other parts of Pioneer so that the game functions correctly with relaxed limits on requested time acceleration, rather than always force time acceleration regardless of the game's limits.

I'll address the other comments etc. later once I've gotten a fresh mind to go over them.

[Max5377]

void Ship::UpdateLuaStats() in this line:

	if (hyperclass) {
		std::tie(m_stats.hyperspace_range_max, m_stats.hyperspace_range) =
			LuaObject<Ship>::CallMethod<double, double>(this, "GetHyperspaceRange");
	}

m_stats.hyperspace_range_max, m_stats.hyperspace_range are mixed up since GetHyperspaceRange returns range and range_max, range is zero at this time, but will be initialized later. That's why maxHyperspaceRange was zero.

[Web-eWorks]

You appear to have a partial manual rebase against master in Ship.cpp - this branch currently doesn't compile because it's missing the change to Ship.h from master. That said, I've tested and pushed the commits I spoke of prior - you should be able to rebase this branch against master in preparation for merge.

[Web-eWorks]

Excellent work, did a quick playtest and everything seems to work correctly!

[yump]

Thank you so much @Max5377, for finding and fixing the cause of #5534. I had to stop playing the game after spending two weekends trying to wrap my brain around all the things that participate in cargo/mass calculations. I was so stuck on this that I never got around to notifying flatpak people about #5574.

... which is apparently still hitting people -- see #5595. Mea maxima culpa.

[Max5377]

@yump Have fun) Don't forget to also thank @Web-eWorks for the advice on finalizing my solution. I don't have Linux, so I can't look why this issue happenes.