#68 fix GetColor function; add relation colors config colors

[Turbo Beholder]

1. GFXColor is used more.
2. "void getColor" nonsense which had to be danced around and conditionally initializing variables that instead of declaring them as static, and got several trivial wrapper functions... is fixed. Now it's all "vs_config->getColor" working exactly like "vs_config->GetVariable" does.
3. GFXMaterial now has a few inline functions using GFXColor - not big deal, but trivial incapsulation makes things more readable.
4. Using colToString (for "#RRGGBB" color codes) and colLerp more.
5. Blending relationship colors (as in messages) are set to config colors. See a request on forum.
6. A few functions are moved where they make more sense (DoSpeech in cmd/ai/aggressive.cpp - really? moved to communications.h/cpp); colLerp is moved to gfxlib_struct.h/cpp as a basic GFXColor function.

[Turbo Beholder]

Update: missed a few static vars. Here it goes.

[Klauss++]

It looks generally ok, but it's such a big patch to review like this...

...can you please split it a bit? Say, one for the getColor thing, another for moving the functions off of aggressive, and another for the blending. The main reason for splitting is to be able to commit separately (and bisect more accurately should bugs appear).

Also, you have commented out code in the patch. Don't do that, just delete the code. One, the diff will show better what code is actually removed, and two, we don't want more commented code in the repo, it's a mess, SVN already stores the old code and makes reverts easy anyway.

[Turbo Beholder]

Part 1 - vs_config->getColor() vs. contrived initializations.

[Klauss++]

When you remove the if (!init) stuff, you're removing an optimization.

Groups of static variables get initialized like that, because a static X = y statement produces an implicit check for an implicit bool to know whether it's been initialized already or not. Doing it explicitly avoids a lot of overhead.

[Klauss++]

Committed with that fixed

[Turbo Beholder]

Hmm. I see. Perhaps not a lot, but still. And to think of it, we'd eventually have to change this back to explicit initialization anyway, for runtime setup of HUD colors.

[Klauss++]

Well, enough overhead to show up on profiles I remember, a way back on
old hardware. Perhaps not so significant nowadays, but still worth
keeping in mind.

On Sat, Mar 1, 2014 at 7:54 PM, Turbo Beholder tbeholder@users.sf.net wrote:

Hmm. I see. Perhaps not a lot, but still. And to think of it, we'd
eventually have to change this back to explicit initialization anyway, for
runtime setup of HUD colors.

---

[patches:#68] fix GetColor function; add relation colors config colors

Status: open
Group: SVN
Labels: functions config colors
Created: Wed Feb 12, 2014 11:20 AM UTC by Turbo Beholder
Last Updated: Mon Feb 17, 2014 10:23 PM UTC
Owner: nobody

GFXColor is used more.
"void getColor" nonsense which had to be danced around and conditionally
initializing variables that instead of declaring them as static, and got
several trivial wrapper functions... is fixed. Now it's all
"vs_config->getColor" working exactly like "vs_config->GetVariable" does.
GFXMaterial now has a few inline functions using GFXColor - not big deal,
but trivial incapsulation makes things more readable.
Using colToString (for "#RRGGBB" color codes) and colLerp more.
Blending relationship colors (as in messages) are set to config colors. See
a request on forum.
A few functions are moved where they make more sense (DoSpeech in
cmd/ai/aggressive.cpp - really? moved to communications.h/cpp); colLerp is
moved to gfxlib_struct.h/cpp as a basic GFXColor function.

---

Sent from sourceforge.net because you indicated interest in
https://sourceforge.net/p/vegastrike/patches/68/

To unsubscribe from further messages, please visit
https://sourceforge.net/auth/subscriptions/

[Klauss++]

Oh, and also please change the colorstring to an actual std::string. It's not performance-critical, and we don't want the source of bugs a hand-managed string structure brings. For instance, if you try to use it in vs_dprintf, it will segfault (no null termination maybe?). In any case, a std::string is safer.

[Turbo Beholder]

I didn't change colorstring other than changing syntax to typedef, and the only function that generates colorstring have null termination:

+    ret.str[0] = '#';
+    ret.str[7] = '\0';

As to vs_dprintf... It's used only in faction_util_generic.cpp or for stdout/stderr. It doesn't use colors normally. So it's hard to tell what failed in your test - maybe "#rrggbb0" code itself rather than "char str[8]" part. When it's needed in the first place?

Now, prettyPrintFloat in basecomputer.cpp - maybe, but it uses different color codes (prefix-suffix, and with float RGB values, thus variable-length), and they seem to be always included into the text, all stitching is performed in PRETTY_ADDx macros. (Maybe it would be better to have another function for doing this neatly?)

As to colorstring performance, it's used for HUD text, so it runs many times each frame. Obviously, far from the worst bottleneck even if we'll take 2d cockpit code alone, but unlike base interface it's still realtime - so why not keep it as light as we can? Conversely,

[Klauss++]

Alright, I thought it was new code.

[Turbo Beholder]

The next part - GFXColor functions.

Diff to the r13669 /trunk. Changes are transparent and it compiles.

[Klauss++]

The only thing I don't like there, is that you made colLerp not inline. So I made it inline, and committed.

[Turbo Beholder]

part 3: Communications/RGB strings.

I took your advice about end functions being std:string and just added an inline wrapper - no mess in code, less calls in runtime, better validations on compilation.

[Klauss++]

Applied

[Turbo Beholder]

So, there's a little bug with atmospheres getting colored. I found only one actual change in these patches:

star_system_xml.cpp

-    ourmat.sr=planet_mat_specular.r;
-    ourmat.sg=planet_mat_specular.g;
-    ourmat.sb=planet_mat_specular.b;
-    ourmat.sa=planet_mat_specular.a;
-    ourmat.__sr__=planet_mat_emissive.r;
-    ourmat.__sg__=planet_mat_emissive.g;
-    ourmat.__sb__=planet_mat_emissive.b;
-    ourmat.__sa__=planet_mat_emissive.a;
...
+    setMaterialSpecular(ourmat, planet_mat_specular);
+    setMaterialEmissive(ourmat, planet_mat_emissive);

However, that's not where the problem is. The way those fields are initialized in universe.cpp
I added before "unsigned int tmp" lines like

    VSFileSystem::vs_fprintf( stderr, "Ambient  (%f %f %f %f) \n" , mat.ar, mat.ag, mat.ab, mat.aa );

turns out that "mat.foo = 1.0F;" sets the value to 1.000000 , while those functions don't - even "setMaterialDiffuse( mat, 1.0F, 1.0F, 1.0F, 1.0F );" for some reason ends up with zero or random uninitialized values.

[Klauss++]

Honestly, I didn't pay much attention to that part, because atmospheres such as that code involve, are being deprecated in favor of shader-generated atmospheres.

Still, we better fix that bug. I'll take a look at it.

[Klauss++]

Lol, the bug is due to setMaterialX receiving the material "mat" by value. Fixed in trunk, r13672

BTW, at work now, can't do extensive stuff, but those setBlah (and getBlah) should be methods... shouldn't they?

[Turbo Beholder]

Gah. Really. Lol.

Well, of course, it should have been a method if GFXMaterial was a class proper. Then again, it being handled as a struct and not a full-on class tend to cause things like this, so... want me to convert it?

Hmm, there are little optimizations using GFXMaterial as a big chunk of data - memcmp using "sizeof (GFXMaterial)" - don't classes have a little pointer overhead breaking it? Or it's cool unless it's actually overloaded? Then there could be other optimizations - e.g. to not put it into netbuffer 4 bytes at a time? I don't know any way to handle this other than either dropping to field-by-field or wrapping the class around an union struct to use them as "mat.chunk" vs. "mat.d.ar" as needed. What to do?

[Klauss++]

structs aren't really different from classes. The only difference is all struct members are public. But they can have everything a class can have other than that.

So, you don't need to convert it, but if you want to change the struct keyword with class be my guest ;)

Classes have no vtable and are otherwise identical to structs in-memory if they have no virtual methods, so the optimizations are still valid as long as you don't introduce any.

[Turbo Beholder]

Also, what's with this?

GFXMaterial ourmat;
GFXGetMaterial( 0, ourmat );
vs_config->getColor( "planet_mat_ambient", &ourmat.ar );
vs_config->getColor( "planet_mat_diffuse", &ourmat.dr );
vs_config->getColor( "planet_mat_specular", &ourmat.sr );
vs_config->getColor( "planet_mat_emmissive", &ourmat.er );

Not just that .power isn't initialized, but why it does GFXGetMaterial() only to immediately override the result? Just to get .power? This can't be right...

[Klauss++]

Not sure. Maybe in case the variables aren't there in the config?

[Turbo Beholder]

Anyway, got rid of it. Try this one? Runs for me. As a bonus, planets don't shine like glass balls all over, possibly because they don't import shininess 60.0 from the default material anymore. ;)

[Klauss++]

But that shininess value is important for oceans (the shiny ball effect is just due to horrible art)

[Turbo Beholder]

But... why it gets to be visible? Shouldn't planets have a separate specular texture for this and default to black if it's absent?

I reset it to 60.0 now, anyway... Hmm, but specular default is set to (0 0 0 1)...

Added a few "constants".

[Klauss++]

They should, but they don't (many).

It's fixable (by producing them), but nobody has come up with good
rocky heightmaps yet.