Clean up l_object.cpp #10512

Zughy · 2020-10-17T21:26:39Z

So, I've decided cleaning up MT code on the go. I think a consistent ordered code is vital both for core devs and for possible contributors. This is, in short, what I did:

consistent names
- ObjectRef = obj
- ServerActiveObject = sao
- LuaEntitySAO = entitysao
- PlayerSAO = playersao
- RemotePlayer = player
consistent structure: check for object, variables declaration, execution+return. These sections are separated by an empty line. When functions were too long, the execution got split up accordingly.
removed all NULL checks. Now there are nullptrs instead, as I find it more straightforward than !something
removed useless comments, e.g. //Do it
removed is_player_connected, a deprecated method that wasn't even listed in the API
descriptions above functions now match the API
removed info about parameters in the functions description: they're already explained in the API
edited the API where needed
simplified a few methods to make them more readable e.g. set_bone_position
added missing Lua checks ( partially solves Many Lua C functions are missing type checks #1794 )

Things I'm not sure about, core dev help needed:

~~how to name p in set_sprite(...)~~
the old -1 value of HUD IDs, returned when ID was not a number. I had a look at the code and it doesn't look to me was meaningful at all so I simply removed it, but a double check is always better
~~debug lines at 2249 and 2257. Do you want to keep them?~~
TODO at 483: has it been solved?

To do

This PR is Ready for Review.

How to test

Well: check the code. Also, I might have made a few mistakes as I'm human, even if I double-checked my edits twice. So idk, take a solid game, play it a little bit and see if it all works accordingly.

Lastly, I want to underline how, imho, this should be high priority: I've seen things which were pretty messed up, like edits pushed a couple of weeks ago going completely against the guidelines (the minimap), or useless 8 year old comments who nobody apparently decided to remove. A tidy environment helps a lot. And I've actually had fun doing it, it was relaxing

…factor

doc/lua_api.txt

paramat · 2020-10-17T22:23:22Z

how to name p in set_sprite(...)

set_sprite(p, num_frames, framelength, select_x_by_camera)
p: {x=column number, y=row number}, the coordinate of the first frame default: {x=0, y=0}

How about 'first_frame' or 'start_frame'?
A comma should be added in the docs after 'the coordinate of the first frame'.

src/script/lua_api/l_object.cpp

Zughy · 2020-10-18T08:15:50Z

as a non native speaker, start_frame is more intuitive to me. I'll wait for more feedback though

sfan5 · 2020-10-18T11:59:18Z

how to name p in set_sprite(...)

frame_pos or sprite_pos

debug lines at 2249 and 2257. Do you want to keep them?

No, anyone can write them again if they need them.

Regarding the renaming: I think it's useful to change the variable if it's misnamed (e.g. ServerActiveObject *co), but changing
o to obj in a short function is not an improvement.
Also I don't think !player to player == nullptr is an improvement.

doc/lua_api.txt

Zughy · 2020-10-18T16:02:57Z

Also I don't think !player to player == nullptr is an improvement.

It was simply a matter of choosing one or the other, as the code featured every variation of it (a lot of them also missed the new line for the body). Relevant discussion: #10480 (comment). As someone who's quite a beginner in C++, nullptr helps me understand better all the pointers thing, that's why I opted for it

doc/lua_api.txt

sfan5 · 2020-10-18T17:20:46Z

It was simply a matter of choosing one or the other, as the code featured every variation of it

My point here is that changing bad style (x == NULL) to good style (e.g. x == nullptr) is useful, but changing good style (!x) to a different good style is not useful.

As someone who's quite a beginner in C++, nullptr helps me understand better all the pointers thing, that's why I opted for it

...just like this wouldn't justify changing !foo to foo == nullptr everywhere in Minetest.

Zughy · 2020-10-18T21:24:01Z

I understand your point now, I'll avoid doing that in the future, I'm sorry (but please don't make me revert the ones I've already edited, it's... a lot)

v-rob · 2020-10-19T17:06:23Z

Reverting shouldn't be necessary at this point; it's not like it's a very hard change to review, even if there are a lot.

Also, I think start_frame or start_pos is pretty good. It doesn't matter a whole lot, any of the given suggestions are better than p.

v-rob

Looks pretty good. I don't like the weird type checks everywhere, but that's better solved with better Lua-interfacing functions, like extending readParam and adding something like paramIsType. But that's for another PR, so I won't press inconsistent type checking here.

doc/lua_api.txt

src/script/lua_api/l_object.cpp

v-rob · 2020-10-19T18:16:42Z

src/script/lua_api/l_object.cpp

 if (isNaN(L, 2))
 throw LuaError("ObjectRef::set_yaw: NaN value is not allowed.");

 float yaw = readParam<float>(L, 2) * core::RADTODEG;
- co->setRotation(v3f(0, yaw, 0));
+
+ entitysao->setRotation(v3f(0, yaw, 0));


Question (not related to this cleanup, just a general question), but doesn't setting the other two to 0 reset the pitch and roll of ObjectRef:set_rotation? If that's so, sounds like a bug.

I have a feeling that doing this any other way may mess with XYZ/ZYX ordering - ie: the yaw won't turn out correctly

An issue should probably be created from this

…factor

Zughy · 2020-10-19T19:34:54Z

Thanks v-rob for the in-depth review, all problems have been addressed, and p is now start_frame. I'd like to remind the other 2 doubts of mine:

the old -1 value of HUD IDs, returned when ID was not a number. I had a look at the code and it doesn't look to me was meaningful at all so I simply removed it, but a double check is always better
TODO at 483: has it been solved?

Also, I noticed readParam<bool> and readParam<std::string> take an optional default value, while others don't. Is it normal? (helper.cpp)

…factor

doc/lua_api.txt

v-rob

Looks good. I'm glad to see cleanups and code consistency.

Lejo1 · 2020-10-28T17:48:53Z

As I just recolonized this PR removes the pretty well used player:is_player_connected function and breaks some mods on the way.
What's the replace for it? Is it minetest.is_player?
If yes I think it should be clear that it only returns true if the player is online.

sfan5 · 2020-10-28T18:17:42Z

is_player_connected has been marked deprecated since 5.2.0 (released 9 months ago) and there has been a deprecation warning the whole time.
Where exactly is it well used?

Lejo1 · 2020-10-28T18:54:39Z

Where exactly is it well used?

That might sound silly. It's just I always used this functions like any other so I expected it to be official. I learned minetest modding mostly by doing so I must have seen the function somewhere else.
Anyhow such changes are needed. Is minetest.is_player now the official replacement?
EDIT: Or is it just completely removed and my maybe dirty way of storing it using minetst.after and using it later is not anymore supported?

sfan5 · 2020-10-29T17:52:33Z

Is minetest.is_player now the official replacement?

is_player does not do the same thing.
If you absolutely have to determine if an ObjectRef is valid you can use e.g. get_yaw() ~= nil, but you should really rework your code to pass along the player name to minetest.after and then retrieve the player again.

Or is it just completely removed and my maybe dirty way of storing it using minetst.after and using it later is not anymore supported?

See here: (section was added with 5.2.0)

minetest/doc/lua_api.txt

Lines 6107 to 6118 in 68cd93b

 ### Advice on handling `ObjectRefs` 

 When you receive an `ObjectRef` as a callback argument or from another API 

 function, it is possible to store the reference somewhere and keep it around. 

 It will keep functioning until the object is unloaded or removed. 

 However, doing this is **NOT** recommended as there is (intentionally) no method 

 to test if a previously acquired `ObjectRef` is still valid. 

 Instead, `ObjectRefs` should be "let go" of as soon as control is returned from 

 Lua back to the engine. 

 Doing so is much less error-prone and you will never need to wonder if the 

 object you are working with still exists.

src/script/lua_api/l_object.cpp

SmallJoker · 2020-11-01T14:36:36Z

src/script/lua_api/l_object.cpp

 std::vector<MinimapMode> modes;
+ s16 selected_mode = luaL_checkint(L, 3);


"2" is a valid number but fails in luaL_checkint.

(Missing type coercion)

src/script/lua_api/l_object.cpp

sfan5 · 2020-11-05T21:50:45Z

src/script/lua_api/l_object.cpp

 return 0;

- u32 id = lua_isnumber(L, 2) ? lua_tonumber(L, 2) : -1;
+ u32 id = luaL_checkint(L, 2);


Assuming Krock's related comment is correct, the issue with luaL_checkint applies here too (strings are valid numbers, but the function won't accept them).

(assuming he's right) why exactly someone should pass a string instead of a number though? The API says nothing about it

This is called type coercion. Lua should convert strings to numbers when used in a numeric context - although, the documents are unclear as to whether this is just with arithmetic or with other contexts too

I mean, that's useful for things like print("this is a number: " .. 55) but I still don't see why someone should pass a string into this function (hud_change). Anyways, a solution could be to have a readParam<int>(...) checking for NaN, applying type coercion and then casting into an int (if it's a decimal).
Also, the previous version was wrong too, because I could have passed a float, and HUDs take no floats

I don't know how important this really is (do other functions consistently take strings as numbers?), especially since you're not meant to do funny things to HUD IDs.

sfan5 · 2020-11-05T21:50:50Z

src/script/lua_api/l_object.cpp

 return 0;

- u32 id = lua_tonumber(L, -1);
+ u32 id = luaL_checkint(L, 2);


Co-authored-by: Zughy <[email protected]>

Zughy added 2 commits October 17, 2020 23:48

massive refactor

cedbf20

Merge branch 'master' of https://github.com/minetest/minetest into re…

62bc6e7

…factor

paramat added Code quality Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Oct 17, 2020

paramat reviewed Oct 17, 2020

View reviewed changes

doc/lua_api.txt Outdated Show resolved Hide resolved

paramat reviewed Oct 17, 2020

View reviewed changes

src/script/lua_api/l_object.cpp Outdated Show resolved Hide resolved

sky_params

bda1555

sfan5 reviewed Oct 18, 2020

View reviewed changes

doc/lua_api.txt Outdated Show resolved Hide resolved

Zughy added 2 commits October 18, 2020 18:51

removed debug messages

a7b115d

updated api

a1d746e

sfan5 reviewed Oct 18, 2020

View reviewed changes

doc/lua_api.txt Outdated Show resolved Hide resolved

tables

da15896

v-rob reviewed Oct 19, 2020

View reviewed changes

Zughy added 3 commits October 19, 2020 21:07

Merge branch 'master' of https://github.com/minetest/minetest into re…

5fc75f3

…factor

fixes

4fcd949

p -> start_frame

9562672

Zughy mentioned this pull request Oct 19, 2020

Set visibility of players/hide them from others #9863

Closed

Zughy added 2 commits October 21, 2020 15:33

Merge branch 'master' of https://github.com/minetest/minetest into re…

46a8d22

…factor

header cleaning

db91138

rubenwardy approved these changes Oct 21, 2020

View reviewed changes

rubenwardy added the One approval ✅ ◻️ label Oct 21, 2020

paramat reviewed Oct 21, 2020

View reviewed changes

doc/lua_api.txt Outdated Show resolved Hide resolved

offset vectors

23f095d

v-rob approved these changes Oct 22, 2020

View reviewed changes

v-rob added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Oct 22, 2020

rubenwardy changed the title ~~l_object.cpp clean up~~ Clean up l_object.cpp Oct 22, 2020

rubenwardy merged commit 33b2c5f into minetest:master Oct 22, 2020

sfan5 mentioned this pull request Oct 23, 2020

deprecated functions should appear by default in the log #10544

Closed

Zughy deleted the refactor branch October 23, 2020 16:32

sfan5 mentioned this pull request Nov 1, 2020

ObjectRef: Re-add legacy code for set_physics_override #10585

Merged

SmallJoker reviewed Nov 1, 2020

View reviewed changes

src/script/lua_api/l_object.cpp Show resolved Hide resolved

SmallJoker reviewed Nov 1, 2020

View reviewed changes

src/script/lua_api/l_object.cpp Show resolved Hide resolved

SmallJoker reviewed Nov 1, 2020

View reviewed changes

src/script/lua_api/l_object.cpp Show resolved Hide resolved

SmallJoker reviewed Nov 1, 2020

View reviewed changes

SmallJoker reviewed Nov 3, 2020

View reviewed changes

src/script/lua_api/l_object.cpp Show resolved Hide resolved

Zughy mentioned this pull request Nov 5, 2020

ObjectRef: fix some v3f checks #10602

Merged

sfan5 reviewed Nov 5, 2020

View reviewed changes

src/script/lua_api/l_object.cpp Show resolved Hide resolved

sfan5 reviewed Nov 5, 2020

View reviewed changes

src/script/lua_api/l_object.cpp

return 0;

u32 id = lua_tonumber(L, -1);

u32 id = luaL_checkint(L, 2);

Copy link

Member

sfan5 Nov 5, 2020

Choose a reason for hiding this comment

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

same

v-rob mentioned this pull request Nov 22, 2020

Prevent the remain rotation angles to be nullified after set_yaw() call. #10662

Closed

HybridDog pushed a commit to HybridDog/minetest that referenced this pull request Dec 17, 2020

Clean up l_object.cpp (minetest#10512)

fc51ad3

Co-authored-by: Zughy <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up l_object.cpp #10512

Clean up l_object.cpp #10512

Zughy commented Oct 17, 2020 •

edited

Loading

paramat commented Oct 17, 2020 •

edited

Loading

Zughy commented Oct 18, 2020 •

edited

Loading

sfan5 commented Oct 18, 2020

Zughy commented Oct 18, 2020 •

edited

Loading

sfan5 commented Oct 18, 2020

Zughy commented Oct 18, 2020

v-rob commented Oct 19, 2020 •

edited

Loading

v-rob left a comment

v-rob Oct 19, 2020

rubenwardy Oct 21, 2020 •

edited

Loading

Zughy commented Oct 19, 2020 •

edited

Loading

v-rob left a comment

Lejo1 commented Oct 28, 2020

sfan5 commented Oct 28, 2020

Lejo1 commented Oct 28, 2020 •

edited

Loading

sfan5 commented Oct 29, 2020

SmallJoker Nov 1, 2020

rubenwardy Nov 3, 2020

sfan5 Nov 5, 2020

Zughy Nov 10, 2020

rubenwardy Nov 10, 2020

Zughy Nov 10, 2020

sfan5 Nov 12, 2020

sfan5 Nov 5, 2020

		std::vector<MinimapMode> modes;
		s16 selected_mode = luaL_checkint(L, 3);

Clean up l_object.cpp #10512

Clean up l_object.cpp #10512

Conversation

Zughy commented Oct 17, 2020 • edited Loading

To do

How to test

paramat commented Oct 17, 2020 • edited Loading

Zughy commented Oct 18, 2020 • edited Loading

sfan5 commented Oct 18, 2020

Zughy commented Oct 18, 2020 • edited Loading

sfan5 commented Oct 18, 2020

Zughy commented Oct 18, 2020

v-rob commented Oct 19, 2020 • edited Loading

v-rob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenwardy Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Zughy commented Oct 19, 2020 • edited Loading

v-rob left a comment

Choose a reason for hiding this comment

Lejo1 commented Oct 28, 2020

sfan5 commented Oct 28, 2020

Lejo1 commented Oct 28, 2020 • edited Loading

sfan5 commented Oct 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zughy commented Oct 17, 2020 •

edited

Loading

paramat commented Oct 17, 2020 •

edited

Loading

Zughy commented Oct 18, 2020 •

edited

Loading

Zughy commented Oct 18, 2020 •

edited

Loading

v-rob commented Oct 19, 2020 •

edited

Loading

rubenwardy Oct 21, 2020 •

edited

Loading

Zughy commented Oct 19, 2020 •

edited

Loading

Lejo1 commented Oct 28, 2020 •

edited

Loading