Jump to content
Eternal Lands Official Forums
Malaclypse

Cleaning up the code

Recommended Posts

While documenting the source, I found some things, that I like to discuss.

 

First there's a file SDL_opengl.h, which is a standard file of libSDL. The version we use is definitely out of sync, that is, it's no longer valid for the current SDL release. It might also be a reason of some instability we encountered during the last weeks, also I can't say this for sure. If you run a diff between the file in ELC and the file distributed with SDL you will notice quite some differences. I didn't check them closely, but it might be that the interface to SDL has changed in the meantime. So, if there's not an important reason to keep this file, I would suggest to remove it from the source tree. I have my client running for several weeks compiled without this file and didn't find anything strange. If we can't remove it for whatever reason, I suggest to at least update it to stay in sync with the current released version of this file.

 

Second, I found some variables (mainly the positional and dimensional variables for all the windows we have, like manufacture_menu_x, options_menu_x_len, etc) that don't need to be declared public. It would be enough to declare them in the responsible source files. There are also some functions that are only used by one or two other functions and imho are therefore better removed from the API and moved to the source files were they are needed. This would clean up the API of the client and can focus the API to contain only those functions, types and variables that are really necessary to be declared public. Note this would not remove any variable or function, it would just change the scope of some of them.

 

If this is of intereset, especially the second issue, I will provide a list of all variables and functions that I would like to move here in the forum to further discuss this topic.

Edited by Malaclypse

Share this post


Link to post
Share on other sites

Well, IMHO anything you do to remove the number of global variables will be most welcomed :D

 

With regards to the SDL_opengl.h: I think it was originally introduced to fix an issue on windows systems. Somethingbeing declared when running linux, but not on windows? Entropy can probably shed more light on this. Or Cicero?

"@(#) $Id: SDL_opengl.h,v 1.3 2004/09/22 22:48:03 cicero Exp $";

Let's change the #include "SDL_opengl.h" statements to #include <SDL_opengl>, but keep the file in the repository. If someone experiences weird things, s/he can easily check if it's caused by using the new header then.

Share this post


Link to post
Share on other sites

Well the only includes of this file are in frustum.c and in particles.c and it doesn't hurt to comment them both. The client will compile without any warning, even if this file is not included explicitly. This is true at least on my linux system. So maybe we can comment out the include statements, and leave the file for a bit longer in the repository, just to be sure. Of course, someone should test this on a windows system to check whether this will work there or not.

 

So I will compile a list of changes for the variables and functions and post it in the next few days.

Share this post


Link to post
Share on other sites

Eh, that file is really unnecessary. I think it defines some things for you if you don't have a glext.h, which apparently Ent didn't have. I've tried to get rid of it several times. Your real GL headers are the only ones you should be using, not the SDL_opengl.h

Share this post


Link to post
Share on other sites

Cicero thanks for this info. Will try to get rid of it then :)

 

So that's what I suggest:

 

Anybody who's involved in programming, please comment the

#include "SDL_opengl.h"

statement at top of the files frustum.c and particles.c, try to recompile and play for some days. If there are no problems with this on either platform after one or two weeks, we can remove those include statements and commit the changes and also remove the SDL_opengl.h file from the clients repository. It can be recovered at any time later, if this will be necessary (the file is still under cvs control).

 

One other point I found with code cleanup topic. Currently there's a wild mix of how macros are written (both constant as well as function macros). Some are complete in upper, some in lower case. Maybe there are even mixed case macros, I'm not sure atm. Imo we should use one style only, both for ease of readability, and for maintenance reasons. I do not care what style we would use. I am used to the all upper case style and if nothing speaks against it, I will change all macros to upper case letters.

 

Any comments?

Edited by Malaclypse

Share this post


Link to post
Share on other sites

I think macroes should be all upper case letters as well.

Share this post


Link to post
Share on other sites

I forgot the reason why SDL_opengl.h was used, rather than the standard GL headers. it MIGHT be due to the fact that windows has headers only for OpenGL 1.1 (by default), and while on Linux the OpenGL headers are generally up to date, they are not on Windows.

If it works on windows without it, then feel free to comment it out, and eventually remove it.

As for the macroes and defines, yes, they should all be upper cases only.

Share this post


Link to post
Share on other sites

Ok, so I will change all the macros to be all upper case :D

 

Anybody already tried to compile and run without the two #include statements? Any experience? I need response on windows based systems, as I don't have the possibility to compile and run on this.

Share this post


Link to post
Share on other sites

These are the changes I made to the macros:

 

 

2d_object.h:

__obj_2d_H__ -> __OBJ_2D_H__

max_obj_2d -> MAX_OBJ_2D

max_obj_2d_def -> MAX_OBJ_2D_DEF

invalid -> INVALID

ground -> GROUND

plant -> PLANT

fence -> FENCE

sector_size_x -> SECTOR_SIZE_X

sector_size_y -> SECTOR_SIZE_Y

 

3d_object.h:

__obj_3d_H__ -> __OBJ_3D_H__

 

actors.h:

__actors_H__ -> __ACTORS_H__

max_current_displayed_text_len -> MAX_CURRENT_DISPLAYED_TEXT_LEN

lock_actors_lists() -> LOCK_ACTORS_LISTS()

unlock_actors_lists() -> UNLOCK_ACTORS_LISTS()

 

actors.c:

ms_per_char -> MS_PER_CHAR

mini_bubble_ms -> MINI_BUBBLE_MS

 

asc.h:

my_xmlStrcpy() -> MY_XMLSTRCPY()

 

cache.h:

__file_cache_H__ -> __FILE_CACHE_H__

 

e3d.h:

max_obj_3d -> MAX_OBJ_3D

max_e3d_cache -> MAX_E3D_CACHE

 

errors.h:

LogError() -> LOG_ERROR()

 

filter.h:

max_filters -> MAX_FILTERS

 

font.h:

draw_ingame_normal() -> DRAW_INGAME_NORMAL()

draw_ingame_small() -> DRAW_INGAME_SMALL()

draw_ingame_alt() -> DRAW_INGAME_ALT()

 

gl_init.h:

check_gl_errors() -> CHECK_GL_ERRORS()

 

ignore.h:

max_ignores -> MAX_IGNORES

 

interface.h:

action_walk -> ACTION_WALK

action_look -> ACTION_LOOK

action_use -> ACTION_USE

action_use_witem -> ACTION_USE_WITEM

action_trade -> ACTION_TRADE

action_attack -> ACTION_ATTACK

 

interface_game -> INTERFACE_GAME

interface_log_in -> INTERFACE_LOG_IN

interface_new_char -> INTERFACE_NEW_CHAR

interface_console -> INTERFACE_CONSOLE

interface_opening -> INTERFACE_OPENING

interface_map -> INTERFACE_MAP

interface_cont -> INTERFACE_CONT

interface_rules -> INTERFACE_RULES

 

lights.h:

global_lights_no -> GLOBAL_LIGHTS_NO

max_lights -> MAX_LIGHTS

 

md2.h:

__file_md2_H__ -> __FILE_MD2_H__

 

new_character.c:

race_human -> RACE_HUMAN

race_elf -> RACE_ELF

race_dwarf -> RACE_DWARF

race_gnome -> RACE_GNOME

race_orchan -> RACE_ORCHAN

race_draegoni -> RACE_DRAEGONI

 

particles.h:

__particles_H__ -> __PARTICLES_H__

max_particle_systems -> MAX_PARTICLE_SYSTEMS

max_particles -> MAX_PARTICLES

particle_random() -> PARTICLE_RANDOM()

particle_random2() -> PARTICLE_RANDOM2()

lock_particles_list() -> LOCK_PARTICLES_LIST()

unlock_particles_list() -> UNLOCK_PARTICLES_LIST()

 

particles.c:

max_particle_defs -> MAX_PARTICLE_DEFS

 

reflection.h:

is_water_tile() -> IS_WATER_TILE()

is_reflecting() -> IS_REFLECTING()

 

reflection.c:

scale_factor -> SCALE_FACTOR

 

sector.h:

sector_get() -> SECTOR_GET()

 

sound.h:

max_buffers -> MAX_BUFFERS

max_sources -> MAX_SOURCES

lock_sound_list() -> LOCK_SOUND_LIST()

unlock_sound_list() -> UNLOCK_SOUND_LIST()

 

text.h:

max_display_text_buffer_lenght -> MAX_DISPLAY_TEXT_BUFFER_LENGTH

log_to_console() -> LOG_TO_CONSOLE()

 

text.c:

allowedCharInName() -> ALLOWED_CHAR_IN_NAME()

 

weather.h:

rain_drop_len -> RAIN_DROP_LEN

 

 

Does anybody see any objections to this? If not, I will commit these changes during tomorrow.

 

<edit>Removed the changes from client_serv.h</edit>

Edited by Malaclypse

Share this post


Link to post
Share on other sites

I also compiled without problems after removing the 2 SDL_opengl.h includes

using Dev-C++ on windows XP and game plays no probs.

Share this post


Link to post
Share on other sites
These changes should increase in game preformance right?

No. Hopefully they'll increase programmer performance, though.

Share this post


Link to post
Share on other sites
Umm...

Let the client_server.h alone (don't change the stuff in there).

I can understand you don't weant to mess up the communication with the server, but he's only changing the names of the constants, not their definitions. Or do you copy-paste between server and client code too?

Share this post


Link to post
Share on other sites
Umm...

Let the client_server.h alone (don't change the stuff in there).

I can understand you don't weant to mess up the communication with the server, but he's only changing the names of the constants, not their definitions. Or do you copy-paste between server and client code too?

Yes, we tend to use the same file between the client and the server, and on the server there might be some defines with the same name but CAPS, so this might cause problems and such.

Share this post


Link to post
Share on other sites

Ok, so I will undo the changes to client_serv.h and commit the other changes sometime tomorrow.

 

Thanks to all for your feedback so far on this, especially for the windows compilation thingy.

Share this post


Link to post
Share on other sites

I commited the changes related to the macros (excluding client_server.h)

 

This also commited the commenting of the SDL_opengl.h in frustum.c and particles.c by mistake. But as they seem to work, I don't change it back and recommit for now. I can do this if any error regarding this file occurs. But as it looks, it seems to work. I'm sorry for this.

Edited by Malaclypse

Share this post


Link to post
Share on other sites

LOL, I was sure this was going to happen at some point. I nearly did it myself twice :D However, this needed to be done anyway, so don't worry.

Share this post


Link to post
Share on other sites

Hehe Grum, I didn't worry about this, as I was thinking like you, in that it need to be done sooner or later. Just want to inform you other guys that it has happened (although unintended :()

 

----------

 

First part, covering macros is done:

 

 

Makros

------

Some macros are not used globally but only in the C file that corresponds to the headers where they are declared. I moved such macros to the corresponding .c files.

 

2d_objects.h:

INVALID, GROUND, PLANT, FENCE -> 2d_objects.c

 

buddy.h:

MAX_BUDDY -> buddy.c

 

chat.h:

CHAT_WIN_TEXT_WIDTH, CHAT_WIN_TEXT_HEIGHT, CHAT_WIN_SCROLL_WIDTH -> chat.c

 

e3d.h:

MAX_E3D_CACHE -> unused, commented

 

elwindows.h:

SCREEN -> unused, commented

 

hud.h:

WALK, SIT, LOOK, TRADE, ATTACK, USE -> unused, commented

 

init.h:

CFG_VERSION, DATA_DIR -> init.c

 

particles.h:

TELEPORTER_PARTICLE_SYS, TELEPORT_PARTICLE_SYS, BAG_PARTICLE_SYS, BURST_PARTICLE_SYS, FIRE_PARTICLE_SYS, FOUNTAIN_PARTICLE_SYS -> particles.c

PARTICLE_RANDOM(), PARTICLE_RANDOM2() -> particles.c

LOCK_PARTICLES_LIST(), UNLOCK_PARTICLES_LIST() -> particles.c

 

sound.h:

MAX_BUFFERS, MAX_SOURCES, BUFFER_SIZE, SLEEP_TIME -> sound.c

LOCK_SOUND_LIST(), UNLOCK_SOUND_LIST() -> sound.c

 

translate.h:

GROUP, DIGROUP, STAT_GROUP -> translate.c

 

weather.h:

RAIN_DROP_SPEED, RAIN_DROP_LEN -> weather.c

 

widgets.h:

LABEL, IMAGE, CHECKBOX, BUTTON, PROGRESSBAR, VSCROLLBAR, TABCOLLECTION -> widgets.c

 

 

Any of these macros that should not be moved to the implementation file?

 

Next, I will check all the variables and after this, the functions. I will also commit the changes individually, that is for macros, vars and funcs, there will be separate commit.

 

I'm going to commit the above changes tomorrow if there are no points against it.

Share this post


Link to post
Share on other sites

Another thing, the map editor also shares some client . and .h files, so please check if it works ok, and if it doesn't then adjust the map editor as well.

Share this post


Link to post
Share on other sites

Ah yes, you're right Ent, I forgot about this. How about the gtk-elconfig module? This also shares some common header files with the client. I will check it too.

Share this post


Link to post
Share on other sites

I added the changes related to the renaming of macros to all upper case to the map editor as well. It compiles and runs on linux, also i cannot tell if anything's working properly, as i'm not used with the map editor, also this shouldn't break anything.

 

Please someone try to compile and run on windows as well.

Share this post


Link to post
Share on other sites

The (UN)LOCK_PARTICLES_LIST() macros from particles.[hc] need to be kept in the header file for the map editor to compile. That's the only change necessary to the above.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

  • Recently Browsing   0 members

    No registered users viewing this page.

×