search

Nelarius / imnodes

A small, dependency-free node editor for dear imgui
MIT License
2.01k stars 244 forks source link

Added functions to set the current context #89

Closed mathisloge closed 3 years ago

mathisloge commented 3 years ago

closes #88

mathisloge commented 3 years ago

I think this pr is actually a chance to make imnodes behave more like ImGui and other well-known extensions such as ImPlot. We could replace Initialize() and Shutdown() with CreateContext() and DestroyContext(), with the same semantics as the other libraries:

  • CreateContext returns the created context, and sets the global context if it is NULL.
  • DestroyContext cleans up the context passed to it, or if left NULL, cleans up and sets the global context to NULL.

What do you think? I left some comments regarding this suggestion.

This would be a breaking change, and would require updating the README and examples as well. But if you would be willing to make the code changes, I can take those and update the README.

I will do the code changes and look in the imgui source code how it is done there. Time estimation is currently next week.

mathisloge commented 3 years ago

we should also add some dllexport/import macros. usually i'm using the auto generated export headers by cmake https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html but we can also add some manually, since this project uses premake, which i don't know and use. But i could also provide a cmake file with proper install commands to directly use the library in other projects with find_package(imnodes CONFIG [...]) [...] target_link_library(myexe .. imnodes::imnodes) and add this lib to vcpkg

Your decision :)

Nelarius commented 3 years ago

Nice work @mathisloge 👌 I had just one small comment left, then I'll merge.

Regarding the export/import macros: seems like a good idea to add those to finish up the dll support. I suppose adding something like IMNODES_API macro to all the function declarations would remove the need to use the export header? Would you open a separate pr for that? 🙂

Regarding the cmake file -- I would actually prefer not to add it, since I was thinking about also removing the premake file entirely. I didn't really intend to offer build support as a part of this repository, and I'm not a cmake user. The premake file is there mainly for my own convenience 😅 I think this shouldn't prevent adding this project to vcpkg, since the cmake build can be added alongside the portfile as far as I know 🤔

mathisloge commented 3 years ago

Sounds good to me. Will open a new PR for the macro definition

you are correct. vcpkg works without a native cmake build script.