Fixes and changes for functions in LuaBody/Player/Ship #5643

Max5377 · 2023-10-10T12:02:29Z

Update:
Rewrite of GetAltitudeRelTo:

This function is moved to body class to support this functionality in C++ code;
This function is used in the LUA version of GetAltitudeRelTo, GetGroundPosition and GetGPS for consistency in calculating altitudes;
This function supports the choice of which height to calculate (sea-level, above-ground).

Other change:
GetGPS is moved to LuaShip.cpp to support this function for other ships, not only the player.

Fixes for this issues:

GetAltitudeRelTo using player pos no matter what first argument is passed;
GetGroundPosition returns nil when not in the rotating frame of ref;
GetGPS sometimes return different height than GetAltitudeRelTo;
Access violation when trying to access frameBody, frameRotation and call GetGroundPosition when body doesn't have frame of ref (ex. when ship is hyperspacing).
Update2:
static bool push_body_to_lua(Body *body):
dynamic_cast is changed to static_cast;
case ModelBody and CargoBody had wrong template type (Body and Star).
P.S.: Unless, there's some good reason for using templates, it can actually be changed to just LuaObject<Body>::PushToLua(body).
(I checked, haven't seen any oddities).

Update3:
Removed space at the start of l_body_get_altitude_rel_to. A lot of comments didn't show up for functions, because they had a space between function name. Deleted spaces, so comments will show up when hovering function name.

Update4:

GetGPS is moved to Ship.lua to support this function for other ships, not only the player. Implementing this function in C++ is considered unnecessary because of slightly different parameter interface and because of this can be simplified a lot;
GetGroundPosition also supports the choice of which altitude to calculate (sea level or above terrain).

Old:
Fixes for this issues:

GetAltitudeRelTo using player pos no matter what first argument is passed;
GetGroundPosition returns nil when not in the rotating frame of ref;
Access violation for all three functions when body doesn't have frame of ref (ex. when ship is hyperspacing).

Need clarification on this:

Is this intentional behavior, that GetAltitudeRelTo and GetGroundPosition returns slightly different values for altitude?
Is the second change (deleteing if (!f->IsRotFrame())) should be changed back? I deleted that, because with this, when, for example you're on the Moon, altitude will already be nil at ~6000m.
Edit: to clarify second point, altitude will be different when IsRotFrame will be false (basically, when space particles (lines) will start to appear or when not in rotating frame).

Max5377 · 2023-10-18T11:31:26Z

@Web-eWorks

Looked a little closer at the description of GetGroundPosition and it says: Get latitude, longitude and altitude of a dynamic body close to the ground or nil (if missing here, small typo) the body is not a dynamic body or is not close to the ground, but should this really be limited to only close bodies?
(Which creates some issues that I mentioned in the initial comment and to add, for example, this function was used in TradeShips module and this was one cause for ships sometimes making illegal jumps. This is also used in Ships.lua IsHyperjumpAllowed and in SearchRescue.lua randomLatLong);
Although I deleted info about HUD displaying different altitudes at the center(reticule.lua) and on the bottom-right(planetary-info.lua) when player is not in rotation frame in the initial comment, but I use this info to describe issue and possible solution.
On HUD in the center GetAltitudeRelTo is used to calculate altitude to surface of the frame of ref/navTarget, but on the bottom-right GetGPS is used.
GetGPS is using slightly different way to calculate altitude which is similar to GetGroundPosition, but when player is not in rotating frame, resulting altitudes will be different for GetGPS/GetGroundPosition than GetAltitudeRelTo.
My suggestion is to add GetAltitudeRelTo to Body class (or some static function somewhere else), add calculation from LuaBody.cpp l_body_get_altitude_rel_to to this method and call this method in
GetGPS, GetGroundPosition and l_body_get_altitude_rel_to when altitude needs to be calculated. This way there will be consistency in getting altitude between all bodies;
Returned latitude and longitude is also different for GetGroundPosition and GetGPS.

These are my thoughts at the moment. I will add comments if I find something more.

sturnclaw · 2023-10-18T19:13:47Z

but should this really be limited to only close bodies?

The problem here is the split between "radar altitude" and "sea level altitude" relative to the body. Altitude above terrain features (radar altitude) is only useful for bodies actually near or on trajectory to intersect the terrain, whereas bodies in high orbit shouldn't care about terrain height but use sea-level altitude instead. Not only does this better mimic what other games (e.g. KSP) do with regards to altitude handling, it's significantly better for performance because we don't have to thrash the instruction cache (I$) evaluating the terrain fractal unless terrain is actually relevant to the body making the query.

My suggestion is to add GetAltitudeRelTo to Body class (or some static function somewhere else), add calculation from LuaBody.cpp l_body_get_altitude_rel_to to this method and call this method in
GetGPS, GetGroundPosition and l_body_get_altitude_rel_to when altitude needs to be calculated. This way there will be consistency in getting altitude between all bodies;

This is fine. The new method should ideally take a parameter indicating whether to return the altitude relative to terrain or sea-level (sbody radius) - I'll leave that at your discretion how best to ensure existing behavior is preserved with the new API.

Returned latitude and longitude is also different for GetGroundPosition and GetGPS

This should be fixed, but be wary of existing code that implicitly depends on the old/incorrect values.

Max5377 · 2023-10-19T12:56:21Z

@Web-eWorks
Almost done, just got one question:
Should GetGPS really be limited to only Player class?

And to correct myself here:
GetGroundPosition is returning latitude and longitude in radians. This is mentioned in the description:

Returns:

latitude - the latitude of the body in radians
longitude - the longitude of the body in radians
altitude - altitude above the ground in meters

Examples:

-- Get ground position of the player
local lat, long, alt = Game.player:GetGroundPosition()
lat = math.rad2deg(lat)
long = math.rad2deg(long)

Missed this while I was writing this comment.
By converting them to degrees, they are equal to the latitude and longitude returned by GetGPS with almost imperceptible inaccuracy.

sturnclaw · 2023-10-19T15:24:02Z

If GetGPS can be replaced by GetGroundPosition with no loss in functionality, then by all means please drop the GetGPS function entirely!

Max5377 · 2023-10-19T17:20:37Z

@Web-eWorks I listed all changes in the initial comment. Let me know if you find something unusual.

Max5377 · 2023-10-20T15:17:49Z

@Web-eWorks I found the cause for assertion error that I mentioned here. TradeShips code has nothing to do with it.

When ship is hyperspacing, it's not in space (in m_bodies array).
void LuaSerializer::SaveComponents(Json &jsonObj, Space *space)
void LuaSerializer::LoadComponents(const Json &jsonObj, Space *space)
This two functions only save/load lua components for bodies that are currently in space.
And since ship is not in space, it's components are not saved/loaded, making it corrupted.
For now, I just swap HyperspaceCloud with it's Ship in above mentioned functions and it fixes this behaviour.

But maybe there's a more clean way to do this?

Also, Ship stores reference to hyperspace cloud, but it loses it after loading the save.
But, should Ship really store reference to HyperspaceCloud(m_hyperspaceCloud) since HyperspaceCloud itself already stores reference to Ship? And I haven't seen it used that much, only when ship is about to go to hyperspace, when it gets created.

sturnclaw

I have a few concerns with the API patterns used in this PR, which are explored in more depth in the individual request comments. Otherwise I'm quite happy with the improvements this brings (especially the dynamic_cast removal and segfault fixes!)

Thank you for your efforts in fixing these bugs and improving the code quality of Pioneer!

src/Body.h

src/lua/LuaBody.cpp

src/lua/LuaPlayer.cpp

src/lua/LuaBody.cpp

src/lua/LuaShip.cpp

src/lua/LuaBody.cpp

sturnclaw

Thanks for addressing the feedback! I consider this PR in a good state to merge, but I've left one new suggestion where the new code is slightly overzealous in my eyes - I don't consider addressing that a requirement to merge however.

src/lua/LuaBody.cpp

Rewrite of GetAltitudeRelTo: 1. This function is moved to body class to support this functionality in C++ code; 2. This function is used in the LUA version of GetAltitudeRelTo, GetGroundPosition and GetGPS for consistency in calculating altitudes; 3. This function supports the choice of which height to calculate (sea level, above ground). Other change: 1. GetGPS is moved to Ship.lua to support this function for other ships, not only the player. Implementing this function in C++ is considered unnecessary because of slightly different parameter interface and because of this can be simplified a lot; 2. GetGroundPosition also supports the choice of which altitude to calculate (sea level or above terrain). Fixes for this issues: 1. GetAltitudeRelTo using player pos no matter what first argument is passed; 2. GetGroundPosition returns nil when not in the rotating frame of ref; 3. GetGPS sometimes return different height than GetAltitudeRelTo; 4. Access violation when trying to access frameBody, frameRotation and call GetGroundPosition when body doesn't have frame of ref (ex. when ship is hyperspacing). Co-Authored-By: Webster Sheets <[email protected]>

bszlrd added Cosmetic or UI Bug fix labels Oct 10, 2023

Max5377 mentioned this pull request Oct 13, 2023

TradeShips hyperjump code change #5647

Merged

sturnclaw self-requested a review October 14, 2023 23:00

Max5377 force-pushed the getaltituderelto-getgroundposition-framebody-fixes branch from 3730917 to 58c31bc Compare October 19, 2023 17:12

Max5377 force-pushed the getaltituderelto-getgroundposition-framebody-fixes branch from 58c31bc to e9d87b1 Compare October 19, 2023 18:30

Max5377 force-pushed the getaltituderelto-getgroundposition-framebody-fixes branch 2 times, most recently from cacbd97 to 9bb464e Compare October 24, 2023 12:13

Max5377 mentioned this pull request Oct 29, 2023

Crash Trader.lua #5653

Closed

sturnclaw requested changes Nov 12, 2023

View reviewed changes

Max5377 force-pushed the getaltituderelto-getgroundposition-framebody-fixes branch from 9bb464e to 5317261 Compare November 15, 2023 08:56

Max5377 changed the title ~~Fixes for GetAltitudeRelTo, GetGroundPosition and frameBody~~ Fixes and changes for functions in LuaBody/Player/Ship Nov 15, 2023

sturnclaw approved these changes Nov 16, 2023

View reviewed changes

src/lua/LuaBody.cpp Outdated Show resolved Hide resolved

Max5377 force-pushed the getaltituderelto-getgroundposition-framebody-fixes branch from 5317261 to 1cb457b Compare November 16, 2023 07:08

sturnclaw merged commit bd660f5 into pioneerspacesim:master Nov 17, 2023
4 checks passed

Max5377 deleted the getaltituderelto-getgroundposition-framebody-fixes branch November 18, 2023 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes and changes for functions in LuaBody/Player/Ship #5643

Fixes and changes for functions in LuaBody/Player/Ship #5643

Max5377 commented Oct 10, 2023 •

edited

Loading

Max5377 commented Oct 18, 2023

sturnclaw commented Oct 18, 2023

Max5377 commented Oct 19, 2023

sturnclaw commented Oct 19, 2023

Max5377 commented Oct 19, 2023

Max5377 commented Oct 20, 2023

sturnclaw left a comment

sturnclaw left a comment

Fixes and changes for functions in LuaBody/Player/Ship #5643

Fixes and changes for functions in LuaBody/Player/Ship #5643

Conversation

Max5377 commented Oct 10, 2023 • edited Loading

Max5377 commented Oct 18, 2023

sturnclaw commented Oct 18, 2023

Max5377 commented Oct 19, 2023

sturnclaw commented Oct 19, 2023

Max5377 commented Oct 19, 2023

Max5377 commented Oct 20, 2023

sturnclaw left a comment

Choose a reason for hiding this comment

sturnclaw left a comment

Choose a reason for hiding this comment

Max5377 commented Oct 10, 2023 •

edited

Loading