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

Python binding issues #1004

Open
6 of 13 tasks
Meakk opened this issue Sep 29, 2023 · 12 comments
Open
6 of 13 tasks

Python binding issues #1004

Meakk opened this issue Sep 29, 2023 · 12 comments
Labels
help wanted Please help with this issue! source:python

Comments

@Meakk
Copy link
Member

Meakk commented Sep 29, 2023

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

To Reproduce

  • Engine deletion

  • del engine

  • not working (expecting the window to close)

  • Overwriting engine

  • e=f3d.Engine()

  • close window

  • e=f3d.Engine(f3d.Window.NATIVE_OFFSCREEN)

  • Python crashes

  • Closing the window manually

  • e=f3d.Engine()

  • close window

  • e.window.render()

  • Crash

  • Blocking interactor

  • e=f3d.Engine()

  • e.interactor.start()

  • Blocking Python prompt, should we? (yes)

  • Crashing interactor

  • e=f3d.Engine()

  • e.interactor.start()

  • close window manually (not using escape)

  • crashes

  • No filename when pressing N

  • e=f3d.Engine()

  • e=f3d.loader.load_geometry("/valid/file.obj")

  • e.interactor.start()

  • press N
    => no filename (pressing H shows that filename is enabled)

Expected, set ui.filename_info first.

@Meakk Meakk added this to the 2.2.0 milestone Sep 29, 2023
@mwestphal
Copy link
Contributor

partially addressed by #1005

@mwestphal
Copy link
Contributor

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
Copy link
Contributor

mwestphal commented Sep 30, 2023

Blocking interactor

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

@mwestphal
Copy link
Contributor

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
Copy link
Contributor

No filename when pressing N

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

@mwestphal
Copy link
Contributor

Engine deletion

Reproduced

@mwestphal mwestphal modified the milestones: 2.2.0, 2.3.0 Sep 30, 2023
@mwestphal mwestphal modified the milestones: 2.3.0, 2.4.0 Dec 21, 2023
@mwestphal mwestphal added the help wanted Please help with this issue! label Jan 7, 2024
@yigitako
Copy link

I can work on that issue.

@mwestphal
Copy link
Contributor

Great, let me know if you need any help

@yigitako
Copy link

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
Copy link
Contributor

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

@Meakk
Copy link
Member Author

Meakk commented Jan 26, 2024

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_ptrf3d::engine 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

@Meakk Meakk added this to F3D Feb 2, 2024
@mwestphal mwestphal removed this from the 2.4.0 milestone Feb 3, 2024
@mwestphal mwestphal moved this to Investigate in F3D Feb 10, 2024
@mwestphal mwestphal moved this from Investigate to To do in F3D Feb 10, 2024
@mwestphal
Copy link
Contributor

I've tested and checked many of the items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Please help with this issue! source:python
Projects
Status: To do
Development

No branches or pull requests

3 participants