[AD] [ alleg-Bugs-1692717 ] some bugs

[SourceForge.net <no...@so...>]

Bugs item #1692717, was opened at 2007-04-02 08:25
Message generated for change (Comment added) made by scherno2000
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105665&aid=1692717&group_id=5665

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: erno (scherno2000)
Assigned to: Nobody/Anonymous (nobody)
Summary: some bugs

Initial Comment:
1. rest(0) does not yield.
2. in fbcon.c console_users is incremented twice, but decremented only once.
3. If allegro application is run in console mode, and get_tty() returns 0 (running application from midnight commander), problems with threads created before fork().
4. If allegro application is run in console mode, from midnight commander, then the shell thinks the application is ended too early because of main process is killed in allegro initialization.

Please read the readme file in attached zip file.


----------------------------------------------------------------------

>Comment By: erno (scherno2000)
Date: 2007-04-10 17:05

Message:
Logged In: YES 
user_id=1542934
Originator: YES

Yes, this is a problem. The things get worse if the triangle is more
elongated. 

{
 10,10,
 50,20,
 30,30
}


I think this is because of  offset added in fill_edge_structure() that are
different if edge->dx is positive or negative. I think the problem can be
solved adjusting the offset, if direction is negative.
I have no more time today, I will test it tomorrow.


----------------------------------------------------------------------

Comment By: Elias Pschernig (elias)
Date: 2007-04-10 16:30

Message:
Logged In: YES 
user_id=32894
Originator: NO

Actually, played some more with it, and this patch doesn't fix the
problem, e.g. the following polygon now looks worse than before.. do you
have any ideas why?

#include <allegro.h>

int main(void)
{
    allegro_init();
    install_keyboard();
    install_timer();

    set_gfx_mode(0, 640, 480, 0, 0);

    clear_to_color(screen, 0);

    int v[] = {
        10, 10,
        30, 20,
        10, 30
    };
    polygon(screen, 3, v, 12);

    readkey();

    return 0;
}

----------------------------------------------------------------------

Comment By: Elias Pschernig (elias)
Date: 2007-04-10 16:12

Message:
Logged In: YES 
user_id=32894
Originator: NO

I think I understand. In my tests, polygon() got slightly slower by this -
I wonder if there's a way to fix it in a way which doesn't cause a
slowdown.

I committed it in any case, having polygon finally work correctly is more
important than being fast. Now as for possible glitches in old programs who
work around the bug - we can just revert if it turns out to be a real
problem.

----------------------------------------------------------------------

Comment By: erno (scherno2000)
Date: 2007-04-10 15:29

Message:
Logged In: YES 
user_id=1542934
Originator: YES

I try to describe the main idea in my loop:

The old _show_polygon() works fine excepting the bottom lines or vertices,
where are missing points, or horizontal lines.
You can imagine the same polygon mirrored by a horizontal axis. Then every
bottom will be top, and every top will be bottom. The same algorithm will
give correct results for every line, that is not bottom on the mirrored
polygon, or equivalently, not on the top of the original polygon.
Making an union of drawn segments of corresponding lines of original and
mirrored polygons, will give correct results for any case.
The algorithm makes exactly this:
The first
  if(edge->bottom != c)
  {
    up=1-up;
  }
simulates drawing like in original algorithm ignoring bottoms(they were
discarded before in original algorithm), if up==1 then is must be drawn,
else no.

The second
  if(edge->top != c)
  {
    dn=1-dn;
  }
simulates drawing of the same line, but viewed as the mirrored polygon.
The bottom of mirrored polygon is top on the original.
If dn==1 should draw, else no.

The next if checks if the union of up and dn is a beginning of a
horizontal line(if last up+dn==0 and current up+dn!=0) then must begin a
new line and x is saved.
If last up+dn!=0 and current up+dn==0, then line ends, and it will really
draw in the bitmap from saved x to the endpoint.

I hope you understand my not too good English.

----------------------------------------------------------------------

Comment By: Elias Pschernig (elias)
Date: 2007-04-10 13:41

Message:
Logged In: YES 
user_id=32894
Originator: NO

The result of the polygon test looks nice, I'm not sure I follow the
change though.. could you describe what was wrong in the edge loop and how
you fixed it?

----------------------------------------------------------------------

Comment By: Milan Mimica (mmimica)
Date: 2007-04-10 13:11

Message:
Logged In: YES 
user_id=1171214
Originator: NO

Commited the fix for the display_switch_lock() problem. Thanks!

----------------------------------------------------------------------

Comment By: erno (scherno2000)
Date: 2007-04-10 12:16

Message:
Logged In: YES 
user_id=1542934
Originator: YES

I agree with you.
I corrected the polygon.c, now should be correct.
I sent the new polygon.c and the diff file too, with the test program.

About console: I discovered another dependency: when set_gfx_mode is
called, then console mus't be already initialized, because
display_switch_lock uses __al_linux_console_fd.
I corrected this by defining the sys_linux_save_console_state() and
sys_linux_restore_console_state() in lsystem.c.

For files see the attachment.

File Added: temp.zip

----------------------------------------------------------------------

Comment By: Elias Pschernig (elias)
Date: 2007-04-06 09:51

Message:
Logged In: YES 
user_id=32894
Originator: NO

>From what I remember (could be wrong), the wanted behavior is like this:

polygon() should have all points inclusive, and work like line() or
rect(). It simply is another 2D primitive. So there indeed is a bug
currently (which is known for some years and there was also at least one
unsuccessful attempt to fix it.. it's not quite trivial to fix).

polygon_3d should be exclusive, that is, if you draw a polygon which is
part of a 3d mesh, then the polygon to the right and the polygon to the
bottom of the first polygon should fit without overlap (so e.g. you can
draw a transparent 3d mesh without getting artefacts at the edges). This is
how e.g. OpenGL works. And it also doesn't fully work, but would be even
harder to fix than polygon(), from what I remember about the code.

----------------------------------------------------------------------

Comment By: erno (scherno2000)
Date: 2007-04-06 07:32

Message:
Logged In: YES 
user_id=1542934
Originator: YES

Please don't consider yet the polygon.c sent because the problem is not
completely solved (the case when first point isn't top or bottom is not
handled). I will send a corrected version later.

What is the reason of polygon() "bug" be considered on purpose?
Now I can't draw any symmetrical shape, because the bottom parts are
missing.



----------------------------------------------------------------------

Comment By: Peter Wang (tjaden)
Date: 2007-04-05 23:39

Message:
Logged In: YES 
user_id=28616
Originator: NO

See, for example, the second last post here:
http://www.allegro.cc/forums/thread/222223

I don't know if it's documented.  If not, that's a documentation bug.

----------------------------------------------------------------------

Comment By: Peter Wang (tjaden)
Date: 2007-04-05 23:27

Message:
Logged In: YES 
user_id=28616
Originator: NO

I believe the polygon() "bug" is actually on purpose.  Please don't commit
it until this is checked.

----------------------------------------------------------------------

Comment By: erno (scherno2000)
Date: 2007-04-05 16:27

Message:
Logged In: YES 
user_id=1542934
Originator: YES

OK, I'm attaching the diff file for polygon.c


File Added: polygon.diff

----------------------------------------------------------------------

Comment By: Elias Pschernig (elias)
Date: 2007-04-05 15:49

Message:
Logged In: YES 
user_id=32894
Originator: NO

Oh, polygon(), that will be nice to have finally fixed indeed (polygon_3d
also has a slight bug from what I remember, but a different one, much
harder to fix). Actually, as the polygon() bug has been worked around for
years, I wonder if it will break existing stuff fixing it now.. but anyway,
I'll take a look at it. Hm, and could you attach a diff instead of the
complete file? I prefer that for some reason.

----------------------------------------------------------------------

Comment By: erno (scherno2000)
Date: 2007-04-05 15:26

Message:
Logged In: YES 
user_id=1542934
Originator: YES

My name is Erno Schwetter.

I made changes to lsystem.c and lconsole.c and header file aintlnx.h: 
I removed the fork() from the init_console() and made the function
__al_init_console() to be called in sys_linux_init() before any thread
creation. The console change remained in init_console() and is made only
when necessary.
Now it works OK at me.
Some times is good to force opening new console every time, maybe could
introduce a function to tell allegro to force this.

There is a problem wit polygon.c: when drawing polygons the bottom
vertices and bottom horizontal lines are not drawn.I put a test program,
and the modified polygon.c.
polytest.cpp is the test program.

The files are attached in the zip.

File Added: files.zip

----------------------------------------------------------------------

Comment By: Milan Mimica (mmimica)
Date: 2007-04-05 12:52

Message:
Logged In: YES 
user_id=1171214
Originator: NO

1st bug: For some reason (unknown to me) allegro 4.3 uses a simplified
implementantion of rest(), as opposed to 4.2's rest(), which uses
sched_yield() if available. You're right, this should be fixed.

2nd bug: I am going to commit the fix. How do I sign you?

3rd and 4th: this only happens when running from mc. I asked at the forums
once why allergo programs cannot be ran from screen (console virtual
screens manager). I was told that allegro programs need a real console. I
guess the same applies to mc. Besides, I couldn't make it work even with
your fixes.

----------------------------------------------------------------------

Comment By: erno (scherno2000)
Date: 2007-04-02 12:57

Message:
Logged In: YES 
user_id=1542934
Originator: YES

The problem is that the background thread __al_linux_bgman_init() is
called in sys_linux_init(), that function calls bg_man_pthreads_init()
where is created the background thread. This is needed by the sound system
too. The sound will not work too if the background thread is created before
the fork. The problem with fork() is that the global variables used by
threads created are in the context of the main process, but after the
fork() the new function calls are using the new global variables.
It isn't enough to put the __al_linux_use_console() in install_timer()
because of the background thread.
Now the fork happens in keyboard_init() or the al_linux_console_graphics
call in fbcon.c, too late.

I didn't studied the source too deeply. Why it is necessary to call fork()
if one branch is immediately exited? If this is necessary can we exit from
child process instead of parent?


----------------------------------------------------------------------

Comment By: Milan Mimica (mmimica)
Date: 2007-04-02 11:43

Message:
Logged In: YES 
user_id=1171214
Originator: NO

The 2nd bug, fix in fbcon.c: OK

The 3rd bug, lsystem.c and lconsole.c: We used to init the console in
sys_linux_init(), like your fix does, but we (me) didn't want the program
to be attached to a console unless really necessary. We delayed the console
initialisation until set_gfx_mode() or install_keyboard() was called (hence
the whole console_users thing)... Now it turns out that the console is
needed because of some fork() call... can you find another way to fix this?
Could we put __al_linux_use_console() call in install_timer() like we do on
keyboard init? Where does this fork() happen exactly?



----------------------------------------------------------------------

Comment By: erno (scherno2000)
Date: 2007-04-02 10:25

Message:
Logged In: YES 
user_id=1542934
Originator: YES

If you want you can see the differences rapidly, if you search the word
ERNO in the files from zip. All of differences are active
only if ERNO is defined.
I didn't sent diffs because I wanted you to check if my modifications are
all correct, but I can send diffs if you want.


I just changed the al_rest() function like this:
when msecs<0 it calls explicitly sched_yield() instead of select(...)
to force yielding from the showa16.cpp with rest(-1), and not change
functionality if the parameter >=0.
I used the #define to keep the old functionality if I comment the 

#define ERNO 

line

the modified function:

#define ERNO
void al_rest(long msecs)
{
#ifdef ALLEGRO_MACOSX
   usleep(msecs * 1000);
#else
   struct timeval timeout;
#ifdef ERNO
   if(msecs<0)
   {
     sched_yield();
   }
   else
#endif
   { 
     timeout.tv_sec = 0;
     timeout.tv_usec = msecs * 1000;
     select(0, NULL, NULL, NULL, &timeout);
   }
#endif
}

You can check showa16.cpp modifying the rest parameter to -1, 0, 1 and
commenting rest.
With rest -1 and 1 the result will be the same (only 1 thread running)
With rest 0 and without rest results will be the same, and half value of
previous (2 threads running).


----------------------------------------------------------------------

Comment By: Peter Wang (tjaden)
Date: 2007-04-02 09:16

Message:
Logged In: YES 
user_id=28616
Originator: NO

Please generate a diffs for each change and send those if possible. It's
difficult to see what's going on and apply your changes. Thanks.


----------------------------------------------------------------------

Comment By: Elias Pschernig (elias)
Date: 2007-04-02 09:07

Message:
Logged In: YES 
user_id=32894
Originator: NO

I'll look at the .zip later. About utime.c - what did you modify and why?

----------------------------------------------------------------------

Comment By: erno (scherno2000)
Date: 2007-04-02 09:03

Message:
Logged In: YES 
user_id=1542934
Originator: YES

In the zip file is an example you can test:

showa16.cpp
.
The problem is, that select(...) with timeout == 0 does not yield to the
other thread. (See the select man page),
but the allegro documentation says that. If the timeout is <> 0 every
thing is OK. If I am calling sched_yield() instead of
select with timeout==0 every thing is OK. I tested this with the modified
version of utime.c (in the zip file) too, and changed the rest parameter
with -1.
 

----------------------------------------------------------------------

Comment By: Elias Pschernig (elias)
Date: 2007-04-02 08:32

Message:
Logged In: YES 
user_id=32894
Originator: NO

I don't know about console mode, but about rest(0), what makes you believe
what you said? From the sources (src/unix/usystem.c), this is the function
called when you do rest(0) in unix:

void _unix_yield_timeslice(void)
{
   #if defined(ALLEGRO_USE_SCHED_YIELD) &&
defined(_POSIX_PRIORITY_SCHEDULING)

      sched_yield();

   #else

      struct timeval timeout;
      timeout.tv_sec = 0;
      timeout.tv_usec = 0;
      select(0, NULL, NULL, NULL, &timeout);

   #endif
}

So normally it directly calls sched_yield (e.g. in linux), and if that's
not available, it will do a select with time 0, which also will call back
to the scheduler and give other programs a chance to run. Note that
yielding does not imply giving up any CPU time in case you expected that..

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105665&aid=1692717&group_id=5665