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

Make smallest area concept optional #80

Merged
merged 12 commits into from
Oct 1, 2024

Conversation

SwissalpS
Copy link
Contributor

The diff may make it look like much changed, but I only wrapped current and previous behaviour into an if then else clause and added the setting in settingtypes.txt.

api.lua Outdated
Comment on lines 130 to 140
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)

settingtypes.txt Outdated Show resolved Hide resolved
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Works. Tested with setting = false and = true, local server.

Will merge in a few days unless there are objections.

@SmallJoker SmallJoker merged commit d2b227e into minetest-mods:master Oct 1, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants