RenderKit / ospray

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

OSPRay 2.x ospSetParam(), ospSetXxx() don't emit error mesg or callback for mismatched parameter type #428

Closed tachyon-john closed 4 years ago

tachyon-john commented 4 years ago

This is similar to issue #427 except that as far as I have been able to determine, the documentation doesn't actually specify what should occur if one calls ospSetParam() or ospSetXxx() with the correct object parameter ID string, but with an incorrect data type. No error messages or error callbacks seem to get triggered in this case either, at least in my testing.

iqch commented 4 years ago
    device = ospGetCurrentDevice();

    ospDeviceSetStatusFunc(device, HandleOSPRayStatusMsg);
    ospDeviceSetErrorFunc(device, HandleOSPRayError);

    ospDeviceCommit(device);

.... // CALLBACKS

void HandleOSPRayStatusMsg(const char *messageText) { cout << "HDOSPRAY STMSG : " << messageText << endl; };

void HandleOSPRayError(OSPError e, const char *errorDetails) { cout << "HDOSPRAY ERROR : " << e << " :: " << errorDetails << endl;

TF_CODING_ERROR("HDOSPRAY ERROR : %s", errorDetails);

};

works for me as expected, sometime even more annoying, than it should be

Win10x64/2.2.1

tachyon-john commented 4 years ago

I think you misunderstood my issue. I have no difficulty with setting the error callback, or getting error output from other scenarios, but the specific cases I have listed (mismatched parameter type) doesn't appear to cause an error message to be generated. There's nothing wrong with the error callback mechanism itself...

paulmelis commented 4 years ago

You can try testing with different values for --osp:log-level (or the equivalent API parameter). Setting it to warning (or lower) should show the warnings (but not errors).

Edit: so the main point is that these parameter errors are actually treated as warnings by OSPRay

iqch commented 4 years ago

I get error when set OSP_VEC4I instead of OSP_VEC4UI, error rises after commiting wrong setup object, not right after setting wrong typed parameter 2020-06-05_10-00-05

tachyon-john commented 4 years ago

I've gotten the mismatched parameter type in some specific cases but not others, so perhaps the issue is that not all of the parameter types are being checked equally well by the different types of objects? I tested various bogus cases on the OSPRenderer object without generating errors at commit time. I will turn on the status message callbacks to make sure I'm not missing these, but I still would have expected errors to show up on the error callback for the cases I encountered.

tachyon-john commented 4 years ago

So, more interesting stuff. If I set OSPRAY_LOG_LEVEL to "debug" before launching the app, I can get output on the status callback. However, if I use the following code to set the log level after registering the error and status callbacks, I get no status output? Anything obviously wrong with setting the logLevel value below per the docs?: if (verbose == RT_VERB_DEBUG) printf("OSPRay2Renderer) setting error / status callbacks...\n"); OSPDevice dev = ospGetCurrentDevice(); ospDeviceSetErrorFunc(dev, vmd_ospray2_error_callback); ospDeviceSetStatusFunc(dev, vmd_ospray2_status_callback); ospDeviceSetParam(dev, "logLevel", OSP_STRING, "debug"); ospDeviceCommit(dev);

I still feel that bogus or mismatched parameters and types should be treated as errors and therefore trigger the error output callback, and not as "status" information, but I see that's a choice that the OSPRay devs have made. If it is going to remain this way, I would strongly recommend documenting this such that it can't be missed, by pointing out this behavior as part of the discussion of ospSetParam(), etc.

paulmelis commented 4 years ago

So, more interesting stuff. If I set OSPRAY_LOG_LEVEL to "debug" before launching the app, I can get output on the status callback. However, if I use the following code to set the log level after registering the error and status callbacks, I get no status output? Anything obviously wrong with setting the logLevel value below per the docs?: if (verbose == RT_VERB_DEBUG) printf("OSPRay2Renderer) setting error / status callbacks...\n"); OSPDevice dev = ospGetCurrentDevice(); ospDeviceSetErrorFunc(dev, vmd_ospray2_error_callback); ospDeviceSetStatusFunc(dev, vmd_ospray2_status_callback); ospDeviceSetParam(dev, "logLevel", OSP_STRING, "debug"); ospDeviceCommit(dev);

Hmm, I'm seeing the same thing (no output) when setting the value through the API. But using --osp:log-level=debug works though.

I still feel that bogus or mismatched parameters and types should be treated as errors and therefore trigger the error output callback, and not as "status" information, but I see that's a choice that the OSPRay devs have made. If it is going to remain this way, I would strongly recommend documenting this such that it can't be missed, by pointing out this behavior as part of the discussion of ospSetParam(), etc.

Well, there is warnAsError which you could set, although that doesn't address your point.

tachyon-john commented 4 years ago

Thanks for validating that you don't get correct logging behavior via the API either. It occurs to me that there may be a catch-22 here: If one uses the wrong string for "logLevel" it'll just silently fail to set it, so then you don't get any of those warnings. Kind of funny. Maybe the docs have a typo there? I'm going to call it a night for now.

paulmelis commented 4 years ago

Ah, seems the docs don't match the code. Docs say that logLevel is of type string, but the code actually assumes an int:

https://github.com/ospray/ospray/blob/2a97f95e1726fdbf65e75ef4cc7e50d29b2b5425/ospray/api/Device.cpp#L85

paulmelis commented 4 years ago

Yup, OSP_LOG_DEBUG is 1, using that value triggers the handlers

paulmelis commented 4 years ago

Echoing @tachyon-john above, I feel more things that are currently flagged as warnings should be reported as errors. For example, my typo ospray::cpp::Geometry geom("curves") will not work due to the incorrect geometry type but it is only reported as a warning ("WARNING: unrecognized geometry type 'curves'") .But any subsequent calls on the geom object will result in errors due to the underlying handle being null. Failing object construction should really be a severe type of error. Plus I can imagine applications building on ospray setting a default log level of error to make those always visible.