Add Space.GetBodiesNear(), ProximityQuery module #5637

sturnclaw · 2023-10-06T21:15:07Z

Stemming from discussion on IRC, this PR exposes the BodyNearFinder functionality as Space.GetBodiesNear(), refactors Space.GetBodies() to be significantly more efficient with all current callsites, and provides an initial prototype of a ProximityTest module for the use of mission modules.

All GetBodies* function callsites now pass a classname (the same as would be used with :isa()) instead of a filter function. This moves the ObjectType filtering inside of C++ where it can be significantly more efficient than calling a Lua filter function for every object iteration (which internally called back into a C++ accessor function... sigh).

On the topic of efficiency, there is an unimplemented further optimization where we can potentially convert ObjectType into a bitfield to enable O(1) body type evaluation, given that we have a strict single-inheritance model for Bodies.

The new ProximityTest module provides a way for mission modules to request nearby bodies to be evaluated for a given reference body and to receive events when a body of the given type enters or leaves the given relevancy range of the reference body. This is mostly intended to enable @JonBooth78's WIP pirate ambush mission module, but could also be used in many different places in the codebase due to the pattern of deferring nearby body checks on an interval.

Note that ProximityTest cannot be serialized and individual tests will have to be recreated by downstream module code after loading a saved game.

This PR also provides partial type information about the LuaSpace API - I've not had time to finish providing type information about all functions in LuaSpace.

Max5377 · 2023-10-07T08:16:52Z

I was as planning to include this in my next PR, but if you want, you can add this changes too, since you're modifying LuaBody.cpp:

Access violation when trying to access frameBody, when current ship doesn't have frame of reference (ex. when hyperspacing)

    static int l_body_attr_frame_body(lua_State *l)`
     {
      ...
      Frame *f = Frame::GetFrame(b->GetFrame());
	if (f == nullptr)
	{
		lua_pushnil(l);
		return 1;
	}
      ...
     }

GetAltitudeRelTo using player pos, no matter what first argument is

    static int l_body_get_altitude_rel_to(lua_State *l)
    {
	Body* b = LuaObject<Body>::CheckFromLua(1);
	const Body *other = LuaObject<Body>::CheckFromLua(2);
	vector3d pos = b->GetPositionRelTo(other);
        ...
    }

impaktor · 2023-10-07T11:00:10Z

@Max5377 If you have a commit, @Web-eWorks could cherry pick it into this PR, if he wants to.

impaktor · 2023-10-07T11:06:45Z

I note this creates a data/modules/common folder. What's the philosophy between putting things there vs. data/libs?

Note that ProximityTest cannot be serialized and individual tests will have to be recreated by downstream module code after loading a saved game.

Sounds like something that's worth pointing out in the Lua-API documentation, as it is intended to be used by beginners.

Max5377 · 2023-10-07T11:43:37Z

@impaktor Do I just fork this branch and do PR, or I can commit directly without PR?
P.S.: just for the future, probably not gonna do anything right now.
Edit: Nevermind, I can't commit directly without write access.
Edit2: Misanderstood it. cherry-pick is a command to create a copy of the commit on your branch from another branch.

sturnclaw · 2023-10-07T22:02:59Z

What's the philosophy between putting things there vs. data/libs?

The idea is for common functionality used in mission modules to live there (e.g. the mission time/reward calculator utility, ship generation/outfitting utilities, etc.) - it's not fundamental to the execution and design of the game, but is a useful tool for creating modular content. The way I see it, data/libs is for "core" functionality and content (e.g. commodities, the mission API, etc.) which is always present and will be used by all aspects of the game - C++, pigui, mission modules, etc. data/modules is for modular "content" which plugs in to the architecture set out in data/libs but avoids having a hard dependency on other modules and can be swapped out by modders to provide a different game experience.

I've put the ProximityTest file in data/modules/common specifically because it's a prototype-quality implementation and not something I want to promote to "core" functionality at this moment. If I can come up with a better name for it and find a way for it to manage its own serialization transparently I'd consider promoting it to "core".

As a side note, individual equipment definitions should most likely live in a data/modules folder instead of core. This would also match with the goal for mods to be able to define their own equipment items without needing to ship their own copy of the equipment item list.

JonBooth78 · 2023-10-09T03:30:05Z

What's the philosophy between putting things there vs. data/libs?

The idea is for common functionality used in mission modules to live there (e.g. the mission time/reward calculator utility, ship generation/outfitting utilities, etc.) - it's not fundamental to the execution and design of the game, but is a useful tool for creating modular content. The way I see it, data/libs is for "core" functionality and content (e.g. commodities, the mission API, etc.) which is always present and will be used by all aspects of the game - C++, pigui, mission modules, etc. data/modules is for modular "content" which plugs in to the architecture set out in data/libs but avoids having a hard dependency on other modules and can be swapped out by modders to provide a different game experience.

I really like this concept, it was something I've been thinking about, whilst playing with pirates logic - for example, I see each mission spawns and equips enemy ships in their own places and in slightly different ways, whereas ideally you'd have some centralized logic to help with that. Actually, I'd like to have centralized logic to help with that as then one can have difficulty settings that would then globally effect all the missions (as appropriate).

JonBooth78 · 2023-10-12T04:46:01Z

OK @Web-eWorks , I had another look at this and realized some usage semantics that I think will make sense for using the ProximityTest.lua, as-is.

I think the general rule of thumb with this API is only create tests for a body of which your script 'owns'. E.g. if you are creating a Pirate ship module, then write the tests for the pirate ships, not the player ship, nor planets/spaceports or other things.

The reason for this would be that otherwise there is the possibility of two different pieces of code requesting the same parameters and overwriting each other's listeners.

One way around this would be for ProximityTest:RegisterTest to take an additional (perhaps optional) field as a string id, so modules could register for unique tests. Another would be for the context returned to support a list of callbacks and to be removed when none are registered.

The other thing that end up being problematic is that the way of scheduling these tests is essentially random and so we hope that the RNG is kind and we don't end up with them clustered together. I've been burned by this in the past, with either really odd, hard to reproduce bugs or frame rate instability.

I personally wonder if the timer field matters too much at all. It might be better to have no guaranteed time for these to happen and process each context, n a frame, continually (where n could vary depending on time acceleration and be one a frame if there was no acceleration on). This may well mean that we're actually doing these a lot more than we would with the current time based logic but it is more likely to give us more consistent frame rate and better worst-case frame time.

Either way, neither of these are showstoppers I don't think, I'm really grateful for this and intend to have a go at re-building the (unpublished) pirates changes I've been working on top of this, exclusively in Lua.

sturnclaw · 2023-10-15T16:31:15Z

The other thing that end up being problematic is that the way of scheduling these tests is essentially random and so we hope that the RNG is kind and we don't end up with them clustered together. I've been burned by this in the past, with either really odd, hard to reproduce bugs or frame rate instability.

I personally wonder if the timer field matters too much at all. It might be better to have no guaranteed time for these to happen and process each context, n a frame, continually (where n could vary depending on time acceleration and be one a frame if there was no acceleration on). This may well mean that we're actually doing these a lot more than we would with the current time based logic but it is more likely to give us more consistent frame rate and better worst-case frame time.

While I do agree with you, the current LuaTimer implementation serves two separate semantic purposes - a (relatively) high-precision interval-based callback implementation for game mechanics which require precise timing, and a way to request functionality to be called at some imprecise time later in the future when it's convenient. If trying to limit the performance impact of simultaneous timers, it would be better to split the two uses into separate APIs.

In the context of the ProximityTest module itself, it'd definitely be possible to limit the total number of proximity queries performed per frame as well; I've not implemented that in the current version simply because I don't expect there to be enough performance cost coming from the module in the near future to necessitate implementing a priority queue in Lua when the current timer implementation does a very good job of stochastic amortization. I can definitely bolt it on afterwards with little difficulty if it becomes needed.

I think the general rule of thumb with this API is only create tests for a body of which your script 'owns'. E.g. if you are creating a Pirate ship module, then write the tests for the pirate ships, not the player ship, nor planets/spaceports or other things.

Regarding the API for proximity test modules, the hash of body/distance/interval was just intended to make a unique key to store the proximity test object for iteration and the design of the module diverged from one in which that key is useful. If there's not an immediate need for multiple mission modules to share the same ProximityTest instance with the same parameters, I can remove the hash key and vastly simplify that aspect of the API.

sturnclaw · 2023-10-17T22:07:54Z

I've simplified and updated the ProximityTest API based on feedback from @JonBooth78, renaming it to ProximityQuery and making the method naming a bit more explicit as to the way the module is meant to be used. This hopefully removes the unnecessary complexity incorporated in the first version of the API and still retains all of the usability of the module.

I've also dropped the Modules.Common. prefix from class naming; all modules in data/modules/common will exist without prefix (unless there's a name collision with something in data/libs) and individual "separable" modules will be prefixed with the module name (e.g. ScanManager.XYZ). There will need to be some iterative renaming over time as we clean up and reduce the code duplication among mission modules, but that's a concern for another series of PRs.

sturnclaw · 2024-01-07T17:35:21Z

Will be merging this PR soon if no objections / further review is forthcoming.

JonBooth78 · 2024-01-08T09:07:59Z

No objection from me, I've been using it a while and all seemed good :-)

- Add helper to push/pull ObjectType enum values from Lua - BodyNearFinder functionality exposed to LuaAPI

- Avoids pushing all bodies in Space to Lua every time we want to perform a very simple type filter operation - Also avoids a Lua -> C -> Lua -> C call chain for each body - Replace with lua-side filtering on a reduced set of bodies where class filter isn't granular enough - Convert all classnames into ObjectType enum values for filtering

- All proximity queries are transient and should not be serialized - Proximity queries should generally be created by code that "owns" the entities being queried. - Queries are not reused/shared based on parameters due to large parameter space making useful sharing unlikely.

sturnclaw added Feature request Performance Bite-sized labels Oct 6, 2023

sturnclaw changed the title ~~Add Space.GetBodiesNear(), ProximityTest module~~ Add Space.GetBodiesNear(), ProximityQuery module Oct 17, 2023

sturnclaw added 4 commits January 8, 2024 20:33

Add Lua Space.GetBodiesNear()

7c7a4cd

- Add helper to push/pull ObjectType enum values from Lua - BodyNearFinder functionality exposed to LuaAPI

Add profiling points to Space.GetBodies/Near()

ff4ab79

sturnclaw force-pushed the lua-bodies-near branch from 32cea10 to 1a97ea2 Compare January 9, 2024 01:34

sturnclaw merged commit bf92d37 into pioneerspacesim:master Jan 9, 2024
4 of 5 checks passed

sturnclaw deleted the lua-bodies-near branch January 9, 2024 02:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Space.GetBodiesNear(), ProximityQuery module #5637

Add Space.GetBodiesNear(), ProximityQuery module #5637

sturnclaw commented Oct 6, 2023

Max5377 commented Oct 7, 2023 •

edited

Loading

impaktor commented Oct 7, 2023

impaktor commented Oct 7, 2023

Max5377 commented Oct 7, 2023 •

edited

Loading

sturnclaw commented Oct 7, 2023

JonBooth78 commented Oct 9, 2023

JonBooth78 commented Oct 12, 2023

sturnclaw commented Oct 15, 2023

sturnclaw commented Oct 17, 2023

sturnclaw commented Jan 7, 2024

JonBooth78 commented Jan 8, 2024

Add Space.GetBodiesNear(), ProximityQuery module #5637

Add Space.GetBodiesNear(), ProximityQuery module #5637

Conversation

sturnclaw commented Oct 6, 2023

Max5377 commented Oct 7, 2023 • edited Loading

impaktor commented Oct 7, 2023

impaktor commented Oct 7, 2023

Max5377 commented Oct 7, 2023 • edited Loading

sturnclaw commented Oct 7, 2023

JonBooth78 commented Oct 9, 2023

JonBooth78 commented Oct 12, 2023

sturnclaw commented Oct 15, 2023

sturnclaw commented Oct 17, 2023

sturnclaw commented Jan 7, 2024

JonBooth78 commented Jan 8, 2024

Max5377 commented Oct 7, 2023 •

edited

Loading

Max5377 commented Oct 7, 2023 •

edited

Loading