search

revery-ui / revery

:zap: Native, high-performance, cross-platform desktop apps - built with Reason!
https://www.outrunlabs.com/revery/
MIT License
8.06k stars 196 forks source link

refactor(native): use wrapped pointers #992

Closed zbaylin closed 3 years ago

zbaylin commented 4 years ago

This is currently breaking very early on -- I get a segfault with the Window in SDL2 and I can't seem to figure out why.

Here's a stack trace from LLDB:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x1400)
  * frame #0: 0x00000001000a2d2d Examples`SDL_GetWindowID_REAL(window=0x0000000000001400) at SDL_video.c:1719:5 [opt]
    frame #1: 0x0000000100b0a325 Examples`resdl_SDL_GetWindowId(vWindow=4432065872) at sdl2_wrapper.cpp:1539:18
    frame #2: 0x00000001002707f0 Examples`camlRevery_Core__Window__create_1627 + 768
    frame #3: 0x0000000100273656 Examples`camlRevery_Core__App__createWindow_inner_1628 + 22
    frame #4: 0x00000001000f02a1 Examples`camlExamples__init_1444 + 1361
    frame #5: 0x000000010027386c Examples`camlRevery_Core__App__start_951 + 284
    frame #6: 0x0000000100b599b8 Examples`caml_start_program + 72

If anybody has any ideas, let me know! I'll keep looking at it.

bryphe commented 4 years ago

Thanks for the help with this, Zach! Much appreciated

zbaylin commented 4 years ago

@bryphe -- I think I implemented all the fixes I caught (including the ones you mentioned), but I'm still getting a segfault on GL_Setup, which is called right after window creation.

zbaylin commented 4 years ago

@bryphe after our conversation last night I realized that was probably why I couldn't get this PR working. I changed the wrap and unwrap functions and, lo and behold, it works! Let me know if there is anything else you need me to do!

bryphe commented 3 years ago

OSX looks good! Wasn't sure if you already tested on Windows / Linux - let me know if I can help validate either of those platforms

zbaylin commented 3 years ago

@bryphe I just tested Linux with both the example app and Oni2, and both work! @CrossR mentioned that he might be able to help test on Windows as well

CrossR commented 3 years ago

The Icon Features bit (progress displayed on the Icon) example crashes for me on Windows with:

Fatal error: exception Invalid_argument("compare: abstract value")
Raised by primitive operation at file "examples/NativeIconExample.re", line 23, characters 11-15
Called from file "lib/Hooks.re", line 235, characters 11-70
Called from file "lib/Hooks.re", line 307, characters 17-48
Called from file "lib/HeterogenousList.re", line 94, characters 16-45
Called from file "lib/Brisk_reconciler_internal.re", line 975, characters 14-158
Called from file "lib/Brisk_reconciler_internal.re", line 735, characters 12-235
Called from file "lib/Brisk_reconciler_internal.re", line 1346, characters 14-401
Called from file "lib/Brisk_reconciler_internal.re", line 1090, characters 16-497
Called from file "lib/ListTR.re", line 32, characters 22-40
Called from file "lib/Brisk_reconciler_internal.re", line 1067, characters 10-1023
Called from file "lib/Brisk_reconciler_internal.re", line 915, characters 14-785
Called from file "lib/Brisk_reconciler_internal.re", line 735, characters 12-235
Called from file "lib/Brisk_reconciler_internal.re", line 1346, characters 14-401
Called from file "lib/Brisk_reconciler_internal.re", line 1090, characters 16-497
Called from file "lib/ListTR.re", line 32, characters 22-40
Called from file "lib/Brisk_reconciler_internal.re", line 1067, characters 10-1023
Called from file "lib/Brisk_reconciler_internal.re", line 915, characters 14-785
Called from file "lib/Brisk_reconciler_internal.re", line 735, characters 12-235
Called from file "lib/Brisk_reconciler_internal.re", line 1189, characters 12-180
Called from file "lib/Brisk_reconciler_internal.re", line 862, characters 12-236
Called from file "lib/Brisk_reconciler_internal.re", line 735, characters 12-235
Called from file "lib/Brisk_reconciler_internal.re", line 1545, characters 8-42
Called from file "lib/Brisk_reconciler_internal.re", line 1606, characters 6-124
Called from file "src/UI/Container.re", line 38, characters 10-64
Called from file "src/UI/Render.re", line 27, characters 17-56
Called from file "src/UI/Render.re", line 26, characters 2-99
Called from file "src/Core/Window.re", line 389, characters 2-17
Called from file "list.ml", line 110, characters 12-15
Called from file "src/Core/App.re", line 298, characters 6-123
Called from file "packages/reason-sdl2/src/sdl2.re", line 822, characters 10-20
Called from file "packages/reason-sdl2/src/sdl2.re", line 268, characters 17-59
Called from file "packages/reason-sdl2/src/sdl2.re", line 268, characters 17-59
Called from file "examples/Examples.re", line 378, characters 0-15

It doesn't crash on master for me, so I assume that its because of these changes? Unless its just because this PR is lagging behind master a bit...

CrossR commented 3 years ago

On Oni2 I'm not getting any crashes (from a basic opening it up and opening a few files etc) but I am missing out on window handles (or whatever the name is). I.e. the ability to drag the window around, and the resize on the edges.

CrossR commented 3 years ago

Revery example is happy with the latest changes, I'm getting a crash now for Oni2 though:

[ERROR] +1524ms Oni2.Exception : Exception (Invalid_argument "compare: abstract value"):
Raised by primitive operation at file "src/UI/HitTest.re", line 10, characters 12-52
Called from file "src/Core/App.re", line 313, characters 16-43
Called from file "src/Core/App.re", line 313, characters 16-43
Called from file "packages/reason-sdl2/src/sdl2.re", line 822, characters 10-20
Called from file "packages/reason-sdl2/src/sdl2.re", line 268, characters 17-59
Called from file "packages/reason-sdl2/src/sdl2.re", line 268, characters 17-59
Called from file "src/bin_editor/Oni2_editor.re", line 497, characters 11-26
bryphe commented 3 years ago

Thanks @CrossR !

Looks like the crash is occurring when we try to compare windows:

        if (sdlWindow == Window.getSdlWindow(window) && node#hasRendered()) {

Which, because they are wrapped in Abstract_val, is expected!

We'll need to have another way to compare windows - like using the https://wiki.libsdl.org/SDL_GetWindowID API

bryphe commented 3 years ago

This is looking good now! Just need another round of cross-plat testing to make sure everything is OK

zbaylin commented 3 years ago

@bryphe I just merged this up with master and tested the example app and Oni out on macOS. All seemed to work well! I can also test on Linux and Windows!

I think it would be a good idea to get this merged before I start work on the native menus, since that will require passing pointers back and forth from the FFI a decent amount.

zbaylin commented 3 years ago

Windows works with both the Examples and Oni as of f2d6793 (the Window compare fix)

bryphe commented 3 years ago

@bryphe I just merged this up with master and tested the example app and Oni out on macOS. All seemed to work well! I can also test on Linux and Windows!

Excellent! Sounds like you already verified on Windows - I'll try out on Linux.

I think it would be a good idea to get this merged before I start work on the native menus, since that will require passing pointers back and forth from the FFI a decent amount.

Right, that makes sense - I was just thinking about this change in the context of the NSObject PR :+1:

bryphe commented 3 years ago

Linux looks good! Sounds like we're all set with this :+1: