Defines (again)

[Torg]

Given the huge number of defines in the client, and the fact some "standard" ones were forgotten in this release, I am suggesting that we remove some of the "standard" ones.

 

I am suggesting anything that has been included in the officially released client should have its define removed after the release, unless there is a good reason for it (temporary fix etc).

 

Thoughts?

[0ctane]

lol. I was just going through make.defaults and realized that I had missed a bunch of "standard" defines for the OSX client.

 

I agree with Torg. Cleaning up the defines would be nice.

[Entropy]

Daniel will handle that, but make a list of defines that you think should be removed.

[ttlanhil]

ANTI_ALIAS adds an option in ctrl+o, so I think it can be merged in

ATI_9200_FIX does just one extra OpenGL call... if it's important enough that other cards don't use this, then I'd prefer a card detection on startup with workarounds implemented there (such as disabling a screen capture on s3, which I used to provide as a workaround client. similar code could then be added for any other bugs we had to work around in specific cards)

AUTO_UPDATE doesn't work on all installs thanks to permissions issues. however one of the things I'll be working on will fix that; unless there's a good reason I'll remove the define after that

BUG_FIX_3D_OBJECTS_MIN_MAX has a description saying it's unneeded for the new file format, which I assume means it's no longer appropriate

COUNTERS is in common use, and could be merged in

now that EYE_CANDY is in use in a release, it may be worth putting that into the main

FONTS_FIX could probably become the default

we have both EL_BIG_ENDIAN and LITTLE_ENDIAN, one of them is certainly not needed

NO_PF_MACRO - is there any need for this? it's just a switch between using a macro or an inline

NOTEPAD - once fixed/finished, I think this could be part of the normal compile

SFX - does this do much that EYE_CANDY doesn't? perhaps it could be rolled into EYE_CANDY

 

ed: heh, I was right about the bug-fix one, it's not even in the code anymore, just make.defaults not for long though (going to remove it now)

Edited June 7, 2007 by ttlanhil

[Puntif]

I'd prefer that EYE_CANDY not be removed quite yet . . . I had to disable eye candy in order to get a compile on my system. My gcc install (slackware linux 2 different versions) couldn't resolve is_finite(). I disabled eye_candy and was able to get a successful compile&link from CVS.

[0ctane]

SFX - does this do much that EYE_CANDY doesn't? perhaps it could be rolled into EYE_CANDY

Yeah, SFX does not really need to be around anymore. Well, I guess it is up to Ent and Roja, but I thought the original SFX code would be kept around for people with graphics cards that don't support the eye_candy effects. Unfortunately, I am pretty sure the EYE_CANDY puts the kabosh on the old, simpler effects.

[Learner]

ANTI_ALIAS adds an option in ctrl+o, so I think it can be merged in

 

ANTI_ALIAS hasn't been in an official client yet, so it's premature to talk about removing the #ifdef

[Torg]

AFK_FIX hasn't been mentioned yet. I know it has been in previous clients, but appears to have been forgotten in 1.4.0.

 

ALPHA_ACTORS and IDLE_FIX have been around for a while I believe, but they may be there if someone has a specific problem I guess.

 

 

/edit: Typos. :-S

Edited June 8, 2007 by Torg

[Learner]

AFK_FIX hasn't been mentioned yet. I know it has been in previous clients, but appears to have been forgotten in 1.4.0.

 

ALPHA_ACTORS and IDLE_FIX have been around for a while I believe, but they may be there if someone has a specific problem I guess.

 

 

/edit: Typos. :-S

ALPHA_ACTORS is too new to consider removing. This is the first full client release it has been in and Roja isn't taking advantage of the feature currently.

[Torg]

ALPHA_ACTORS is too new to consider removing. This is the first full client release it has been in and Roja isn't taking advantage of the feature currently.

Sorry, my mistake. I thought I'd seen it before.

[ttlanhil]

would it be fair to assume that anything that doesn't compile is due to be removed, since it's unmaintained? with the exception of where it requires another feature to be set and is documented to do so, at least

[Learner]

would it be fair to assume that anything that doesn't compile is due to be removed, since it's unmaintained? with the exception of where it requires another feature to be set and is documented to do so, at least

I don't like agreeing to a blanket statement like that, because what if a useful debug feature doesn't compile because of recent changes? OR what if a new set of going is going to go in to replace an unused feature and the old is usable as a reference? Or what if we don't want to remove broken platform support (such as freebsd) in the hopes that someone will pick it back up again?

[ttlanhil]

well, true, but I don't mean things like debugging or platform support. those I expect not to be as well maintained.

 

I mean things like NEW_TEX, if I comment that out, it no longer compiles properly (maybe it's related to my other settings... but without documentation, that'd be annoying)

 

swap due for likely, I guess. I'm just getting a little tired of having to fix the code because other people didn't test with the right defines (yes, I know, there's a lot, and that's hard. but that's part of the problem. on the other hand, sometimes people write code inside an #ifdef and don't test with that #ifdef. that's not so good)

[Learner]

well, true, but I don't mean things like debugging or platform support. those I expect not to be as well maintained.

 

I mean things like NEW_TEX, if I comment that out, it no longer compiles properly (maybe it's related to my other settings... but without documentation, that'd be annoying)

 

swap due for likely, I guess. I'm just getting a little tired of having to fix the code because other people didn't test with the right defines (yes, I know, there's a lot, and that's hard. but that's part of the problem. on the other hand, sometimes people write code inside an #ifdef and don't test with that #ifdef. that's not so good)

I thought you meant removing the code to stuff that doesn't compile or get used, like TERRAIN

 

Keep in mind when removing the #ifdef lines that the map_editor and other platforms need to continue to compile. Some options are required in compiling the main game so that the map editor can compile. Others we have left in as emergency compile options (like NO_MUSIC) so those were switched to #ifndef.

 

Another one we can't change is ALUT_WAV, because depending on the version of OpenAL/ALUT you are compiling against, that is either needed or not.

[ttlanhil]

I thought you meant removing the code to stuff that doesn't compile or get used, like TERRAIN

There is no TERRAIN anymore, but IIRC, it was marked as do-not-use in make.* so I'm fine with that (anyone who does use it either knows why or deserves to have bugs).

As long as anything that's not to be used is marked as such, so people don't select it, I don't really mind (ideally, there should be a reason why it's kept, but if properly marked it's not so big a deal)

Keep in mind when removing the #ifdef lines that the map_editor and other platforms need to continue to compile. Some options are required in compiling the main game so that the map editor can compile.

That's fine. But if the defines are only for the map editor, then shouldn't we either

1) disable them in the code if MAP_EDITOR is not set, or

2) not include them in make.defaults ?

If it's only in the code and not in make.defaults, then it's not as big a deal (although there are a lot of them already, and some people may wonder why...)

Others we have left in as emergency compile options (like NO_MUSIC) so those were switched to #ifndef.

Sure, that's fine by me. I don't use the music, so I'll probably leave things like that in there.

However, that's not so much what I'm looking at.

Theroetically, only people working on the sound/music effects can cause damage to NO_MUSIC... and even then, it should take a bit of effort to do so.

And it's an isolated define.

On the other hand, if you look at the actor animation and textures and such, there are several defines... This is where it gets trickier, because you may change something that appears unrelated and test with the most common options, only to be told that there's one combination that doesn't work.

Or worse yet, you aren't told about it.

Another one we can't change is ALUT_WAV, because depending on the version of OpenAL/ALUT you are compiling against, that is either needed or not.

It's a legacy feature, I know, that's fine... But... I was under the impression that it was added because later versions of OpenAL/ALUT no longer had the wav loader included... So ELC had a wav loader added in the code, and people could set ALUT_WAV to say "don't use the one in ELC, use the one in the library".

If so, setting that flag, for the people with the older library, may be a little better performance-wise, but if everyone uses the one in ELC, shouldn't it still Just Work?

Edited June 10, 2007 by ttlanhil

[0ctane]

It's a legacy feature, I know, that's fine... But... I was under the impression that it was added because later versions of OpenAL/ALUT no longer had the wav loader included... So ELC had a wav loader added in the code, and people could set ALUT_WAV to say "don't use the one in ELC, use the one in the library".

If so, setting that flag, for the people with the older library, may be a little better performance-wise, but if everyone uses the one in ELC, shouldn't it still Just Work?

Well, as far as ALUT_WAV is concerned, we need to keep it for Mac compatability. I have not had a chance to figure out how I can get around that define yet.

[Learner]

Another one we can't change is ALUT_WAV, because depending on the version of OpenAL/ALUT you are compiling against, that is either needed or not.

It's a legacy feature, I know, that's fine... But... I was under the impression that it was added because later versions of OpenAL/ALUT no longer had the wav loader included... So ELC had a wav loader added in the code, and people could set ALUT_WAV to say "don't use the one in ELC, use the one in the library".

If so, setting that flag, for the people with the older library, may be a little better performance-wise, but if everyone uses the one in ELC, shouldn't it still Just Work?

ELC does NOT have a wave loader in the code, instead it uses the generic sound loader in ALUT instead of the old waver loader. If a wave loader was ever added, it should be removed. A patch was submitted, but the other routine that is part of OpenAL/ALUT was used instead.

 

For things that we aren't sure if, the #ifdef's could be change to #ifndef and the change the define itself. That way it's on by default but can be disabled.

[ttlanhil]

ELC does NOT have a wave loader in the code, instead it uses the generic sound loader in ALUT instead of the old waver loader. If a wave loader was ever added, it should be removed. A patch was submitted, but the other routine that is part of OpenAL/ALUT was used instead.

ahh, yeah, that patch was what I was thinking of

 

Oh well, as long as someone maintains it and other coders don't break it, something like ALUT_WAV shouldn't give too many problems until just about everyone has a newer version of OpenAL/ALUT

[ttlanhil]

Okay, how about GL_EXTENSION_CHECK ?

I don't see a great need for it to be in an #ifdef, esp if it's in the official client, but if it's to remain an option, it should, IMO, be added to make.defaults .

[Learner]

Okay, how about GL_EXTENSION_CHECK ?

I don't see a great need for it to be in an #ifdef, esp if it's in the official client, but if it's to remain an option, it should, IMO, be added to make.defaults .

The only reason it was there was so it could be disabledif there last minute problems in the prerelease. If there have been no reports of problems with the checking, it could be removed.

[ttlanhil]

*Bump* Is there any more input on this, or are we about ready for the final list to be made?

[ttlanhil]

Okay, I think I have the full list. I've trimmed out map editor, platform, debug (Although it might not hurt to trim the number of debug commands), etc. defines.

 

This is how I currently see things.

 

Roll in:

AFK_FIX
AUTO_UPDATE
CLICKABLE_CONTINENT_MAP (might be better in the keep-experimental section, but it seems stable)
COUNTERS
FONTS_FIX
NEW_TEX (this one seems to be required to play EL at all)
NPC_SAY_OVERTEXT (move the #define to client_serv.h where it should have been all along)
OPTIONS_I18N
PARANOID_CAMERA
USE_INLINE

 

Roll out:

USE_VERTEX_ARRAYS(Only use in code is commented out, and '#if 0'-ed out, and there's a variable used instead anyway)

 

Combine:

EYE_CANDY & SFX
CUSTOM_LOOK & CUSTOM_UPDATE
EL_BIG_ENDIAN & LITTLE_ENDIAN (Opposite each other, of course, but I see no reason to have both of these)

 

Leave as defines for portability/library reason:

PNG_SCREENSHOT
ALUT_WAV
NO_MUSIC

 

Leave as defines because they are, currently, new/experimental/unfinished:

ALPHA_ACTORS
ANTI_ALIAS
DYNAMIC_ANIMATIONS
IDLE_FIX
MASKING
MINES
MINIMAP
NEW_ACTOR_ANIMATION
NEW_ALPHA
NEW_FILE_IO
NEW_LIGHTING
NEW_SOUND
NEW_WEATHER
NOTEPAD
USE_SHADER
USE_TANGENT
PAWN
UID
USE_EXTRA_TEXTURE
ZLIB

 

And a few remaining ones:

ATI_9200_FIX (Roll in? Would it be that bad for other cards to have this?)
FUZZY_PATHS (Roll in? It shouldn't do any harm.)
NO_PF_MACRO (Roll in? That is, use the (typed) inline instead of a macro.)
OLD_CLOSE_BAG (Roll out? Or is there still a need for this?)
OLD_TEXTURE_LOADER (Roll out? Or is there still a need for this?)
SIMPLE_LOD (Roll in? It shouldn't do any harm.)

 

Which of the above do people disagree with (or agree with, for the last section)? If the list is getting close to accurate, then maybe we can have it trimmed down soon (I'm happy to make the changes, if no-one else feels a need to do it themselves, once we have the list)

 

ed: I'll attempt to compile the map editor with the changes in #defines to make sure it doesn't break anything there, but there's no guarantee I can even compile the beastie now...

Edited July 1, 2007 by ttlanhil

[Learner]

CLICKABLE_CONTINENT_MAP has never been in a client, brand new, leave the define.

 

NPC_SAY_OVERTEXT should either have the code in multiplayer.c removede totally or changed to have to be specifically defined. client_serv.h is the master definition of packet types, anything NOT in there is not supported and an invalid packet.

 

PARANOID_CAMERA has never been in an official client, so the #ifdef should not be removed. What does it even do?

 

CUSTOM_LOOK has only been in the most recent client. Are you sure it's ready to forced in?

 

FUZZY_PATHS I don't think this got into the most recent client(was supposed to), same question applies.

 

NO_PF_MACRO when was this put in? Has it been tested enough? Is there anyone that can't use the inline? Why even an inline vs a macro? in theory the inbine should be preferred over the #define

 

OLD_TEXTURE_LOADER is brand new so it should be left alone for now.

 

SIMPLE_LOD was a quick FPS improvementfor perspective thats helps some people, but it either should be made tunable or a better LOD logic go in and tested before that define is removed?

[ttlanhil]

CLICKABLE_CONTINENT_MAP has never been in a client, brand new, leave the define.

On one hand, yes, new things should be tested first; but on the other hand, so much of what happens between releases doesn't have an #ifdef for it; and this code is almost small enough that it could have gone in without an #ifdef. It's not that large a change, and unless there's an issue with memory lost or an OpenGL call out of order, which seems quite unlikely, it can't break anything. Hence why I thought it was suitable for the list

NPC_SAY_OVERTEXT should either have the code in multiplayer.c removede totally or changed to have to be specifically defined. client_serv.h is the master definition of packet types, anything NOT in there is not supported and an invalid packet.

Okay, well, there is a 'hole' in client_serv.h for the number it's given. Other than that, someone who works on the server (which would be you) should check to see if it should go in client_serv.h; having it in a *.c looks like some sort of hack (and it has been there a long time)

PARANOID_CAMERA has never been in an official client, so the #ifdef should not be removed. What does it even do?

As I understand it, if the angle of the camera changes, then the scene is set to be redrawn. That's all. Turns out it's something that, IMO, should have been done from the start, as changing camera angle can cause blank areas on the map because the drawable regions data wasn't updated.

CUSTOM_LOOK has only been in the most recent client. Are you sure it's ready to forced in?

No, but I'm not saying it should be. I'm only saying CUSTOM_LOOK and CUSTOM_UPDATE should be rolled together to be only the one define. What I've read suggests that they are rather related; and a single define still allows us to disable it on demand.

The same thing goes for EYE_CANDY and SFX; while I think they're probably about ready to be part of the main, I'm not certain... However I do think that SFX can be removed and anything it provides to EYE_CANDY can be transfered.

FUZZY_PATHS I don't think this got into the most recent client(was supposed to), same question applies.

Okay, maybe not, but considering how little a change it makes, what harm can it do?

NO_PF_MACRO when was this put in? Has it been tested enough? Is there anyone that can't use the inline? Why even an inline vs a macro? in theory the inbine should be preferred over the #define

Long, long ago, AFAIK. By default, it seems we use use a macro, but there's an inline version as an alternative (possibly for compilers that didn't support tricky macros? MSVC 2k2? Dunno). I'm much in favour of using the inline version instead.

OLD_TEXTURE_LOADER is brand new so it should be left alone for now.

Actually, it's the old, Xaphier put in new texture loading as the default. As such, OLD_TEXTURE_LOADER is sort of a fallback in case there's trouble with the new code (yes, this is the opposite of how we normally add things, with a NEW_* disabled by default). The new texture loading seems stable, and has been made the default (rightly or not), so I'm unsure if there's a need to maintain the old code.

SIMPLE_LOD was a quick FPS improvementfor perspective thats helps some people, but it either should be made tunable or a better LOD logic go in and tested before that define is removed?

Well, there's a near plane setting already, and Emajekral's 1st person view has a far plane clipping option... In the case where it would be most beneficial, it's not required... While some LOD checking is required, and tuning it would be good, I don't see the harm in putting it in now and tuning later

[Learner]

In general, when we add a new fature or change how things work, we need to keep the #ifdef or #ifndef around for one or more client releases incase there are unusual problems with specific playrs or unexpected side effects.

 

So, anything that has just been added as a new feature still needs to keep a #ifdef or #ifndef so we can still revert to the old code, that applies to the CONT maps, new loader etc.

 

For the say over text ... the server doesn't even send that currently, so it should not be removed because it has barely been tested!

 

Yes, we need to remove old #ifdef's & #ifndef's, but things that haven't been included in the regular client or are having problems should still be compile configurable so we can still revert to the old code when problems appear.