Malaclypse Report post Posted March 5, 2005 (edited) 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 March 5, 2005 by Malaclypse Share this post Link to post Share on other sites
Grum Report post Posted March 5, 2005 Well, IMHO anything you do to remove the number of global variables will be most welcomed 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
Malaclypse Report post Posted March 6, 2005 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
Cicero Report post Posted March 7, 2005 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
Malaclypse Report post Posted March 7, 2005 (edited) 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 March 7, 2005 by Malaclypse Share this post Link to post Share on other sites
Wytter Report post Posted March 7, 2005 I think macroes should be all upper case letters as well. Share this post Link to post Share on other sites
Grum Report post Posted March 7, 2005 I think macroes should be all upper case letters as well. Me three. Share this post Link to post Share on other sites
Entropy Report post Posted March 7, 2005 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
Malaclypse Report post Posted March 8, 2005 Ok, so I will change all the macros to be all upper case 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
Sadez Report post Posted March 8, 2005 I compiled without problems after removing the 2 SDL_opengl.h includes (on VC6). Share this post Link to post Share on other sites
Malaclypse Report post Posted March 8, 2005 (edited) 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 March 9, 2005 by Malaclypse Share this post Link to post Share on other sites
Entropy Report post Posted March 8, 2005 Umm... Let the client_server.h alone (don't change the stuff in there). Share this post Link to post Share on other sites
Specter Report post Posted March 8, 2005 (edited) These changes should increase in game preformance right? Edited March 8, 2005 by Specter Share this post Link to post Share on other sites
MoonlitKnight Report post Posted March 8, 2005 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
Grum Report post Posted March 8, 2005 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
Grum Report post Posted March 8, 2005 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
Entropy Report post Posted March 9, 2005 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
Malaclypse Report post Posted March 9, 2005 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
Malaclypse Report post Posted March 9, 2005 (edited) 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 March 9, 2005 by Malaclypse Share this post Link to post Share on other sites
Grum Report post Posted March 9, 2005 LOL, I was sure this was going to happen at some point. I nearly did it myself twice However, this needed to be done anyway, so don't worry. Share this post Link to post Share on other sites
Malaclypse Report post Posted March 9, 2005 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
Entropy Report post Posted March 10, 2005 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
Malaclypse Report post Posted March 10, 2005 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
Malaclypse Report post Posted March 10, 2005 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
Malaclypse Report post Posted March 10, 2005 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