RenderKit / ospray

An Open, Scalable, Portable, Ray Tracing Based Rendering Engine for High-Fidelity Visualization
http://ospray.org
Apache License 2.0
1.01k stars 182 forks source link

Suggest revision of OSPRay 2.x error and status callback function signatures to include a app-provided handle/pointer for use by callback function. #430

Closed tachyon-john closed 4 years ago

tachyon-john commented 4 years ago

The OSPRay error and status callback function prototypes don't currently facilitate an application-provided handle or pointer parameter to be set by OSPRay when the callbacks are used. This means that if the callback needs to funnel output into a GUI, some C++ class instance, something owned by a different thread, or other outcome other than just stdout/stderr, the callback would end up doing icky things with global variables. I would suggest that OSPRay follow the more typical convention of allowing the callback function to receive an application-provided pointer or handle that it can use to do what it needs to do, without resorting to global variables or other ugly coding approaches that are best avoided.

For the error callback that would be something akin to: typedef void (OSPErrorFunc)(void appctxhandle, OSPError, const char errorDetails); The application-provided handle would be a new parameter provided in the matching set call: void ospDeviceSetErrorFunc(OSPDevice, OSPErrorFunc, void appctxhandle);

I would make the same changes for the status message callback, and indeed ALL callback functions and their corresponding registration APIs that OSPRay uses, for any purpose.

johguenther commented 4 years ago

Again, good point. It's currently beyond me why we forgot to have the user pointers also for the error and status callbacks: the "progress and cancel" callback in v1.8 had it.

tachyon-john commented 4 years ago

@johguenther "Stuff happens"... I've made the same mistake myself, probably also more than once. Having made it and suffered consequences, it helps me notice this issue and remind others ;-)