Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

al_draw_line coordinates are off when resizing a window on MacOS #1410

Closed
allefant opened this issue Feb 20, 2023 · 10 comments · Fixed by #1412
Closed

al_draw_line coordinates are off when resizing a window on MacOS #1410

allefant opened this issue Feb 20, 2023 · 10 comments · Fixed by #1412

Comments

@allefant
Copy link
Contributor

Tried the Allegro examples on my old work Mac and one thing I noticed is that ex_resize looks off. For example:

Screen.Recording.2023-02-20.at.10.42.45.AM.mov

I have not investigated yet but ex_resize2 works fine so it's not resizing in general that's broken, but maybe something with al_draw_line? I checked the source of ex_resize and I don't see anything obvious but seemingly the al_draw_line commands assume a larger display than what it is.

Model Name: MacBook Pro
Model Identifier: MacBookPro16,1
Processor Name: 8-Core Intel Core i9
System Version: macOS 12.6 (21G115)

@allefant
Copy link
Contributor Author

allefant commented Feb 21, 2023

The plot thickens. In a weird way. I added a call to draw a picture in ex_resize (like in ex_resize2) and that makes al_draw_line work. But even stranger, I made it so the space key switches between drawing the picture or not. While the picture is being drawn resize works. When only lines are drawn resize is broken.

It seems like al_draw_bitmap() does something which corrects the display size, while al_draw_line() does not do so and somehow the display size is too large.

Here's my modified ex_resize where I can switch working resize on and off with the space key:

#include "allegro5/allegro.h"
#include <allegro5/allegro_primitives.h>
#include "allegro5/allegro_image.h"

#include "common.c"

ALLEGRO_BITMAP *bmp;
ALLEGRO_DISPLAY *display;
bool draw_picture = true;

static void redraw(void)
{
    ALLEGRO_COLOR black, white;
    int w, h;

    white = al_map_rgba_f(1, 1, 1, 1);
    black = al_map_rgba_f(0, 0, 0, 1);

    al_clear_to_color(white);

    if (draw_picture) {
        al_draw_scaled_bitmap(bmp,
                0, 0, al_get_bitmap_width(bmp), al_get_bitmap_height(bmp),
                0, 0, al_get_display_width(display), al_get_display_height(display),
                0);
    }
    
    w = al_get_display_width(display);
    h = al_get_display_height(display);
    al_draw_line(0, h, w / 2, 0, black, 0);
    al_draw_line(w / 2, 0, w, h, black, 0);
    al_draw_line(w / 4, h / 2, 3 * w / 4, h / 2, black, 0);
    al_flip_display();
}

int main(int argc, char **argv)
{
    
    ALLEGRO_TIMER *timer;
    ALLEGRO_EVENT_QUEUE *events;
    ALLEGRO_EVENT event;
    

    (void)argc;
    (void)argv;

    if (!al_init()) {
        abort_example("Could not init Allegro.\n");
    }
    al_init_primitives_addon();
    al_init_image_addon();
    events = al_create_event_queue();

    /* Setup a display driver and register events from it. */
    al_set_new_display_flags(ALLEGRO_RESIZABLE|
      ALLEGRO_GENERATE_EXPOSE_EVENTS);
    display = al_create_display(640, 480);
    if (!display) {
        abort_example("Could not create display.\n");
    }
    al_register_event_source(events, al_get_display_event_source(display));
    
    bmp = al_load_bitmap("data/mysha.pcx");
    if (!bmp) {
      abort_example("Unable to load image\n");
    }

    timer = al_create_timer(1.0 / 60);
    al_start_timer(timer);

    al_install_keyboard();
    al_register_event_source(events, al_get_keyboard_event_source());
    al_register_event_source(events, al_get_timer_event_source(timer));

    redraw();
    while (true) {
        
        al_wait_for_event(events, &event);
        if (event.type == ALLEGRO_EVENT_TIMER) {
            redraw();
        }
        else if (event.type == ALLEGRO_EVENT_DISPLAY_RESIZE) {
            al_acknowledge_resize(event.display.source);
        }
        else if (event.type == ALLEGRO_EVENT_DISPLAY_CLOSE) {
            break;
        }
        else if (event.type == ALLEGRO_EVENT_KEY_DOWN) {
            if (event.keyboard.keycode == ALLEGRO_KEY_ESCAPE) break;
            if (event.keyboard.keycode == ALLEGRO_KEY_SPACE) draw_picture = !draw_picture;
        }
    }

    return 0;
}

@allefant
Copy link
Contributor Author

It looks like under MacOS when a window gets resized it happens on the main thread - but _al_ogl_setup_gl then checks for the current target bitmap: https://github.com/liballeg/allegro5/blob/master/src/opengl/ogl_display.c#L46

Which is of course always NULL on the main thread. Need to investigate and not sure yet how to fix.

This also may be related to the red flashing while resizing, maybe it's MacOS telling us we are calling things from a wrong thread?

@beoran
Copy link
Contributor

beoran commented Feb 21, 2023

Yes, the red flashing is a sign we are definitely doing something wrong...

@pedro-w
Copy link
Contributor

pedro-w commented Feb 21, 2023

See also #580, #1350 about the flickering. Also I feel like I did something (at least filed an issue) about resizing in ex_resize2 but can't find anything :(
I believe recent versions of MacOS print a message to the console when calling functions from the wrong thread, have you seen any of these?
[edit] aha
https://www.allegro.cc/forums/thread/618421
unfortunately never got any further with it!

@allefant
Copy link
Contributor Author

The red flashing also happens in release mode.

There is nothing printed in the console and nothing in allegro.log - I just used lldb to step through and saw that after the resize it sets up OpenGL in the wrong thread. Somehow al_draw_bitmap must do something that re-initializes OpenGL properly but haven't had time to look into it further.

I'm also not exactly sure what the design is for MacOS. I think we try to make OpenGL calls from both the main thread and the user thread (i.e. the one with the main() function, which is not run on the main thread, confusingly). To do so we switch the OpenGL context to the appropriate thread as needed. But we expect Allegro calls (like al_get_current_display) to be made from the user thread only. Hopefully there already is a simple solution and is already used by al_draw_bitmap and so just has to be applied to al_draw_line as well.

@pedro-w
Copy link
Contributor

pedro-w commented Feb 21, 2023

I'll take a look at this but first off I've come across #1411, aieee!

@allefant
Copy link
Contributor Author

I believe I found out the reason for the red and for al_draw_bitmap fixing the viewport.

al_draw_bitmap sometimes calls al_use_transform, which will restore the viewport: https://github.com/liballeg/allegro5/blob/master/src/bitmap_draw.c#L117

So what happens on resize is, Allegro attempts to restore the viewport, but fails since it is on the wrong thread (the problem is not the OpenGL context which we switch to the other thread, but Allegro's tls will not be available and so there won't be any current display).

The next time the window is redrawn al_clear_to_color() will have the wrong opengl viewport (just like al_draw_line) and so only clear a portion of the window. Then when al_draw_flip() is called the uninitialized part of the framebuffer helpfully is left red by MacOS. Since al_draw_bitmap was also called the frame after that everything will be fine again.

@pedro-w
Copy link
Contributor

pedro-w commented Feb 24, 2023

Great work on the resizing! I found a few links from the flutter project relating to red flashing on resize for MacOS. But I don't really understand their explanation and solution. What do you think?
flutter/flutter#35078
https://docs.google.com/document/d/1allwMZXgX9gGVPguFy3-XydjXEIJgYR1Uhz8Vhm9Rrs/edit#heading=h.pub7jnop54q0
flutter/engine#21525

@allefant
Copy link
Contributor Author

Interesting. I don't understand either, but part of it seems to involve rendering into an offscreen buffer instead of the macos window?

I found a local fix for my game to eliminate the red flicker - which is to basically stop drawing until the resize is done.

It is necessary to distinguish the following two ALLEGRO_DISPLAY_RESIZE events for it:

https://github.com/liballeg/allegro5/blob/master/src/macosx/osxgl.m#L595-L605
https://github.com/liballeg/allegro5/blob/master/src/macosx/osxgl.m#L527-L534

When the first one (from windowDidResize) is received by my game I immediately stop calling any graphics functions (especially al_acknowledge_resize and al_flip_display). And only when the second one (from viewDidEndLiveResize) is received I call al_acknowledge_resize and enable rendering again.

The only drawback is that during the entire resize MacOS just pixel-scales the contents of whatever the window was when the resize starts. But still looks much better than the red flickering.

I propose adding a field to ALLEGRO_EVENT_DISPLAY_RESIZE:

bool in_progress;

It would always be false except on MacOS where it would be true for the events created from windowDidResize. In my game when I receive the first in_progress event I would stop rendering and only on the final one I would handle the actual resize.

@allefant
Copy link
Contributor Author

allefant commented Mar 4, 2023

I found a few links from the flutter project

Actually, building Allegro with ALLEGRO_SDL fixes the red flashing. Maybe the SDL sources are easier to read than Flutter sources - each of them does resizing the correct way. Just we don't :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants