[airstrike] bug in timing code

Erik Auerswald auerswal at unix-ag.uni-kl.de
Thu Sep 1 03:34:08 EDT 2005


Hi,

I think there is a need for me to elaborate more on my statements...

First I'd like to mention that a framerate of 30fps is equivalent to a
frame duration of about 33ms (and vice versa). Using the value 30 for
the duration of one frame in ms as well as for the framerate in fps
introduces only a small error easy to overlook (btw. SDL does not guarantee
any timing better than 10 ms). Nonetheless using it this way leads to
hard to understand code which might result in hard to find bugs.

> > Using the name FRAMERATE for the time of one frame is a litte bit
> > confusing, I suggest using the name FRAMETIME.
> 
> Framerate = frames per second, so the define name is correct.

The scheduling functions use game frames as time unit. The length of a
game frame is defined by the function display_waitframe() in
core/display.c. This function uses display.frame_time for the duration
of one frame (time passed until the next frame is displayed), measured
in ms (see manpage of SDL_Delay). display.frame_time is set to FRAMERATE
in line 8 of this file.

I assumed this code to be implemented as it was intended. This
assumption is backed by the comments in sprites/biplane.c concerning
PLANE_{SAY,PUFF}_DELAY, {red_baron,blue_duke}.{bomb,bullet}_delay.
Therefore I suggested the use of FRAMETIME for the define. The other
suggested changes are based on this as well.

> FRAMETIME would indicate time that it takes to do one frame, i.e. 1/30,
> but I'd rather use integers.

That's what I had in mind (using ms instead of s as time unit).

> > there is a bug in engine/schedule.c, function rate_limit(): The
> > transformation from delay in units of 10ms to gameframes is not correct.
> > The same wrong formula is used in engine/sprite.c, function
> > sprite_alarm(). The correct formula is delay*10/time_for_one_frame.
> ------------
>        /* convert milliseconds to frames */
> -      *timer = now + delay * FRAMERATE / 10;
> +      *timer = now + (delay * 10) / FRAMETIME;
> ------------
> 
> The original code is also here correct.
> - timer = now + number of frames to achieve the delay
> - delay = x * 1/10 secs
> I.e. delay/10 gives you the seconds and multiplying that with FPS
> gives you the number of frames in that number of seconds.

Indeed the code is correct if a framerate in fps and a delay in (1/10)s
is assumed. The code suggested by me assumes a frame duration given in
ms and a delay given in (1/100)s.

> (Division is at the end in the code just because we're dealing with
> integers.)

Therefore I used parentheses to prevent compiler optimizations (IIRC
order of execution of operators with the same precendence inside one
statement is not defined unless parentheses are used).

> The term 'milliseconds' in all the comments should be changed to 1/10 secs
> however...

The comments should be updated and IMHO delays should be specified in
the same time unit in the whole code. What unit to use (game frames,
100ms, 10ms or something else) should be discussed and the result
documented.

All timing code should be based on one representation, either framerate or
frame duration, given in one define (or variable read from a configuration
file). If the other information is needed it should be deduced from the
definition (using a macro or function). Either the function wait_frame()
(or rather using display.frame_time = 1000/FRAMERATE) or rate_limit()
should be rewritten to reflect this decision. Changing rate_limit()
would mean changing delay values in many parts of the code. If I hadn't
already done this (see attached patch) I would object this change as
the current code obviously works. ;-)

I am not really satisfied by using 100ms as the delay granularity when
the engine supports a granularity of 1 game frame (about 30ms at the
moment). Specifying delays in game frames is not very good, because
it is easier to think in (milli)seconds and adjusting the framerate of
the game should not change the gameplay. Specifying delays in units of
10ms makes good use of the SDL timing capabilies. Therefore I suggest

1) using 10ms as time unit to specify delays,
2) defining the duration of one game frame in ms.

(Suggestion 1 would mean to adjust rate_limit() to use 10ms time units
independent of Suggestion 2.)

The attached patch implements this (except the delays I missed... ;),
but does not yet adjust most of the comments (in some cases comments and
values used do not correspond because of an assumed time unit of 100ms
and an implemented one of 90ms).

I would like to read your comments. :-)

Erik
-------------- next part --------------
diff -uNrB trunk/share/levels/0/settings timing/share/levels/0/settings
--- trunk/share/levels/0/settings	2005-08-28 21:15:41.000000000 +0200
+++ timing/share/levels/0/settings	2005-09-01 05:38:36.000000000 +0200
@@ -12,5 +12,5 @@
 generator player biplane 700 500 0 objtag=p2plane
 
 # fuel at 4 sec intervals, 6 barrels max (teaches people to take them)
-generator singleton fuel 400 520 6 timeout=40
+generator singleton fuel 400 520 6 timeout=360
 
diff -uNrB trunk/share/levels/1/settings timing/share/levels/1/settings
--- trunk/share/levels/1/settings	2005-08-19 22:24:02.000000000 +0200
+++ timing/share/levels/1/settings	2005-09-01 05:39:43.000000000 +0200
@@ -8,18 +8,18 @@
 sprite bouncer 600 200 vy=1
 
 # leader + a couple of other birds, at 3 sec intervals
-generator multi bird 400 300 3 rx=240 ry=120 vx=2 vy=2 timeout=30
+generator multi bird 400 300 3 rx=240 ry=120 vx=2 vy=2 timeout=270
 
 # ...and two clouds
-generator singleton cloud 80 250 99 vx=0.5 ry=150 timeout=50
-generator singleton cloud 720 250 99 vx=-0.5 ry=150 timeout=70
+generator singleton cloud 80 250 99 vx=0.5 ry=150 timeout=450
+generator singleton cloud 720 250 99 vx=-0.5 ry=150 timeout=630
 
 # players: sprite x y lives [keys]
 generator player biplane 100 500 3 objtag=p1plane
 generator player biplane 700 500 3 objtag=p2plane
 
 # infinitely fuel at 5 sec intervals (after previous barrel taken)
-generator singleton fuel 400 520 0 timeout=50
+generator singleton fuel 400 520 0 timeout=450
 
 # level ends when 5 birds are killed
 trigger level-done spritekill bird 5 "msg=Five birds killed!"
diff -uNrB trunk/share/levels/2/settings timing/share/levels/2/settings
--- trunk/share/levels/2/settings	2005-08-19 22:24:14.000000000 +0200
+++ timing/share/levels/2/settings	2005-09-01 05:40:01.000000000 +0200
@@ -13,8 +13,8 @@
 generator player biplane 600 250 3 objtag=p2plane
 
 # fuel at 5 sec intervals, five barrels total
-generator singleton fuel 200 380 5 timeout=50
-generator singleton fuel 600 380 5 timeout=50
+generator singleton fuel 200 380 5 timeout=450
+generator singleton fuel 600 380 5 timeout=450
 
 # bottom penguins appear when player takes fuel first time
 generator singleton pingu 200 400 1  vx=1 gentag=p1gen objtag=p1pingu active=0
@@ -27,8 +27,8 @@
 trigger level-done proximity 600 250 50 o p2pingu "msg=Pingu came home"
 
 # boxes come when balloon is killed
-generator multi box 100 50 99 gentag=box1gen timeout=40 active=0
-generator multi box 700 50 99 gentag=box2gen timeout=40 active=0
+generator multi box 100 50 99 gentag=box1gen timeout=360 active=0
+generator multi box 700 50 99 gentag=box2gen timeout=360 active=0
 trigger box1gen,box2gen tagkill balloon
 
 # level restarts if no player is alive or at least one has died
diff -uNrB trunk/share/levels/3/settings timing/share/levels/3/settings
--- trunk/share/levels/3/settings	2005-08-19 22:24:23.000000000 +0200
+++ timing/share/levels/3/settings	2005-09-01 05:40:29.000000000 +0200
@@ -28,5 +28,5 @@
 generator player biplane 645 80 3 vx=-2
 
 # and some fuel at 10 sec intervals
-generator singleton fuel 120 120 5 timeout=100
-generator singleton fuel 680 120 5 timeout=100
+generator singleton fuel 120 120 5 timeout=900
+generator singleton fuel 680 120 5 timeout=900
diff -uNrB trunk/share/levels/5/settings timing/share/levels/5/settings
--- trunk/share/levels/5/settings	2005-08-19 22:26:32.000000000 +0200
+++ timing/share/levels/5/settings	2005-09-01 05:40:40.000000000 +0200
@@ -26,4 +26,4 @@
 generator player biplane 520 180 3 tag=p2plane
 
 # and fuel at 5 sec intervals
-generator singleton fuel 400 450 5 timeout=50
+generator singleton fuel 400 450 5 timeout=450
diff -uNrB trunk/src/core/display.c timing/src/core/display.c
--- trunk/src/core/display.c	2005-08-19 22:31:15.000000000 +0200
+++ timing/src/core/display.c	2005-09-01 03:09:56.000000000 +0200
@@ -5,7 +5,7 @@
 
 struct display display = 
   {
-    .frame_time = FRAMERATE,
+    .frame_time = FRAMETIME,
     .use_alpha = 1
   };
 
diff -uNrB trunk/src/core/display.h timing/src/core/display.h
--- trunk/src/core/display.h	2005-08-19 22:31:15.000000000 +0200
+++ timing/src/core/display.h	2005-09-01 03:09:56.000000000 +0200
@@ -4,7 +4,7 @@
 #include <SDL.h>
 #include "image.h"
 
-#define FRAMERATE 30
+#define FRAMETIME 30
 #define DIRTY_TILE_SIZE 64
 
 extern struct display
diff -uNrB trunk/src/engine/schedule.c timing/src/engine/schedule.c
--- trunk/src/engine/schedule.c	2005-08-31 07:28:41.000000000 +0200
+++ timing/src/engine/schedule.c	2005-09-01 06:05:23.000000000 +0200
@@ -93,8 +93,7 @@
 {
   if (now >= *timer)
     {
-      /* convert milliseconds to frames */
-      *timer = now + delay * FRAMERATE / 10;
+      *timer = now + ms_to_frames(delay * 10);
       return 1;
     }
   else
diff -uNrB trunk/src/engine/schedule.h timing/src/engine/schedule.h
--- trunk/src/engine/schedule.h	2005-08-19 22:31:03.000000000 +0200
+++ timing/src/engine/schedule.h	2005-09-01 06:06:54.000000000 +0200
@@ -18,9 +18,12 @@
 int frame_nr(void);
 /* Return 1 and set the timer if more than delay passed since last time,
    else return 0 (meaning "try later"). 
-   'delay' is in milliseconds. 
+   'delay' is in 10 milliseconds. 
 */
 int rate_limit(unsigned int delay, unsigned int *timer);
-
-
+/* convert milliseconds to frames */
+static INLINE unsigned int ms_to_frames(unsigned int ms)
+{
+  return ms / FRAMETIME;
+}
 #endif
diff -uNrB trunk/src/engine/sprite.c timing/src/engine/sprite.c
--- trunk/src/engine/sprite.c	2005-08-19 22:31:03.000000000 +0200
+++ timing/src/engine/sprite.c	2005-09-01 06:05:22.000000000 +0200
@@ -63,7 +63,7 @@
 
 void sprite_alarm(unsigned int delay, sprite_t *target, msg_t msg)
 {
-  schedule(delay*FRAMERATE/10,SCHED_NORMAL,send_alarm,msg,target);
+  schedule(ms_to_frames(delay*10),SCHED_NORMAL,send_alarm,msg,target);
 }
 
 /* Frame checklist
diff -uNrB trunk/src/sprites/biplane.c timing/src/sprites/biplane.c
--- trunk/src/sprites/biplane.c	2005-08-19 22:31:28.000000000 +0200
+++ timing/src/sprites/biplane.c	2005-09-01 05:44:49.000000000 +0200
@@ -19,9 +19,9 @@
 #define FUEL_MAX_AMOUNT 1000
 #define BOMB_MAX_COUNT  12
 
-/* how often (in 10 ms) plane can do something */
-#define PLANE_SAY_DELAY 5
-#define PLANE_PUFF_DELAY 2
+/* delay between 2 actions in units of 10ms */
+#define PLANE_SAY_DELAY 45
+#define PLANE_PUFF_DELAY 18
 
 typedef struct biplane_model
 {
@@ -62,8 +62,8 @@
 {
   .engine_strength = 0.08,
   .turn_amount = 0.05,
-  .bomb_delay = 12,	/* N * 10ms */
-  .bullet_delay = 3,
+  .bomb_delay = 108,	/* N * 10ms */
+  .bullet_delay = 27,
   .hitpoints = 30,
   .mass = 1,
   .air_isotropic = 0.004,
@@ -93,8 +93,8 @@
 {
   .engine_strength = 0.08,
   .turn_amount = 0.05,
-  .bomb_delay = 12,	/* N * 10ms */
-  .bullet_delay = 3,
+  .bomb_delay = 108,	/* N * 10ms */
+  .bullet_delay = 27,
   .hitpoints = 30,
   .mass = 1,
   .air_isotropic = 0.000,
@@ -207,7 +207,7 @@
 	    {
 	      set_model((struct biplane *)s,((struct biplane *)s)->model->crashing_model);
 	    }
-	  sprite_alarm(200,s,msg_kill());
+	  sprite_alarm(1800,s,msg_kill());
 	}
     }
   mech_update(ms);
@@ -219,7 +219,7 @@
   if (s->state & BIPLANE_CRASHING && !(s->state & BIPLANE_DYING))
     {
       s->state |= BIPLANE_DYING;
-      sprite_alarm(30,s,msg_kill());
+      sprite_alarm(270,s,msg_kill());
     }
 }
 
diff -uNrB trunk/src/sprites/bomb.c timing/src/sprites/bomb.c
--- trunk/src/sprites/bomb.c	2005-09-01 02:39:52.000000000 +0200
+++ timing/src/sprites/bomb.c	2005-09-01 05:45:08.000000000 +0200
@@ -33,7 +33,7 @@
   ((mech_sprite_t *)s)->gravity = 0.45;
   ((mech_sprite_t *)s)->air_resistance_ang = 0.01;
   
-  sprite_alarm(5,s,msg_activate());
+  sprite_alarm(45,s,msg_activate());
   return s;
 }
 
diff -uNrB trunk/src/sprites/bonusmachine.c timing/src/sprites/bonusmachine.c
--- trunk/src/sprites/bonusmachine.c	2005-08-19 22:31:28.000000000 +0200
+++ timing/src/sprites/bonusmachine.c	2005-09-01 05:47:02.000000000 +0200
@@ -57,7 +57,7 @@
 	mech_defaults((mech_sprite_t *)s);
 	((mech_sprite_t *)s)->rmass = 0.000000001;
 	((mech_sprite_t *)s)->gravity = 0;
-	sprite_alarm(100,s, msg_fire());
+	sprite_alarm(900,s, msg_fire());
 	s->animation = machine;
 	return s;
 }
@@ -70,7 +70,7 @@
 	case MSG_FIRE:
 		/* proceed with machine animation */
 		sprite_set_animation(s, machine_open);
-		sprite_alarm(300, s, msg_fire());
+		sprite_alarm(2700, s, msg_fire());
 		break;
 	default:
 		return MSG_RET_UNKNOWN;
@@ -210,7 +210,7 @@
 	 * (and itself)
 	 */
 	sprite_msg(b, msg_set_target(s));
-	sprite_alarm(120, b, msg_kill());
+	sprite_alarm(1080, b, msg_kill());
 }
 
 static int ring_setup(void)
@@ -226,7 +226,7 @@
 	sprite_t *s;
 	s = obj_alloc(sizeof(sprite_t), &bonusring.object_type);
 	/* wait a sec before showing the bonus */
-	sprite_alarm(10, s, msg_fire());
+	sprite_alarm(90, s, msg_fire());
 	s->animation = ring;
 	return s;
 }
diff -uNrB trunk/src/sprites/draco.c timing/src/sprites/draco.c
--- trunk/src/sprites/draco.c	2005-08-19 22:31:28.000000000 +0200
+++ timing/src/sprites/draco.c	2005-09-01 05:47:41.000000000 +0200
@@ -50,7 +50,7 @@
 		 */
 		s->state = DRACO_RIGHT;
 		s->flags &= ~SPRITE_PAUSED;
-		sprite_alarm(40,s,msg_deactivate());
+		sprite_alarm(360,s,msg_deactivate());
 	}
 	x = 0;
 	y = 0;
@@ -82,12 +82,12 @@
 		/* going down */
 		if (s->vel.y > -1) {
 			s->state = DRACO_UP;
-			sprite_alarm(30,s,msg_deactivate());
+			sprite_alarm(270,s,msg_deactivate());
 		} else {
 			/* going up */
 			if (s->vel.y < 0) {
 				s->state = DRACO_DOWN;
-				sprite_alarm(20,s,msg_deactivate());
+				sprite_alarm(180,s,msg_deactivate());
 			}
 			/* otherwise just flap */
 		}
diff -uNrB trunk/src/sprites/hippo.c timing/src/sprites/hippo.c
--- trunk/src/sprites/hippo.c	2005-08-19 22:31:28.000000000 +0200
+++ timing/src/sprites/hippo.c	2005-09-01 05:48:02.000000000 +0200
@@ -26,7 +26,7 @@
 	((mech_sprite_t *)s)->air_resistance = 0.0001;
 	((mech_sprite_t *)s)->gravity = 0.4;  /* and full of gas */
 	((mech_sprite_t *)s)->bounce = 1.0;
-	sprite_alarm(10, s, msg_activate());
+	sprite_alarm(90, s, msg_activate());
 	s->state = get_random() % 8;
 	return s;
 }
@@ -53,7 +53,7 @@
 	case MSG_ACTIVATE:
 		/* randomize rotation direction */
 		s->state = get_random() % 8;
-		sprite_alarm(20, s, msg_activate());
+		sprite_alarm(180, s, msg_activate());
 		break;
 	case MSG_OUTOFBOUNDS:
 		fprintf(stderr, "Hippo: Out of screen (top or bottom)!\n");
diff -uNrB trunk/src/sprites/ufo.c timing/src/sprites/ufo.c
--- trunk/src/sprites/ufo.c	2005-08-19 22:31:28.000000000 +0200
+++ timing/src/sprites/ufo.c	2005-09-01 05:48:22.000000000 +0200
@@ -59,7 +59,7 @@
 		/* fire three missiles before quitting */
 		us->missiles = 3;
 		ms->lin_impulse.y = 0.02;
-		sprite_alarm(100, s, msg_fire());
+		sprite_alarm(900, s, msg_fire());
 	}
 	/* when to turn */
 	if (s->pos.x + TURN_DIST < us->middle) {
@@ -105,7 +105,7 @@
 		m->vel.y = 0.1;
 
 		/* queue next missile */
-		sprite_alarm(100, s, msg_fire());
+		sprite_alarm(900, s, msg_fire());
 		break;
 	case MSG_KILL:
 		sprite_kill(s);
diff -uNrB trunk/src/sprites/zeppelin.c timing/src/sprites/zeppelin.c
--- trunk/src/sprites/zeppelin.c	2005-08-19 22:31:28.000000000 +0200
+++ timing/src/sprites/zeppelin.c	2005-09-01 05:48:38.000000000 +0200
@@ -55,7 +55,7 @@
 			if (s->state > WRECKED + 8 && s->state != KILLED) 
 			{
 				create_effect(&explosion, vector(s->pos.x, s->pos.y + 20));
-				sprite_alarm(20,s,msg_kill());
+				sprite_alarm(180,s,msg_kill());
 				s->state = KILLED;
 			}
 		}
diff -uNrB trunk/src/tests/testengine.c timing/src/tests/testengine.c
--- trunk/src/tests/testengine.c	2005-08-19 22:30:42.000000000 +0200
+++ timing/src/tests/testengine.c	2005-09-01 03:09:56.000000000 +0200
@@ -58,7 +58,7 @@
   if (reason) {
     ui_message(reason,400,300,ALIGN_CENTER,big_font,20); 
   }
-  for (idx = 0; idx < 2*FRAMERATE; idx++) {
+  for (idx = 0; idx < 2*FRAMETIME; idx++) {
     /* update screen while waiting */
     draw_frame();
     game_frame();      


More information about the airstrike mailing list