greenfork / nimraylib_now

The Ultimate Raylib gaming library wrapper for Nim
MIT License
151 stars 17 forks source link

Destructors #72

Closed greenfork closed 1 year ago

greenfork commented 2 years ago

Currently crashes on exit. If I remove closeWindow(), doesn't crash. Probably because there's a call to unloadTexture after the window was closed and that causes the crash.

$ cd examples/textures/
$ nim r --gc:orc textures_to_image.nim
Hint: used config file '/home/grfork/.choosenim/toolchains/nim-1.6.2/config/nim.cfg' [Conf]
Hint: used config file '/home/grfork/.choosenim/toolchains/nim-1.6.2/config/config.nims' [Conf]
..........................................................................................
/home/grfork/reps/nimraylib_now/src/nimraylib_now/static_build.nim(4, 1) Hint: duplicate import of 'os'; previous import here: /home/grfork/reps/nimraylib_now/src/nimraylib_now/mangled/raylib.nim(6, 6) [DuplicateModuleImport]
.........
CC: stdlib_dollars.nim
CC: ../../src/nimraylib_now/mangled/raylib.nim
Hint:  [Link]
Hint: gc: orc; opt: none (DEBUG BUILD, `-d:release` generates faster code)
53442 lines; 0.713s; 75.309MiB peakmem; proj: /home/grfork/reps/nimraylib_now/examples/textures/textures_to_image.nim; out: /home/grfork/.cache/nim/textures_to_image_d/textures_to_image_5FC9DD2C66B4B8B494B4BD9E4BF3F178F0C7F7DD [SuccessX]
Hint: /home/grfork/.cache/nim/textures_to_image_d/textures_to_image_5FC9DD2C66B4B8B494B4BD9E4BF3F178F0C7F7DD  [Exec]
INFO: Initializing raylib 4.0
INFO: DISPLAY: Device initialized successfully
INFO:     > Display size: 1920 x 1080
INFO:     > Screen size:  800 x 450
INFO:     > Render size:  800 x 450
INFO:     > Viewport offsets: 0, 0
INFO: GLAD: OpenGL extensions loaded successfully
INFO: GL: Supported extensions count: 229
INFO: GL: OpenGL device information:
INFO:     > Vendor:   AMD
INFO:     > Renderer: AMD RENOIR (DRM 3.44.0, 5.16.5-arch1-1, LLVM 13.0.0)
INFO:     > Version:  4.6 (Core Profile) Mesa 21.3.5
INFO:     > GLSL:     4.60
INFO: GL: DXT compressed textures supported
INFO: GL: ETC2/EAC compressed textures supported
INFO: TEXTURE: [ID 1] Texture loaded successfully (1x1 | R8G8B8A8 | 1 mipmaps)
INFO: TEXTURE: [ID 1] Default texture loaded successfully
INFO: SHADER: [ID 1] Vertex shader compiled successfully
INFO: SHADER: [ID 2] Fragment shader compiled successfully
INFO: SHADER: [ID 3] Program shader loaded successfully
INFO: SHADER: [ID 3] Default shader loaded successfully
INFO: RLGL: Render batch vertex buffers loaded successfully in RAM (CPU)
INFO: RLGL: Render batch vertex buffers loaded successfully in VRAM (GPU)
INFO: RLGL: Default OpenGL state initialized successfully
INFO: TEXTURE: [ID 2] Texture loaded successfully (128x128 | GRAY_ALPHA | 1 mipmaps)
INFO: FONT: Default font loaded successfully (224 glyphs)
INFO: FILEIO: [resources/raylib_logo.png] File loaded successfully
INFO: IMAGE: Data loaded successfully (256x256 | R8G8B8 | 1 mipmaps)
INFO: TEXTURE: [ID 3] Texture loaded successfully (256x256 | R8G8B8 | 1 mipmaps)
INFO: TEXTURE: [ID 3] Pixel data retrieved successfully
INFO: TEXTURE: [ID 4] Texture loaded successfully (256x256 | R8G8B8 | 1 mipmaps)
INFO: TEXTURE: [ID 3] Unloaded texture data from VRAM (GPU)
INFO: TEXTURE: [ID 2] Unloaded texture data from VRAM (GPU)
INFO: SHADER: [ID 3] Default shader unloaded successfully
INFO: TEXTURE: [ID 1] Default texture unloaded successfully
INFO: Window closed successfully
Traceback (most recent call last)
/home/grfork/reps/nimraylib_now/src/nimraylib_now/mangled/raylib.nim(3103) textures_to_image
/home/grfork/reps/nimraylib_now/src/nimraylib_now/mangled/raylib.nim(3104) =destroy
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
Error: execution of an external program failed: '/home/grfork/.cache/nim/textures_to_image_d/textures_to_image_5FC9DD2C66B4B8B494B4BD9E4BF3F178F0C7F7DD '
planetis-m commented 2 years ago

Yes exactly, so either put closeWindow in a try/finally or defer or let the user encapsulate in a destructor backed object.

greenfork commented 2 years ago

Yes exactly, so either put closeWindow in a try/finally or defer

On top-level defers are forbidden. It might be an okay idea to wrap every example in main.

or let the user encapsulate in a destructor backed object.

I don't think it is a nice idea to say that to the user. The users of this library might be of beginner level.

Currently another problem is that I'm not sure that we have a complete list of all possible destructors. We can't say to the user "here is a list of types you need to memorize, you don't need to unload them; here is another list which you should unload manually".

unloadVrStereoConfig I removed it, its a noop for now.

That is fine, we can add it later once it has implementation.

unloadWaveSamples just a for loop that calls unloadWaveSample, since we used seq[Wave] it was not need. unloadImageColors, unloadFontData, unloadImagePalette I think same as above

  1. It's not entirely clear that we want a seq type here.
  2. It's not obvious that Nim would manage the memory that was initialized outside of Nim. We can check it of course but I'm afraid of corner cases.

unloadFileData, unloadFileText it's hard to tell what to do with these, could we instead use std equivalents instead of LoadFile*

It is okay to use the std variant but we probably still want a destructor for it so that --gc:orc is going to manage the memory in this case.

unloadCodepoints haven't decided what to do with codepoints yet.

This can wait for sure.

planetis-m commented 2 years ago

Currently another problem is that I'm not sure that we have a complete list of all possible destructors.

There is one I left out... ClearDroppedFiles and I don't think anyone has yet.