search

pthom / hello_imgui

Hello, Dear ImGui: unleash your creativity in app development and prototyping
https://pthom.github.io/hello_imgui
MIT License
604 stars 91 forks source link

RunnerParams backendType compatibility is broken #95

Closed learn-more closed 4 months ago

learn-more commented 4 months ago

The field backendType is mapped to platformBackendType with a reference in the struct RunnerParams, but this is problematic as soon as the struct is copied. (It will still point to the old instance).

This is visible in debug builds of MSVC in the samples that use the void Run(const SimpleRunnerParams& simpleRunnerParams) function (could be indirect), the one I encountered it in is hello_globe. When starting this in debug, you will hit an assertion that mentions failed to factor a runner, because inside FactorRunner the backendType is 0xcccccccc (The old object is on the stack and happens to be overwritten in a debug build accidentally). (Another thing is that FactorRunner should probably not use backendType if that is marked as obsolete).

Here is a godbolt demonstrating this issue in a simplified form: https://godbolt.org/z/chfs49vxq

My suggestion would be to replace backendType with an std::optional<BackendType> backendType;, and update FactorRunner to use platformBackendType if backendType is not set.

If you want, I could create a PR for this change?

Additionally, it might make sense to add a ci workflow with the define HELLOIMGUI_DISABLE_OBSOLETE_BACKEND set.

pthom commented 4 months ago

Hello, Nice catch, thanks! Using a reference as a synonym was indeed questionable.

Using an optional might be a solution. However, I think it would be better to remove the legacy synonyms. Not many users are actually using this, and adapting to the new API is straightforward (and I did not make a guarantee of API stability, although I try to keep it stable as much as possible).

If you are willing to push a PR that removes those synonyms, please do :-)