Environment & IGameDef code refactoring #4985

nerzhul · 2017-01-02T22:19:33Z

Commit 1

Split ClientEnvironment & ServerEnvironment from environment.h (better code separation & more friendly with IDE because those files are very huge)
environment.cpp/h now only contains common environment interface
Cleanup includes & class declarations in client & server environment to improve build speed
ServerEnvironment::m_gamedef is now a pointer to Server instead of IGameDef, permitting to cleanup many casts.

Commit 2

Move ITextureSource* IGameDef::getTextureSource() to Client only.

Also move ITextureSource *IGameDef::tsrc() helper

This needs a huge code refactor to use Client* instead of IGameDef* client side to use the right object type everywhere

This permits to remove some irrlicht implicit dependency server side

Commit 3

IGameDef cleanup & misc cleanups

drop getShaderSource, getSceneManager, getSoundManager & getCamera abstract call
drop unused emerge() call
cleanup server unused functions (mentionned before)
Drop one unused parameter from ContentFeatures::updateTextures

Commit 4

Rename all Server *gamedef & Client *gamedef

Server *game => Server *server
Client *gamedef => Client *client

This also shows that game.cpp sent sometimes Client + InventoryManager + IGameDef in same functions but it's the same objects

Commit 5

Remove duplicate Client pointer in GUIFormSpecMenu::GUIFormSpecMenu

Commit 6

move checkLocalPrivilege to Client
Remove some unnecessary casts
create_formspec_menu: remove IWritableTextureSource pointer, as client already knows it
Fix some comments

nerzhul · 2017-01-02T22:30:13Z

Note: i didn't fixed the code style issues, it's a pure code move

rubenwardy · 2017-01-02T23:04:14Z

As a guess the abstraction of IGameDef is there for a reason - to discourage people from getting the Server or Client

nerzhul · 2017-01-02T23:16:21Z

IGameDef is a real problem as it share some common unused attributes, like shadersource (client only), emerge (server only) and many other client things. I will push another commit to cleanup some useless functions

Also IGameDef add some irrlicht types declared to server, if at a moment we want to move outside irrlicht it's also a problem solved

nerzhul · 2017-01-02T23:31:11Z

I think it's ready for a review.

To resume:

Enhance compilation times
Environment source split for client & server
IGameDef cleanups which trigger type modifications in many code parts to reflect the real objects types
Remove some dead functions & comments

nerzhul · 2017-01-03T13:30:11Z

I addressed some @Zeno- which shows that we sometimes pass duplicates pointers to same objects. All are now fixed.

Please review & merge @sfan5 @Zeno- @sofar @est31

There is no functional change, just re-organization & reducing client-server link + cleanups

juhdanad

This PR should be merged as soon as possible before merge conflicts arise.

juhdanad · 2017-01-03T22:03:17Z

src/clientenvironment.cpp

+ }
+
+ // Protocol v29 make this behaviour obsolete
+ if (((Client*) getGameDef())->getProtoVersion() < 29) {


This should not happen in ClientEnvironment.

juhdanad · 2017-01-03T22:49:12Z

src/gamedef.h

-
- // Only usable on the server. Thread safe if not written while running threads.
- virtual EmergeManager *getEmergeManager() { return NULL; }
+ virtual IRollbackManager* getRollbackManager() { return NULL; }

 // Used on the client
 virtual bool checkLocalPrivilege(const std::string &priv)


This should be removed from there, since it is only overridden on the client and used by the camera.

exact, this should be moved to client

juhdanad · 2017-01-03T22:54:05Z

src/gamedef.h

- virtual ITextureSource* getTextureSource()=0;
-
- virtual IShaderSource* getShaderSource()=0;
-
 // Used for keeping track of names/ids of unknown nodes
 virtual u16 allocateUnknownNodeId(const std::string &name)=0;

 // Only usable on the client


This comment should be removed because the server has an event manager, too.

juhdanad · 2017-01-03T22:57:08Z

src/guiFormSpecMenu.cpp

@@ -81,13 +81,12 @@ static unsigned int font_line_height(gui::IGUIFont *font)
 GUIFormSpecMenu::GUIFormSpecMenu(irr::IrrlichtDevice* dev,
 JoystickController *joystick,
 gui::IGUIElement* parent, s32 id, IMenuManager *menumgr,
- InventoryManager *invmgr, IGameDef *gamedef,
+ Client *client,


The texture source is also in the client (and the device too, but there is no getter for that).

client->tsrc() ? if i remember it's the function

in fact i check TextureSource is needed here because one constructor was called and only knows texture source, not client

juhdanad · 2017-01-03T23:01:56Z

src/itemdef.cpp

@@ -543,7 +543,7 @@ class CItemDefManager: public IWritableItemDefManager
 request = m_get_clientcached_queue.pop();

 m_get_clientcached_queue.pushResult(request,
- createClientCachedDirect(request.key, gamedef));
+ createClientCachedDirect(request.key, (Client *)gamedef));


gamedef should be client. This function is called only from the Game object, and it already supplies the client as a parameter.

right i forgot it

in fact no, at this moment it's not possible to change this because we depend on IWritableItemDefManager which is client & server side

juhdanad · 2017-01-03T23:06:00Z

src/nodedef.cpp

- IShaderSource *shdsrc = gamedef->getShaderSource();
- scene::ISceneManager* smgr = gamedef->getSceneManager();
+
+ Client *client = (Client *)gamedef;


The parameter type should be Client. This is called only once by the client.

here it's not possible, please look at caller, it's a IGamedef because this function is at this moment shared between client and server

juhdanad · 2017-01-03T23:18:11Z

src/serverenvironment.cpp

+ ScopeProfiler sp(g_profiler, "SEnv: handle players avg", SPT_AVG);
+ for (std::vector<RemotePlayer *>::iterator i = m_players.begin();
+ i != m_players.end(); ++i) {
+ RemotePlayer *player = dynamic_cast<RemotePlayer *>(*i);


This cast is not necessary.

not covered by this PR exactly because it's code move but yes

juhdanad · 2017-01-03T23:18:34Z

src/serverenvironment.cpp

+ std::vector<v3s16> players_blockpos;
+ for (std::vector<RemotePlayer *>::iterator i = m_players.begin();
+ i != m_players.end(); ++i) {
+ RemotePlayer *player = dynamic_cast<RemotePlayer *>(*i);


This cast can be removed, too.

like previous comment

nerzhul · 2017-01-04T06:58:00Z

@juhdanad i fixed all possible points which doesn't need more huge refactoring
For the last points needing more refactoring, we will see in another PR, to limit the changeset size, which is already big

sofar · 2017-01-04T07:58:39Z

Can't compile (master with this PR git am'd):

In file included from /home/sofar/git/minetest/src/clientenvironment.h:24:0,
                 from /home/sofar/git/minetest/src/collision.cpp:26:
/home/sofar/git/minetest/src/clientobject.h:43:4: error: ‘IrrlichtDevice’ has not been declared
    IrrlichtDevice *irr){}
    ^
/home/sofar/git/minetest/src/clientobject.h:54:17: error: ‘ISceneNode’ in namespace ‘irr::scene’ does not name a type
  virtual scene::ISceneNode *getSceneNode(){return NULL;}
                 ^
/home/sofar/git/minetest/src/clientobject.h:55:17: error: ‘IMeshSceneNode’ in namespace ‘irr::scene’ does not name a type
  virtual scene::IMeshSceneNode *getMeshSceneNode(){return NULL;}
                 ^
/home/sofar/git/minetest/src/clientobject.h:56:17: error: ‘IAnimatedMeshSceneNode’ in namespace ‘irr::scene’ does not name a type
  virtual scene::IAnimatedMeshSceneNode *getAnimatedMeshSceneNode(){return NULL;}
                 ^
/home/sofar/git/minetest/src/clientobject.h:58:17: error: ‘IBillboardSceneNode’ in namespace ‘irr::scene’ does not name a type
  virtual scene::IBillboardSceneNode *getSpriteSceneNode(){return NULL;}
                 ^
In file included from /home/sofar/git/minetest/src/collision.cpp:26:0:
/home/sofar/git/minetest/src/clientenvironment.h:67:3: error: ‘IrrlichtDevice’ has not been declared
   IrrlichtDevice *device);
   ^
/home/sofar/git/minetest/src/clientenvironment.h:140:2: error: ‘IrrlichtDevice’ does not name a type
  IrrlichtDevice *m_irr;
  ^
src/CMakeFiles/minetestserver.dir/build.make:2054: recipe for target 'src/CMakeFiles/minetestserver.dir/collision.cpp.o' failed
make[2]: *** [src/CMakeFiles/minetestserver.dir/collision.cpp.o] Error 1

Did I miss an irrlicht update? I'm on archlinux, 1.8.4 irrlicht.

nerzhul · 2017-01-04T08:07:07Z

Which compiler do you use ? I'm also with archlinux

juhdanad · 2017-01-04T10:46:28Z

In my opinion it is good to go. 👍
(as soon as @sofar's compilation error is solved)

nerzhul · 2017-01-04T13:15:35Z

I tested on another computer with g++ it works, testing now with clang++

Works also with clang, i don't understand your compilation error. CI is fine too

@sofar clientenv.h include clientobj.h which include irrlichttypes_extrabloated.h which include if SERVER not def IrrlichtDevice.h

I didn't build server alone, i'm doing it now

nerzhul · 2017-01-04T13:33:35Z

@sofar i forced pushed commit 5 with compilation fix, i missed to re-enable -DBUILD_SERVER=1 on my cmake installation

nerzhul · 2017-01-04T13:37:10Z

i rebased the branch

sofar · 2017-01-04T15:37:39Z

stock arch gcc: gcc version 5.3.0 (GCC)

nerzhul · 2017-01-04T15:39:30Z

@sofar ignore my compiler question is was a compilation problem with -DENABLE_SERVER=1 and now travis build with server

sofar · 2017-01-04T15:40:05Z

compiles now.

nerzhul · 2017-01-04T15:52:55Z

nice ! just wait for your review now

nerzhul · 2017-01-04T22:38:56Z

rebased since recent commits on master @sofar @Zeno- @juhdanad please re-test

juhdanad · 2017-01-04T23:08:36Z

I have reviewed the conflicting files (since the conflict was with my commit).
Everything is at the correct place, this still should be merged. 👍

nerzhul · 2017-01-05T07:26:01Z

Fixed android build in last commit

nerzhul · 2017-01-05T22:04:16Z

ping @sfan5 @est31 this change is requested to make more cleanups & refactoring in environment, please test it, @Zeno- tested it before last rebase and agreed with this. It's needed if we want to make things cleaner and start to have a best OOP model

Zeno- · 2017-01-06T07:47:38Z

Still works fine for me. I agree that separation like this makes sense and it's something that is certainly needed. I'm fine with it.

nerzhul · 2017-01-06T07:54:01Z

@sofar @est31 @sfan5 i will now wait 1 day before merging this, please add an eye on it , it's required to make more cleanups & refactors in the code.

Please don't merge more PR modifying environment during this period

kwolekr · 2017-01-06T07:55:24Z

One big question.

Why did you change all of the IGameDefs to Client or Server? This is wrong.

nerzhul · 2017-01-06T08:11:40Z

In fact for this commit set there is two major reasons: * Drop all unneeded casts in environment * Cleanup Igamedef non common functions between client & serverPlease also note Igamedef was not replaced everywhere, just in this commit set required parts.This commit set is relatively big. If we wanted to re-use Igamedef properly in right places this requires to do more refactoring.

nerzhul · 2017-01-08T09:04:21Z

rebased since last recent changes

* Cleanup includes & class declarations in client & server environment to improve build speed * ServerEnvironment::m_gamedef is now a pointer to Server instead of IGameDef, permitting to cleanup many casts. * Cleanup IGameDef * Move ITextureSource* IGameDef::getTextureSource() to Client only. * Also move ITextureSource *IGameDef::tsrc() helper * drop getShaderSource, getSceneManager, getSoundManager & getCamera abstract call * drop unused emerge() call * cleanup server unused functions (mentionned before) * Drop one unused parameter from ContentFeatures::updateTextures * move checkLocalPrivilege to Client * Remove some unnecessary casts * create_formspec_menu: remove IWritableTextureSource pointer, as client already knows it * Fix some comments * Change required IGameDef to Server/Client pointers * Previous change that game.cpp sometimes calls functions with Client + InventoryManager + IGameDef in same functions but it's the same objects * Remove duplicate Client pointer in GUIFormSpecMenu::GUIFormSpecMenu

sfan5

👍 except mentioned issues

sfan5 · 2017-01-09T15:15:18Z

src/client.h

- { return m_env; }
+ ClientEnvironment& getEnv() { return m_env; }
+ ITextureSource *tsrc() { return getTextureSource(); }
+ ISoundManager *sound() { return getSoundManager(); }


not properly aligned

sfan5 · 2017-01-09T15:15:25Z

src/client.h

 virtual IShaderSource* getShaderSource();
- virtual scene::ISceneManager* getSceneManager();
+ IShaderSource *shsrc() { return getShaderSource(); }


additional spaces

sfan5 · 2017-01-09T15:20:37Z

src/clientmap.cpp

@@ -766,7 +766,7 @@ void ClientMap::renderPostFx(CameraMode cam_mode)
 const ContentFeatures& features = m_nodedef->get(n);
 video::SColor post_effect_color = features.post_effect_color;
 if(features.solidness == 2 && !(g_settings->getBool("noclip") &&
- m_gamedef->checkLocalPrivilege("noclip")) &&
+ ((Client *) m_gamedef)->checkLocalPrivilege("noclip")) &&


ClientMap has an m_client that can be used instead

Exact, i will fix your issues soon, thanks for notice this

sfan5 · 2017-01-09T15:34:59Z

Also:

In file included from /tmp/minetest/src/client.cpp:32:
In file included from /tmp/minetest/src/client.h:24:
/tmp/minetest/src/clientenvironment.h:33:1: warning: class 'PointedThing' was previously declared as a
      struct [-Wmismatched-tags]
class PointedThing;
^
/tmp/minetest/src/util/pointedthing.h:36:8: note: previous use is here
struct PointedThing
       ^
/tmp/minetest/src/clientenvironment.h:33:1: note: did you mean struct here?
class PointedThing;
^~~~~
struct

and

In file included from /tmp/minetest/src/clientmap.cpp:20:
In file included from /tmp/minetest/src/clientmap.h:25:
In file included from /tmp/minetest/src/camera.h:32:
/tmp/minetest/src/client.h:49:1: warning: struct 'PointedThing' was previously declared as a class
      [-Wmismatched-tags]
struct PointedThing;
^
/tmp/minetest/src/clientenvironment.h:33:7: note: previous use is here
class PointedThing;
      ^

nerzhul · 2017-01-09T15:37:28Z

did i missed to push ? it seems travis doesn't have problems... what is your compiler ?

EDIT: Oh it's a warning pointed by clang, will fix, thanks

nerzhul · 2017-01-09T18:32:00Z

@sfan5 i fixed the last points you mentionned and also removed a long time unused function & optimize compilation by not including networkpacket.h in client.h

nerzhul added @ Build CMake, build scripts, official builds, compiler and linker errors @ Server / Client / Env. Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Jan 2, 2017

nerzhul force-pushed the clientenv_split branch from 0d1e597 to aec8aeb Compare January 2, 2017 22:29

nerzhul changed the title ~~Environment code refactoring~~ Environment & IGameDef code refactoring Jan 2, 2017

juhdanad reviewed Jan 3, 2017

View reviewed changes

nerzhul force-pushed the clientenv_split branch from f05fb14 to 1435e12 Compare January 4, 2017 13:33

nerzhul force-pushed the clientenv_split branch from 1435e12 to cfa2599 Compare January 4, 2017 13:36

nerzhul force-pushed the clientenv_split branch from cfa2599 to 1c4cfb2 Compare January 4, 2017 14:14

nerzhul force-pushed the clientenv_split branch from 1c4cfb2 to 922c99e Compare January 4, 2017 22:36

nerzhul force-pushed the clientenv_split branch from 922c99e to 9ed543f Compare January 4, 2017 22:51

nerzhul force-pushed the clientenv_split branch from 9ed543f to cd28a1e Compare January 5, 2017 07:21

Zeno- added the One approval ✅ ◻️ label Jan 6, 2017

nerzhul force-pushed the clientenv_split branch from cd28a1e to 0fb6ab0 Compare January 8, 2017 09:03

nerzhul force-pushed the clientenv_split branch from 0fb6ab0 to 4734317 Compare January 8, 2017 11:58

sfan5 requested changes Jan 9, 2017

View reviewed changes

drop ClientMap::sectorWasDrawn which is unused + various @sfan5 points.

068ddbe

sfan5 approved these changes Jan 9, 2017

View reviewed changes

nerzhul added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Jan 9, 2017

nerzhul merged commit 8e7449e into minetest:master Jan 9, 2017

nerzhul deleted the clientenv_split branch June 6, 2017 22:28

Environment & IGameDef code refactoring #4985

Environment & IGameDef code refactoring #4985

Conversation

nerzhul commented Jan 2, 2017 • edited Loading

Commit 1

Commit 2

Commit 3

Commit 4

Commit 5

Commit 6

nerzhul commented Jan 2, 2017

rubenwardy commented Jan 2, 2017 • edited Loading

nerzhul commented Jan 2, 2017 • edited Loading

nerzhul commented Jan 2, 2017 • edited Loading

nerzhul commented Jan 3, 2017

juhdanad left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerzhul commented Jan 4, 2017 • edited Loading

sofar commented Jan 4, 2017

nerzhul commented Jan 4, 2017 via email

juhdanad commented Jan 4, 2017

nerzhul commented Jan 4, 2017 • edited Loading

nerzhul commented Jan 4, 2017

nerzhul commented Jan 4, 2017

sofar commented Jan 4, 2017

nerzhul commented Jan 4, 2017

sofar commented Jan 4, 2017

nerzhul commented Jan 4, 2017

nerzhul commented Jan 4, 2017

juhdanad commented Jan 4, 2017

nerzhul commented Jan 5, 2017

nerzhul commented Jan 5, 2017 • edited Loading

Zeno- commented Jan 6, 2017

nerzhul commented Jan 6, 2017 • edited Loading

kwolekr commented Jan 6, 2017

nerzhul commented Jan 6, 2017 via email • edited Loading

nerzhul commented Jan 8, 2017

sfan5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfan5 Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfan5 commented Jan 9, 2017

nerzhul commented Jan 9, 2017 • edited Loading

nerzhul commented Jan 9, 2017

nerzhul commented Jan 2, 2017 •

edited

Loading

rubenwardy commented Jan 2, 2017 •

edited

Loading

nerzhul commented Jan 2, 2017 •

edited

Loading

nerzhul commented Jan 2, 2017 •

edited

Loading

nerzhul commented Jan 4, 2017 •

edited

Loading

nerzhul commented Jan 4, 2017 •

edited

Loading

nerzhul commented Jan 5, 2017 •

edited

Loading

nerzhul commented Jan 6, 2017 •

edited

Loading

nerzhul commented Jan 6, 2017 via email •

edited

Loading

sfan5 Jan 9, 2017 •

edited

Loading

nerzhul commented Jan 9, 2017 •

edited

Loading