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

How to support nanobind? #3

Open
davidlatwe opened this issue Feb 20, 2024 · 9 comments
Open

How to support nanobind? #3

davidlatwe opened this issue Feb 20, 2024 · 9 comments

Comments

@davidlatwe
Copy link
Contributor

Hi @pthom ,

If I want to integrate litgen with nanobind, make it able to chose the flavor of binding, what would you suggest?

There are lots of small differences between the two, like:

  • Naming changed: .def_property -> .def_prop_rw
  • Interface changed: obj.cast<bool>() -> py::cast<bool>(obj)
  • ...

Although they can be done with str.replace(), regex and some hardwork, but if litgen can add one new flavor that would be awesome.

Thanks. :)

@pthom
Copy link
Owner

pthom commented Feb 20, 2024

Hello,

If you want to do this, here are some advices:

litgen is not a small library, so I think you may encounter a few subtleties, such as how to make methods overridable, which is very much linked to pybind11 at the moment.

If you take this route, please keep me informed of your progress!

thanks

@pthom
Copy link
Owner

pthom commented Mar 27, 2024

Hello, could you give me more information about your current status? Did you manage to get something working? If so, I would be interested in having more information. Thanks.

@davidlatwe
Copy link
Contributor Author

davidlatwe commented Mar 27, 2024

Hey @pthom, sorry for being late!

We did manage to get our nanobind flavor imgui binding, here's what I did:

About this one particular commit davidlatwe@19b4826 in my litgen fork, it was for fixing this:

    # imgui.CalcTextSize("Test")
    ^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: CalcTextSize(): incompatible function arguments. The following argument types are supported:
    1. CalcTextSize(text: str, text_end: Optional[str] = None, hide_text_after_double_hash: bool = False, wrap_width: float = -1.0) -> imgui.ImVec2

Somehow, py::arg("text_end") = py::none() is not enough for nanobind to work with NULL as default:

ImVec2 ImGui::CalcTextSize(const char* text, const char* text_end = NULL, ...)

It has to be like this:

m.def("CalcTextSize", [](const char * text, std::optional<std::string> text_end = std::nullopt, ...)
{
    auto CalcTextSize_adapt = [](const char * text, std::optional<std::string> text_end = std::nullopt, ...)
    {
        const char * text_end_adapt_default_null = nullptr;
        if (text_end.has_value())
            text_end_adapt_default_null = text_end.value().c_str();

        auto lambda_result = ImGui::CalcTextSize(text, text_end_adapt_default_null, ...);
        return lambda_result;
    };

    return CalcTextSize_adapt(text, text_end, ...);
},
py::arg("text"), py::arg("text_end") = py::none(), ...);

I am sure there are still some bugs in our imgui nanobind binding, but it is working great so far for our goals. But util now though, because I found that I have to also try supporting older versions of Python which are not supported by nanobind, so I am going back to pybind11 at the moment. (Our library runs in another application which has its own Python embedded from 2.7-3.11+ depends on its version. And we have Python 3.11+ embedded in our library and ImGui will be bound with it for internal GUI drawing. We'd like to use only one binding library through out entire project. So although ImGui is aimed to Python 3.11+, our other bindings are aimed to the host application for public use. Therefore we now have to go with pybind11 v2.9.2 to see if that works.)

Oh and, about this one:

you should add an option bind_library + enum BindLibraryType inside options.py

I have not done that yet 😅

I installed my litgen fork in a venv with pip install -e, and here's the script that we used to run the generator:

import os
import shutil
# ./external/imgui_bundle/external/imgui/bindings/generate_imgui.py
import generate_imgui

ROOT = os.path.dirname(__file__)


@generate_imgui.my_time_it
def generate():
    # Note: Don't need imgui_test_engine, so not using `generate_imgui.main()`
    # Note: Don't need, but also cannot bind imgui_internal because `GImGui`
    #   symbol is unresolvable by linker.
    generate_imgui.autogenerate_imgui()

    stub_dir = generate_imgui.STUB_DIR
    pydef_dir = generate_imgui.PYDEF_DIR
    wrapper_dir = os.path.join(os.path.dirname(pydef_dir), "imgui_pywrappers")
    files = [
        # cpp
        os.path.join(pydef_dir, "nanobind_imgui.cpp"),
        # pyi
        os.path.join(stub_dir, "imgui/__init__.pyi"),
        # wrapper
        os.path.join(wrapper_dir, "imgui_pywrappers.cpp"),
        os.path.join(wrapper_dir, "imgui_pywrappers.h"),
    ]

    return files


def to_nanobind(file):
    with open(file, "r") as fp:
        lines = fp.readlines()

    in_text_buffer_class = False
    in_text_buffer_delete = False

    new_lines = []
    for line in lines:
        # error LNK2001: unresolved external symbol "public:
        #   static char * ImGuiTextBuffer::EmptyString"
        if "auto pyClassImGuiTextBuffer =" in line:
            in_text_buffer_class = True
        if in_text_buffer_class:
            if '.def("begin",' in line or '.def("c_str",' in line:
                in_text_buffer_delete = True
            if '.def("size",' in line:
                in_text_buffer_delete = False
            if '.def("append",' in line:
                in_text_buffer_delete = False
                in_text_buffer_class = False
            if in_text_buffer_delete:
                continue

        line = line.replace(" = NULL", " = nullptr")
        new_lines.append(line)

    with open(file, "w") as fp:
        fp.writelines(new_lines)


def run():
    # Generate binding
    files = generate()

    # Copy generated file
    out_dir = os.path.join(ROOT, "src")
    cpp_files = []
    for src in files:
        fname = os.path.basename(src)
        if fname.endswith(".cpp") or fname.endswith(".h"):
            cpp_files.append(fname)
        shutil.copy2(src, os.path.join(out_dir, fname))

    # Final nanobind related cleanup
    for fname in cpp_files:
        to_nanobind(os.path.join(out_dir, fname))


if __name__ == "__main__":
    run()

@pthom
Copy link
Owner

pthom commented Mar 27, 2024

Hello,

Thanks for the details info!

You went far into the customization, since I see you added a specific adapter (which is deep inside litgen internals).
Well done! Did you find it easy to find your way in the code?

Note: your to_nanobind(file) function could potentially be a postprocess

you should add an option bind_library + enum BindLibraryType inside options.py

I have not done that yet 😅

There is no rush. Adding this to litgen would require additional effort (integration tests, polishing, etc.); all of which is time consuming, I know. If you have time to work on it later, I would appreciate it; but it depends on whether you have time to invest on it or not.

And we have Python 3.11+ embedded in our library and ImGui will be bound with it for internal GUI drawing

May I ask what kind of project you are working on?

@davidlatwe
Copy link
Contributor Author

😊

Well done! Did you find it easy to find your way in the code?

Thanks! Yeah, it was easy enough to get my way. With some simple text search, I can learn where I need to go. And the variable/function naming is quite clear.

This is us 👋🏼 : https://ragdolldynamics.com/

We use ImGui to draw our user interface on the 3D viewport in another application like Maya/Blender. You can find the trace of ImGui in here: https://learn.ragdolldynamics.com/tutorials/manikin/#shapes

At the moment, our user interface was written in C++, but in the futrue we may enable our clients the ability to make their own UI with our embedded Python and ImGui Python binding. To do so, there can have only one ImGuiContext instance, which means we need to build our own Python binding and link to our shared library for accessing ImGui.

By the way, here is one interesting thing I found. When I build the binding with nanobind from our original ImGui source code, the build works without any problem. But when I switch back to pybind11, I got error like error C2027: use of undefined type 'ImGuiDockNodeSettings'. Then I realized imgui_bundle has this tweak for it to build. Somehow nanobind does not need such change.

@mattparks
Copy link

It has been a few months since there has been activity on this issue. Is there a path forward for litgen to support automatic nanobind code generation? If that's a feature that can be supported, I'd love to use this tool in one of my projects.

Thanks!

@davidlatwe
Copy link
Contributor Author

Hey @mattparks , nanobind support currently only available from my forks:

And I only did the necessary changes for my own project so not fully tested. If possible, you can give my forks a shot. In the mean time, I will try submit a PR.

@pthom
Copy link
Owner

pthom commented Oct 3, 2024

Hi @davidlatwe

I had a deep look at your branches for litgen, and imgui_bundle, and it is indeed a very thorough and nice work. Congrats!

I hope someday it will be possible to merge it.

Feel free to open a PR, even if not complete. It might remain as a separate branch in a first step.
Before merging to main, I think we would need to adapt at least some of the unit tests (code generation) and some of the integration tests (bindings tests) inside litgen.

@davidlatwe
Copy link
Contributor Author

Thanks for asking! Pull requests created 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants