Skip to content

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
Contributor

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.

@sacr1ficez
Copy link
Contributor

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
Contributor

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
@tederis
Copy link
Member

tederis commented Dec 21, 2024

Thanks for the PR. But my personal opinion is that functions shall stay as simple as possible. The filtering can easily be done outside of getElementsWithinRange. We should prefer modularity instead of making simple things complicated.

@Fernando-A-Rocha Fernando-A-Rocha deleted the get-elements-range-arg branch March 7, 2025 11:47
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