Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add element/table ignore arg to getElementsWithinRange #3871

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Fernando-A-Rocha
Copy link
Contributor

@Fernando-A-Rocha Fernando-A-Rocha commented Nov 26, 2024

getElementsWithinRange ( float x, float y, float z, float range [, string elemType = "",
int interior, int dimension, element/table ignore = nil ] )

Added new arg to the function clientside and serverside. The ignore parameter can be an element or a table of elements.

Fixes #3869

Tested and ready.

@Fernando-A-Rocha Fernando-A-Rocha marked this pull request as ready for review November 26, 2024 01:40
@TheNormalnij
Copy link
Member

Why do we need that? You can simply filter the original output in lua and use optimal algorithms and structures for that.
The additional parameter is totally unnecessary. We can keep the function simple.

@CArg22
Copy link

CArg22 commented Nov 29, 2024

I don't like ad hoc solutions, instead universal solution should be created. Make new struct/class called "ElementFilter", make support for it in argument parser. then change function signature and logic inside to:

CClientEntityResult CLuaElementDefs::GetElementsWithinRange(CVector pos, float radius, std::optional<std::string> type, std::optional<unsigned short> interior,
                                                            std::optional<unsigned short> dimension)
                                                            std::optional<unsigned short> dimension, ElementFilter filter){
...
-           if (std::find(ignoreEntities.begin(), ignoreEntities.end(), pElement) != ignoreEntities.end())
-                return true;
+          return filter.Matches(pElement);
}

Now everyone can use "element filter"easily everywhere, old functions should be updated too.

It will be easy to add new filters in future and in general maintain it.

Lua equivalent of filter can be for now simple:

{
  ignoredElements = {elementA, elementB, ... }
}

but in future can be easily extended by changing code in one place.

@ds1-e
Copy link
Contributor

ds1-e commented Nov 29, 2024

Why do we need that? You can simply filter the original output in lua and use optimal algorithms and structures for that.

Sure, you can. But having extra ignoreElement would spare you from additional if nesting in loop, or using additional function just for the sake of skipping element(s).

The additional parameter is totally unnecessary.

That's not true.

We can keep the function simple.

Even after this change, it still will be simple. Adding another argument won't get it nowhere near to level of processLineOfSight/isLineOfSightClear (which also allows you to ignore specific element)

@TracerDS
Copy link
Contributor

Also what about memory? If there is a lot of elements the that may also affect performance. IgnoreElement would be very useful here

@TheNormalnij
Copy link
Member

Also what about memory? If there is a lot of elements the that may also affect performance. IgnoreElement would be very useful here

This function doesn't produce much memory, and you transfer from LuaVM the same number of elements or more that you filter.
The performance cost of the function is O(M*N), where M is the number of elements and N is the number of filtered elements. It's poor performance. Developers should choose the right data structure to filter their collections.
I think you will prefer a single if for 1-3 elements, an array for 4-10 elements, and a hashmap for >10 elements.

You cannot expect a performance boost here. However, the performance will always be terrible for some sets of filtered elements.

Sure, you can. But having extra ignoreElement would spare you from additional if nesting in loop, or using additional function just for the sake of skipping element(s).

This is not a reason to make API complicated. We don't need to add ignoreElements to every function that produces a table output. You can write a universal solution for tables, like table.filter or filteredIterator

@CArg22
Copy link

CArg22 commented Nov 30, 2024

This is not a reason to make API complicated

First of all, if you have big server all of those extra allocations to later literraly throw away entire table slow down the server.

Instead of array, all simillar functions should return iterator that doesn't allocate any memory.

@TheNormalnij
Copy link
Member

First of all, if you have big server all of those extra allocations to later literraly throw away entire table slow down the server.

In this case, you still need to allocate your ignoreElements array. And poor data structure choice has a bigger impact on performance than an extra allocation.

Instead of array, all simillar functions should return iterator that doesn't allocate any memory.

It sounds interesting. It would be nice to have a performance check.

@tederis tederis added the enhancement New feature or request label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getElementsWithinRange: ignoredElement
6 participants