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.

Sign up for GitHub

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

Make smallest area concept optional #80

Merged
merged 12 commits into from
Oct 1, 2024
Merged

Make smallest area concept optional #80

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
aa14c9d
check interact permission on smallest area at position
SwissalpS Sep 2, 2024
1b86480
expose getSmallestAreaAtPos()
SwissalpS Sep 3, 2024
fd6d7e4
also return area id
SwissalpS Sep 3, 2024
56763e1
cleanup as suggested by SmallJoker
SwissalpS Sep 8, 2024
2d15113
Merge branch 'master' of https://github.com/minetest-mods/areas
SwissalpS Sep 17, 2024
68e8237
make smallest area concept optional
SwissalpS Sep 17, 2024
e50cc25
revised description of setting
SwissalpS Sep 22, 2024
71ae3c0
de-duplicate code using table
SwissalpS Sep 22, 2024
1f770bc
revised settingtypes.txt thanks SmallJoker
SwissalpS Sep 23, 2024
a4a4ff3
whitespace tweak
SwissalpS Sep 24, 2024
2b43015
shorten code and indicate why var is needed
SwissalpS Sep 24, 2024
321b96b
Merge branch 'optionalSmallestAreaUsingTable'
SwissalpS Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 46 additions & 18 deletions api.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,37 +114,65 @@ function areas:getSmallestAreaAtPos(pos)
end

-- Checks if the area is unprotected, open, owned by player
-- or player is part of faction of smallest area at position.
-- or player is part of faction of [smallest] area at position.
function areas:canInteract(pos, name)
if minetest.check_player_privs(name, self.adminPrivs) then
return true
end
local area = self:getSmallestAreaAtPos(pos)
-- No area, player owns it or area is open
if not area
or area.owner == name
or area.open
then
return true
elseif areas.factions_available and area.faction_open then
if (factions.version or 0) < 2 then
local faction_name = factions.get_player_faction(name)
if faction_name then
if areas.config.use_smallest_area_precedence then
local area = self:getSmallestAreaAtPos(pos)
-- No area, player owns it or area is open
if not area
or area.owner == name
or area.open
then
return true
elseif areas.factions_available and area.faction_open then
if (factions.version or 0) < 2 then
local faction_name = factions.get_player_faction(name)
if faction_name then
for _, fname in ipairs(area.faction_open or {}) do
if faction_name == fname then
return true
end
end
end
else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is duplicated. Please offload it to a function or find out a smarter way.

Hint:

local arealist
if areas.config.use_smallest_area_precedence then
	arealist[???] = self:getSmallestAreaAtPos(pos)
else
	arealist = self:getAreasAtPos(pos)
end

for _, area in pairs(arealist) do

end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of code is used a lot, so I took the time to write two variants and benchmark them. The results did not show any clear difference in execution speed on my old laptop in a devtest game with only areas and playerfactions mods.
To keep it simple the test world had no factions or areas defined at all.
Variant A (function):
https://github.com/SwissalpS/minetest_areas/blob/665d308c99bf771d77ef738c17c97d347039800e/api.lua#L118-L171

branch function

Variant B (table):
https://github.com/SwissalpS/minetest_areas/blob/71ae3c0a6ca80698f364142d80342bceb7a48214/api.lua#L118-L159

branch table

Variant B uses 12 lines less, I prefer Variant A by a slim margin for its readability, what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether to pick A (helper func) or B (new table) is entirely subjective. Though, I find it interesting that the local function showed no performance impact. LuaJIT might realize that none of the caller's function stack is needed, thus resulting in a negligible performance difference.

I prefer variant B (L126-128 could be removed too) for having more linear code flow (top to bottom) and smaller branches (as in LoC).

Your preference - variant A - is also OK. It could further be reduced in line count by moving the remaining access checks into the helper function (name would be subject to change).

Implement what you think is easier to understand. I'll be fine with either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it interesting that the local function showed no performance impact.

Maybe my computer has way to much on its hands to be a good platform for benchmarks. Other processes might influence the results.

LuaJIT might realize that none of the caller's function stack is needed

I don't think my setup is using LuaJIT. On another repo I was using table.pack which the CI of github didn't have and somebody comented that LuaJIT doesn't have that.

L126-128 could be removed

OK, I did feel it made clearer what was going on, but it works since my other concern has disappeared with these changes.
Instead I added ", _" in line 124 to make clear why the variable smallest_area is needed:

local smallest_area, _ = self:getSmallestAreaAtPos(pos)

for _, fname in ipairs(area.faction_open or {}) do
if faction_name == fname then
if factions.player_is_in_faction(fname, name) then
return true
end
end
end
else
for _, fname in ipairs(area.faction_open or {}) do
if factions.player_is_in_faction(fname, name) then
return true
end
return false
else
local owned = false
for _, area in pairs(self:getAreasAtPos(pos)) do
if area.owner == name or area.open then
return true
elseif areas.factions_available and area.faction_open then
if (factions.version or 0) < 2 then
local faction_name = factions.get_player_faction(name)
if faction_name then
for _, fname in ipairs(area.faction_open or {}) do
if faction_name == fname then
return true
end
end
end
else
for _, fname in ipairs(area.faction_open or {}) do
if factions.player_is_in_faction(fname, name) then
return true
end
end
end
end
owned = true
end
return not owned
end
return false
end

-- Returns a table (list) of all players that own an area
Expand Down
3 changes: 3 additions & 0 deletions settingtypes.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
# Static paths do not work well with settings
#areas.filename (Configuration file path) string (world_path)/areas.dat

# Use smallest area volume precedence concept. (experimental may change)
SwissalpS marked this conversation as resolved.
Show resolved Hide resolved
areas.use_smallest_area_precedence (Smallest area rules) bool false

# Allow players with a privilege create their own areas using /protect
# within the specified size and amount limits.
areas.self_protection (Self protection) bool false
Expand Down
Loading