f3d-app / f3d

Fast and minimalist 3D viewer.
https://f3d.app
BSD 3-Clause "New" or "Revised" License
2.59k stars 184 forks source link

Python binding issues #1004

Open Meakk opened 9 months ago

Meakk commented 9 months ago

Describe the bug Some Python features are broken. Let's fix them or remove the bindings before the release

To Reproduce

mwestphal commented 9 months ago

partially addressed by #1005

mwestphal commented 9 months ago

Overwriting engine / Closing the window manually

Unable to "really" reproduce, the window cannot be closed properly unless the interactor is running. My window manager doesn't let me close the window (Awesome, ArchLinux). Which WM are you using ?

I think it could be considered some kind of force closing. I'm not sure how we can handle it more cleanly but no F3D code is running when you close that window.

mwestphal commented 9 months ago

Blocking interactor

Reproduced. We need to use threads to fix that. I think we definitely want it.

mwestphal commented 9 months ago

Crashing interactor

Unable to reproduce, my window manager let me close the window without issue and python is not crashing.

I think your window manager must be quite brutal ^^

mwestphal commented 9 months ago

No filename when pressing N

Yes, and it makes complete sense. Should be adressed in the context of #443

mwestphal commented 9 months ago

Engine deletion

Reproduced

yigitako commented 6 months ago

I can work on that issue.

mwestphal commented 6 months ago

Great, let me know if you need any help

yigitako commented 6 months ago

Engine Deletion Code Used To test

import f3d e = f3d.Engine() del e

The del statement removes the reference to the object, potentially making it eligible for garbage collection, but it does not mean that the window should be close.

As for my understanding, I may be wrong. Engine holds a reference to itself, so even when we remove our reference to it (the variable “e”) the reference count never falls to 0 because it still has its own internal reference(Python Developer’s Guide, n.d.) .

Additionally, I have tried to delete a tkinter, a python gui tool, window with the del statement and it didn't closed the window. Code Used to at tkinter

import tkinter root = tkinter.Tk() del root

I believe we should define an "Engine" attribute specifically dedicated to the task of closing windows and performing any necessary clean-ups.

Reference: Python Developer’s Guide. (n.d.). Garbage collector design. [online] Available at: https://devguide.python.org/internals/garbage-collector/index.html [Accessed 17 Jan. 2024].

mwestphal commented 5 months ago

@Meakk @snoyer I think you investigated a bit this, please follow up with your conclusions.

Meakk commented 5 months ago

For the engine deletion, here's my conclusion:

If not, we need to try implementing enter and exit in the bindings. In the post above, there's another answer with more details (https://stackoverflow.com/a/74656071).

Specifically there's a comment:

You might need to put your listener class in a wrapper class.

I think that might be the solution. So the Engine class in python will bind a new C++ class f3d::engine_wrapper which contains a std::unique_ptr we reset to nullptr in the wrapper destructor AND the exit binding.

And an untested sample:

class engine_wrapper
{
public:
  engine_wrapper():mEngine(new engine()) {}

  py::object enter() { return py::cast(mEngine.get()); }
  void exit(py::handle, py::handle, py::handle) { mEngine = nullptr; }

  // forward all functions
  f3d::options& getOptions() { return mEngine.getOptions(); }
  // ...

private:
  std::unique_ptr<f3d::engine> mEngine;
}

py::class_<f3d::engine_wrapper> engine(module, "Engine"); // note usage of engine_wrapper instead of directly the engine