refactor(native): use wrapped pointers #992

zbaylin · 2020-08-25T00:31:11Z

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.

packages/reason-sdl2/src/sdl2_wrapper.cpp

bryphe · 2020-08-25T01:18:49Z

Thanks for the help with this, Zach! Much appreciated

zbaylin · 2020-08-25T02:14:44Z

@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 · 2020-09-09T22:34:38Z

@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

Code change looks great, @zbaylin - thanks for the heroic work here. One step closer to multicore... 😎

We should validate it across platforms - a sanity check running the example app Onivim 2 against this PR should be sufficient. There's some C bindings that aren't covered in our tests yet, so I just want to make sure we test those:

bryphe · 2020-10-15T23:48:41Z

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

…re/native/pointer_wrap

zbaylin · 2020-10-16T18:41:03Z

@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 · 2020-10-16T19:48:48Z

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 · 2020-10-16T21:55:57Z

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 · 2020-10-20T20:10:33Z

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 · 2020-10-20T20:49:14Z

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 · 2020-11-12T19:26:28Z

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

…re/native/pointer_wrap

…-ui/revery into chore/native/pointer_wrap

zbaylin · 2020-11-22T21:49:20Z

@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 · 2020-11-22T22:49:01Z

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

bryphe · 2020-11-24T18:34:46Z

@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 👍

bryphe · 2020-11-24T18:40:17Z

Linux looks good! Sounds like we're all set with this 👍

zbaylin added 2 commits August 24, 2020 20:17

reason-sdl2: use wrapped pointers

97ee0db

Native: use wrapped pointers

746f5d9

zbaylin requested a review from bryphe August 25, 2020 00:31

zbaylin added the WIP label Aug 25, 2020