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

Get rid of recursive call in default.dig_up #3133

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

Conversation

Emojigit
Copy link
Contributor

Fixes #3075

This PR fixes the recursive calling of default.dig_up and adds a parameter for a maximum number of nodes searched.

This PR is ready for review.

mods/default/functions.lua Outdated Show resolved Hide resolved
mods/default/functions.lua Outdated Show resolved Hide resolved
mods/default/functions.lua Outdated Show resolved Hide resolved
@Emojigit Emojigit requested a review from sfan5 June 17, 2024 10:53
@Emojigit
Copy link
Contributor Author

@sfan5 Change applied in 0e79158. Ready for review again.

@Emojigit Emojigit requested a review from sfan5 June 17, 2024 22:32
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

works

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 print in minetest.dig_up while digging papyrus. Height limit works too.

mods/default/functions.lua Show resolved Hide resolved
mods/default/functions.lua Outdated Show resolved Hide resolved
@Emojigit Emojigit requested a review from SmallJoker June 22, 2024 06:45
@Emojigit Emojigit requested a review from appgurueu June 25, 2024 11:08
Comment on lines +319 to +322
minetest.log("error", "Error raised during `default.dig_up` call:")
for line in debug.traceback(...):gmatch("([^\n]*)\n?") do
minetest.log("error", line)
end
Copy link
Member

@SmallJoker SmallJoker Jun 26, 2024

Choose a reason for hiding this comment

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

As an alternative to manual parsing and outputting - might minetest.error_handler(...) produce better output?

EDIT: This should work: local noerr, success = xpcall(function() bla bla end, minetest.error_handler) and then use the returned values to decide whether or not to reset in_dig_up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weirdly, using minetest.error_handler does not print anything about the error... IDK why.

for line in debug.traceback(...):gmatch("([^\n]*)\n?") do
minetest.log("error", line)
end
end)
Copy link
Member

@sfan5 sfan5 Jun 26, 2024

Choose a reason for hiding this comment

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

not sure if to call this clever or a hack but here's a cleaner suggestion:

local in_dig_up = {}
setmetatable(in_dig_up, {__mode = "k"})

function default.dig_up(pos, node, digger, max_height)
	local sentinel = {}
	if in_dig_up[sentinel] then return end
	-- ...
	in_dig_up[sentinel] = true
	-- ..
	-- no resetting needed
end

it's even re-entrant
what is going on here? -> https://www.lua.org/pil/17.html

Copy link
Member

Choose a reason for hiding this comment

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

This is a beautiful hack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stack overflow due to default.dig_up
4 participants