"Darkness" bug

[Bereror]

I know, this bug is reported at least 1000 times  

Because this bug happens to me almost every time I log into the game, I spent some time trying to find out why it happens. So here are my findings. Maybe they can be used to fix the \"darkness\" bug once and forever.

Normal players who do not have the privilege to modify their source files and recompile Planesift may look at the end of this long post for a workaround.


Lights in the game are updated when crossing sectors or when the time changes. After starting up the client, sectors are changed from NULL to SectorWhereWeKeepEntitiesResidingInUnloadedMaps and then to the final sector (hydlaa_plaza in my test case).

Once the sector is changed, the ModeHandler::ClearLightFadeSettings() function in the modehandler.cpp file is called. There is the following line in that function:

Code: [Select]


last_interpolation_reset = csGetTicks() - interpolation_time - 1000;


csGetTicks() returns the number of milliseconds since the CS engine was initialized and interpolation_time is 40000.

Now, the number of milliseconds since the CS engine was initialized depends on a) speed of the computer, b) ping time to the server and c) how fast the user clicks on buttons to get into the game. In my case it is usually somewhere between 32000 and 38000.

So what would be the result if csGetTicks() returns 38000? It would be -3000 (actually a very large positive number because the variable is of type unsigned int, but it is easier to use negative numbers here).

Then the ModeHandler::UpdateLights(csTicks when) function is called in the same file with when set to the same value than last_interpolation_reset or -3000 in our case.

This function is supposed to update lights in a smooth way that the lights do not get switched on/off. It needs at least two runs to get lights updated where the first run initializes some light and color values.

If this function is now called with the value -3000, we run into the \"darkness\" bug, because there is this line:

Code: [Select]


if (when > last_interpolation_reset + interpolation_time)


-3000 + 40000 would be 37000. Since all these variables are actually unsigned integers, -3000 is a very large positive value (4294964296) and the equation is TRUE.

The function gets called only once, lights are updated with uninitialized light and color values and we have the \"darkness\" bug and/or strange colors on the screen.

In my client I changed the ModeHandler::ClearLightFadeSettings() function in the following way:

Code: [Select]


  csTicks current_ticks;
  current_ticks = csGetTicks();
  if (current_ticks >= (interpolation_time - 1000))
    last_interpolation_reset = current_ticks - interpolation_time - 1000;
  else
    last_interpolation_reset = 0;


Seems to be working and I haven\'t seen any more problems.


All others who have to wait for official updates, there is a workaround for you:
After you start up Planeshift, wait at least 41 seconds before you enter the game. This makes sure that your lights and colors are properly updated.

[LigH]

Speaking of very large integers: Wouldn\'t it be iseful to check the sources in general where \"unsigned integer\" calculations shall be changed to \"signed integer\" instead? The C language family is horrible here, you may have to add typecasts for the calculation results where it is \"risky\".

Anyway - if darknes strikes, we know now that we shall tell those people: \"Next time, log in slower.\"

This may be the reason that I almost never experienced it: My PC is just tooooo sloooow loading the world.

[Nilrem]

Very interesting argument, and I think it\'s pretty correct.
I attached the URL to the bug of the BugTracker.

EDIT: Tested 5 times leaving a margin of 1 minutes before trying to join fragnetics and all of them I experienced no darkness bug. Indeed it can be the solution, thanks a lot Bereror for diving inside the code and catching the weak part. Thumbs up. :]

[stfrn]

Since it seems to be valid, I commited this to CVS, we will see if things are improved in the next update. Either way, good work Bereror

[DaveG]

YAY!!!  42 cheers for Bereror!  

We have a new bug fixer.  

[Radix]

Reference:  Bereror post at 6:05 today

  I may have missed something in the offered fix, or maybe this can be useful.

 To the point:
 
 In the C language associativity evaluates the \"+\" and \"-\" operators  sequences from left to right
       
Such that:

######
  if (current_ticks >= (interpolation_time - 1000))

    last_interpolation_reset = current_ticks - interpolation_time - 1000;
######

  Evaluates as:

######
  if (current_ticks >= (interpolation_time - 1000))

    last_interpolation_reset = ((current_ticks - interpolation_time) - 1000));
######

However it seems that the logic is meant to be as follows:

######
  if (current_ticks >= (interpolation_time - 1000))

    last_interpolation_reset = (current_ticks - (interpolation_time - 1000));
######

 case example:

#################
current_ticks = 38000;
interpolation_time = 40000;

 if (current_ticks >= (interpolation_time - 1000))
/* if (38000 >= (40000 - 1000)
    if (38000 >= (39000)            (Note :  difference is -1000)
*/
last_interpolation_reset = ((current_ticks - interpolation_time) - 1000));
 
/* last_interpolation_reset = ((38000 - 40000) - 1000))  
    last_interpolation_reset = ((- 2000) - 1000))
    last_interpolation_reset = ((- 3000))
*/

/*
   last_interpolation_reset = (38000 - (40000 - 1000))
   last_interpolation_reset = (38000 - (39000))
   last_interpolation_reset = (- 1000))
*/
##################

 The only point is that if the csGetTicks() function returns 40001 through 41001, the \"if-then-else\" will fall-through incorrectly, and set a negative value for last_interpolation_reset.

 The example results are based on the (constant) -1000 shown in the fix.

 If I am off in the weeds... please delete this post as trash! :|

[Bereror]

Fixing old bugs always means introducing new ones  

Of course the if statement should be:

Code: [Select]


if (current_ticks >= (interpolation_time [B]+[/B] 1000))


The intention was to make sure that last_interpolation_reset is never less than zero.

Thanks Radix for reading the post and trying to understand the logic  

Isn\'t OSS great? There are always more eyes reading the code and finding your mistakes.

[stfrn]

Fixing old bugs always means introducing new ones

Not if I can help it. Anyways, where were you people when I fixed 30+ bugs in the last update But good to get the as many as possible fixed, or atleast brought down in annoyance. Likely there will still be some lighting problems, but with this there be once less cause.

Feel free to pass on any other fixes, or ideas for fixes. And no, \"idea: fix npcclient!\" has already been suggested :rolleyes:

[Waylayen]

Ic: The streets are filled with blood! Apocalypse Nears!
All follow the dark way, and live trough salvation!

Oc: New bug? or something Well known?

[Bereror]

Originally posted by Waylayen
Oc: New bug? or something Well known?

By my best knowledge this is a variation of the same \"darkness\" bug. When lights are updated, every component is updated separately using a formula that makes the change smooth for your eyes. If the formula is not properly initialized, you may get strange results where one component is changed too much and others are not changed at all.

[DaveG]

As far as I know, the colors bug only happens on certain systems.  It\'s believed to be related to the graphics hardware.  However, if that common assumption is wrong, and you can manage to fix yet another bug (that I\'ve never personally had ) more power to ya.  

[Radix]

====================== (could not get \"quote\" to work)
Of course the if statement should be:
-------
code:

    if (current_ticks >= (interpolation_time + 1000))
-------
The intention was to make sure that last_interpolation_reset is never less than zero.
============================

  That \"+\" certainly nulled my exercise.

  I did understand the intent to not set a value less than zero, and I could have explained my point more simply.  Did not consider my audience very well.  

Thanks for acknowledging the attempt.