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

Add option to also check the center to `find_node_near` #5255

Merged
merged 3 commits into from May 4, 2017

Conversation

@red-001
Copy link
Contributor

red-001 commented Feb 17, 2017

Adds the feature requested in #5213.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Feb 17, 2017

Does 'search centre' default to false if the bool is missing?

@@ -611,7 +611,7 @@ int ModApiEnvMod::l_find_node_near(lua_State *L)
ndef->getIds(lua_tostring(L, 3), filter);
}

for(int d=1; d<=radius; d++){
for(int d = !lua_toboolean(L, 4); d <= radius; d++){

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Feb 17, 2017

Member

this is unreadable

@red-001

This comment has been minimized.

Copy link
Contributor Author

red-001 commented Feb 17, 2017

Yes it defaults to false

@red-001 red-001 force-pushed the red-001:center_check branch from a4d0e54 to 7f0a074 Feb 17, 2017

@@ -2233,6 +2233,7 @@ and `minetest.auth_reload` call the authetification handler.
* `minetest.find_node_near(pos, radius, nodenames)`: returns pos or `nil`
* `radius`: using a maximum metric
* `nodenames`: e.g. `{"ignore", "group:tree"}` or `"default:dirt"`
* `search_centre`: If true `pos` is also checked for the nodes

This comment has been minimized.

Copy link
@kaeza

kaeza Feb 17, 2017

Contributor

Indent by four spaces instead of a tab.

@@ -611,7 +611,8 @@ int ModApiEnvMod::l_find_node_near(lua_State *L)
ndef->getIds(lua_tostring(L, 3), filter);
}

for(int d=1; d<=radius; d++){
int start_at = !lua_toboolean(L, 4);

This comment has been minimized.

Copy link
@kaeza

kaeza Feb 17, 2017

Contributor

int start_at = lua_toboolean(...) ? 0 : 1; would be clearer IMHO.

I think that's what @rubenwardy meant by "unreadable".

This comment has been minimized.

Copy link
@paramat

paramat Feb 17, 2017

Member

Yes kaeza's suggestion here is better. Also a better variable name would be good, 'min_radius' or 'start_radius'?

@red-001 red-001 force-pushed the red-001:center_check branch 4 times, most recently from f267e27 to 78fa290 Feb 17, 2017

@@ -2233,6 +2233,7 @@ and `minetest.auth_reload` call the authetification handler.
* `minetest.find_node_near(pos, radius, nodenames)`: returns pos or `nil`

This comment has been minimized.

Copy link
@paramat

paramat Feb 21, 2017

Member

'search centre' needs to be included here in square brackets to show it is optional, like:

* `minetest.set_mapgen_setting(name, value, [override_meta])`
@@ -2233,6 +2233,7 @@ and `minetest.auth_reload` call the authetification handler.
* `minetest.find_node_near(pos, radius, nodenames)`: returns pos or `nil`
* `radius`: using a maximum metric
* `nodenames`: e.g. `{"ignore", "group:tree"}` or `"default:dirt"`
* `search_centre`: If true `pos` is also checked for the nodes

This comment has been minimized.

Copy link
@paramat

paramat Feb 21, 2017

Member

Need to state it is optional and defaults to 'false', like:

* `override_meta` is an optional boolean (default: `false`).
@SmallJoker

This comment has been minimized.

Copy link
Member

SmallJoker commented Mar 5, 2017

@paramat

Like all tests in Lua, lua_toboolean returns 1 for any Lua value different from false and nil; otherwise it returns 0. It also returns 0 when called with a non-valid index.

According to this sentence in the Lua reference manual, this will work as expected.

v3s16 p = pos + (*i);
content_t c = env->getMap().getNodeNoEx(p).getContent();
if(filter.count(c) != 0){
if(filter.count(c) != 0) {

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Mar 5, 2017

Member

If already cleaning up the code style, also add spaces after if, for and while.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Apr 14, 2017

Tested?

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Apr 21, 2017

@red-001 Is it tested? Needs to be tested with no bool argument to check it behaves as before in this situation.

@@ -116,7 +116,7 @@ class ModApiEnvMod : public ModApiBase {
// get_day_count() -> int
static int l_get_day_count(lua_State *L);

// find_node_near(pos, radius, nodenames) -> pos or nil
// find_node_near(pos, radius, nodenames, search_centre) -> pos or nil

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 21, 2017

Member

center instead of centre ?

This comment has been minimized.

Copy link
@paramat

paramat Apr 21, 2017

Member

Yes on search code uses 'center' throughout. 'centre' is only used occasionally in docs and comments (mostly by me).

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 22, 2017

Member

centre is french for center :p

This comment has been minimized.

Copy link
@tenplus1

tenplus1 Apr 24, 2017

Contributor

In geometry, a centre of an object is a point in some sense in the middle of the object.

This comment has been minimized.

Copy link
@paramat

paramat Apr 24, 2017

Member

So for consistency please change to 'center'.

@@ -612,27 +612,28 @@ int ModApiEnvMod::l_find_node_near(lua_State *L)
v3s16 pos = read_v3s16(L, 1);
int radius = luaL_checkinteger(L, 2);
std::set<content_t> filter;
if(lua_istable(L, 3)){
if(lua_istable(L, 3)) {

This comment has been minimized.

Copy link
@paramat

paramat Apr 24, 2017

Member

space after 'if'

int table = 3;
lua_pushnil(L);
while(lua_next(L, table) != 0){
while(lua_next(L, table) != 0) {

This comment has been minimized.

Copy link
@paramat

paramat Apr 24, 2017

Member

space after 'while'

// key at index -2 and value at index -1
luaL_checktype(L, -1, LUA_TSTRING);
ndef->getIds(lua_tostring(L, -1), filter);
// removes value, keeps key for next iteration
lua_pop(L, 1);
}
} else if(lua_isstring(L, 3)){
} else if(lua_isstring(L, 3)) {

This comment has been minimized.

Copy link
@paramat

paramat Apr 24, 2017

Member

space after 'if'

@@ -599,7 +599,7 @@ int ModApiEnvMod::l_get_gametime(lua_State *L)
}


// find_node_near(pos, radius, nodenames) -> pos or nil
// find_node_near(pos, radius, nodenames, search_centre) -> pos or nil

This comment has been minimized.

Copy link
@paramat

paramat Apr 24, 2017

Member

'center'

* `radius`: using a maximum metric
* `nodenames`: e.g. `{"ignore", "group:tree"}` or `"default:dirt"`
* `search_centre`: If true `pos` is also checked for the nodes

This comment has been minimized.

Copy link
@paramat

paramat Apr 24, 2017

Member

'center'

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Apr 24, 2017

Line comments: code syle and spelling of 'center'. Once done and tested as requested above #5255 (comment) +1.

Tenplus1 you should bug the author for this not the core devs :)

@SmallJoker SmallJoker force-pushed the red-001:center_check branch from 6276f06 to 2f41a12 Apr 25, 2017

@@ -612,27 +612,27 @@ int ModApiEnvMod::l_find_node_near(lua_State *L)
v3s16 pos = read_v3s16(L, 1);
int radius = luaL_checkinteger(L, 2);
std::set<content_t> filter;
if(lua_istable(L, 3)){
int table = 3;

This comment has been minimized.

Copy link
@paramat

paramat Apr 25, 2017

Member

Why removed?

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Apr 26, 2017

Member

Consistency of the arguments with the other Lua function calls and there's no noticeable speed difference in caching this index.

This comment has been minimized.

Copy link
@paramat
@paramat

This comment has been minimized.

Copy link
Member

paramat commented Apr 26, 2017

Tested? "Needs to be tested with no bool argument to check it behaves as before in this situation."
If so +1.

@red-001

This comment has been minimized.

Copy link
Contributor Author

red-001 commented May 3, 2017

@red-001 red-001 force-pushed the red-001:center_check branch from 2f41a12 to e5a4f60 May 3, 2017

@paramat paramat added One approval and removed One approval labels May 3, 2017

@paramat

This comment has been minimized.

Copy link
Member

paramat commented May 3, 2017

See line comment above about lua_api.txt:

Needs to state it is optional and defaults to 'false', like:

  • override_meta is an optional boolean (default: false).
ndef->getIds(lua_tostring(L, 3), filter);
}

for (int d=1; d<=radius; d++){
int start_radius = lua_toboolean(L, 4) ? 0 : 1;

This comment has been minimized.

Copy link
@paramat

paramat May 3, 2017

Member

Tertiary operators have the conditional in brackets:
int start_radius = (lua_toboolean(L, 4)) ? 0 : 1;

This comment has been minimized.

Copy link
@Zeno-

Zeno- May 4, 2017

Contributor

Why? I find that a lot harder to read compared to the current form

This comment has been minimized.

Copy link
@paramat

paramat May 4, 2017

Member

Both work, but our code always has the conditional in brackets in tertiary operators, so i guess just for consistency. I somewhat agree about the readability.

sfan5 added some commits May 4, 2017

@sfan5

sfan5 approved these changes May 4, 2017

@sfan5 sfan5 added the One approval label May 4, 2017

@sfan5

This comment has been minimized.

Copy link
Member

sfan5 commented May 4, 2017

@paramat ready for re-review

@paramat

This comment has been minimized.

Copy link
Member

paramat commented May 4, 2017

👍

@nerzhul nerzhul added this to the 0.4.16 milestone May 4, 2017

@nerzhul

nerzhul approved these changes May 4, 2017

@nerzhul nerzhul merged commit d6cf545 into minetest:master May 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@red-001 red-001 deleted the red-001:center_check branch May 4, 2017

@nerzhul nerzhul added this to Done in Minetest 0.4.16 May 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.