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

VoxelManip: Abort if an argument is missing instead of silently doing nothing #7567

Merged
merged 3 commits into from Aug 2, 2018

Conversation

HybridDog
Copy link
Contributor

@HybridDog HybridDog commented Jul 19, 2018

Before this PR, you could e.g. call the calc_lighting for a non-mapgen vmanip object and nothing happened.
Now it aborts instead, the modder is informed that they did something wrong.

@SmallJoker
Copy link
Member

I don't think that an error is appropriate here. A warning would be sufficient.

@HybridDog
Copy link
Contributor Author

if (!vm->m_area.contains(VoxelArea(pmin, pmax)))
throw LuaError("Specified voxel area out of VoxelManipulator bounds");
An error is also used when wrong position arguments were passed in the current code.

@sfan5
Copy link
Member

sfan5 commented Jul 19, 2018

I would downgrade the "called for a non-mapgen VoxelManip" to warnings, having missing/wrong parameters as errors is fine IMO.

@rubenwardy
Copy link
Member

I'm not sure this is a good idea. Especially as using set_data without an argument is perfectly valid, and should be solved by documentation anyway

@HybridDog
Copy link
Contributor Author

HybridDog commented Jul 19, 2018

Especially as using set_data without an argument is perfectly valid, and should be solved by documentation anyway

I think calling set_data without an argument is slower than testing if the argument is missing before calling it in lua because of less lua to core switches. According to lua_api, an argument should be passed:

* `set_data(data)`: Sets the data contents of the `VoxelManip` object

I would downgrade the "called for a non-mapgen VoxelManip" to warnings, having missing/wrong parameters as errors is fine IMO.

I agree because I think there may be some edge-cases where a vmanip object is passed to some commonly used function.

@paramat
Copy link
Contributor

paramat commented Jul 19, 2018

using set_data without an argument is perfectly valid

Is it? It doesn't fallback to anything, just does 'return 0'.

This PR is ok for me with warnings or errors.

@HybridDog
Copy link
Contributor Author

I would downgrade the "called for a non-mapgen VoxelManip" to warnings, having missing/wrong parameters as errors is fine IMO.

done

@paramat paramat added the Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements label Jul 25, 2018
@paramat
Copy link
Contributor

paramat commented Jul 25, 2018

👍 assuming it's tested.

@@ -91,7 +91,7 @@ int LuaVoxelManip::l_set_data(lua_State *L)
MMVManip *vm = o->vm;

if (!lua_istable(L, 2))
return 0;
throw LuaError("set_data called with missing parameter");
Copy link
Contributor

Choose a reason for hiding this comment

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

"VoxelManip:set_data() called with ..."

Same for the other throw LuaError lines?

Copy link
Contributor Author

@HybridDog HybridDog Jul 26, 2018

Choose a reason for hiding this comment

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

Thanks, this makes it clear that the message refers to vmanip. I did not add the parenthesis because I think they would be confusing when the message is about missing parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops yes this clarification is important.

@paramat
Copy link
Contributor

paramat commented Jul 26, 2018

👍

@paramat paramat merged commit 741e3ef into minetest:master Aug 2, 2018
@HybridDog HybridDog deleted the vmanip_argabort branch August 3, 2018 15:30
ClobberXD pushed a commit to ClobberXD/minetest that referenced this pull request Aug 17, 2018
…ng (minetest#7567)

Error on missing parameter.
Warning when using a method on the incorrect type of LuaVoxelManip.
osjc pushed a commit to osjc/minetest that referenced this pull request Jan 23, 2019
…ng (minetest#7567)

Error on missing parameter.
Warning when using a method on the incorrect type of LuaVoxelManip.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements @ Script API >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants