pjbroad
commented
3 years ago There's a lot here to discuss but generally its a great idea. I'm going the make excuses that sound like I'm not supportive... :roll_eyes:
I'll do a more thorough check but some obvious choices to remove cannot be done right now for example NEW_SOUND. Sound is not yet supported for the Android client so it is compiled without the option. Its the same for CUSTOM_LOOK and CUSTOM_UPDATE. There are undoubtedly others. Another obvious but not-possible-currently choice is NEW_EYES, the Other Life client does not use NEW_EYES, the feature it is not supported and so the Other Life client compiles without it. Again there are are undoubtedly others.
Some caution: We are hopefully close to a 1.9.6 release which will include some nice map updates and possibly some new sounds and a corresponding sever update. I'm a little concerned to introduce so much churn in the code base unless that's way off and we can do lots of testing and checking. In the past when we have stripped out #DEFS there have been mistakes. For example one time we lost thunder sounds, another time all the ground textures orientations got scrambled. These were not noticed for a while and took time to fix because it was not immediately clear what had happened and its hard to find a bug in code that has been deleted.
If we are going to do this, may I suggest limiting single #DEF changes to a single commit or pull request. Perhaps make sure we review changes too, to avoid the above issues. Changes should be checked (at least compiled) for the Map Editor, and the Android and Other-Life clients too. If we do the candidate checking and get a definitive hit list, the task could be shared around.
gvissers
It's spring in the northern hemisphere, and I really should start painting the window frames in the front of my house. So obviously, instead of that, I have been looking a bit at the various compilation options for the client in an effort to find out if we can cut back a bit on the sheer number of them. A number of these options have been in released clients for years, and could IMO just be enabled unconditionally. For others I wonder if they will ever be used. I will admit that I'm only looking at this from a PC user point of view, though, so it is possible that I miss some information.
To start with, I think the following options could just be enabled unconditionally (and the code paths where these are disabled removed):
BANDWIDTH_SAVINGS. Would the client even work correctly anymore with current servers with this disabled? If there is value in keeping it, I propose the logic is inverted, i.e. make aNO_BANDWIDTH_SAVINGSoption.ANIMATION_SCALING: Not sure on the details of how this works, but it has been around for 10 years and I cannot find complaints about it.FASTER_MAP_LOAD: I may be biased on this one, but it works well and provides some serious speedups when loading a map. Though the effect may be less noticeable on modern systems.FASTER_STARTUP: idemNEW_EYES: Enabled by default for 6 years now, and seems to work.CLUSTER_INSIDES: Again, biased, but it works, and provides tangible benefits.CUSTOM_LOOK: Personally not sure, but I believe it works? The feature has been around for 15 years...CUSTOM_UPDATE: idemFUZZY_PATHS: Experimental in 2007, and the code changes are trivial. Yes, your path may (probably will) not strictly be the shortest possible anymore, but is anyone seriously bothered by that? I like the added randomness. Now, if we could get our characters to prefer the roads over walking through the fields, that would be a nice improvement...NEW_SOUND: "new" being very relative here. I think this one has proven its worth.PNG_SCREENSHOT: dead useful, been around for ages.USE_INLINE: very few users left, onlycache.hif I'm not mistaken. Compilers and standards have improved, let's just enable this unconditionally, and change all uses of__inline__toinlinewhile we're at it. In addition: can we update to the C standard to C99, or is there a good reason not to?NO_PF_MACRO: the macro is ugly, and I highly doubt it has any benefits over an inline function with any halfway decent compiler. Let's remove the macro and simply use the function.Options I think we could remove:
EL_BIG_ENDIAN: replace bySDL_BYTEORDER == SDL_BIG_ENDIANor judicious use of theSDL_Swapfunctions.PAWN: though it breaks my heart, I don't thinks this is ever going to be used. Besides, it is a really obscure language, and just getting Pawn code it to compile is a challenge in itself.UID: Needs server support, which apparently has not been forthcoming for the past decade.WRITE_XML: with this option, translatable strings fromtranslate.cwill be saved to XML files, possibly overwriting installed game data. This has no place in the game client. If we want to keep the functionality, it should be relegated to a separate tool.ENCYCLOPEDIA: No activity for 14 years. Was apparently meant to create a new encyclopedia parser (now that sounds familiar...), but it looks like it never took off.MAP_EDITOR2: an interesting project, that has never made it either.Options I think we should keep around for the time being:
FSAA: It's been around for a while, but I believe has only been fully enabled recently?MIDDLE_MOUSE_PASTE: This seems to be broken ATM, at least on my system? I happen to like the feature, but I believe not many people do. Then again, it is enabled by default...NEW_CURSOR: I believe this has never been wired up to actually work? I've been meaning to look into having coloured cursors, but still haven't done so. I'm also not yet convinced this option is the way to go.JSON_FILES: relatively new, though it seems to work well. I don't know if there is a downside to just enabling this unconditionally (old data files, perhaps?).TTF: also too new, although the option does not protect much, only the ability to use TrueType fonts.USE_SIMD: not every machine has SSE instructions. Though it would be nice if we could detect this in the build process somehow. That being said, is there any reason not to enable this by default? By far the most common machines these days will be x86_64.USE_ALGETSOURCEI_AL_BUFFER: we should really fix that....Category "I honestly don't know":
ANTI_ALIAS: is this used/useful? Has this been superseded by FSAA?DYNAMIC_ANIMATIONS: "appears broken" according to the makefile. Can we fix it? Do we want to?EXT_ACTOR_DICT: "Removes remaining hard-coded actor def dictionaries". Ok? Are there any left at this point? What about the updated actor def files mentioned for this entry?NEW_ALPHA: Been around for ages, looks like it's meant for transparent 3D objects? Has this ever been tested? Any reason not to enable it, apart from possible performance issues? Code wise, it's not a very intrusive feature.SIMPLE_LOD: I don't use it. Does anyone?SOUND_FORK_BUGFIX: is this still needed? If so, some comments in the code might be a good idea.USE_ACTORS_OPTIMIZER: I don't believe I have ever used this. Does it provide any tangible benefits?I would like to invite everyone who compiles their own client (and especially @pjbroad who has a much better overview of all EL related projects than I have) to weigh in with their opinions on what flags should be kept or removed. I am willing to do the work to clean up the code, but let's try to build some consensus first.