RenderKit / ospray

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

Incorrect warning for parameters with multiple types #396

Closed paulmelis closed 4 years ago

paulmelis commented 4 years ago

You're probably aware of this but for parameters that accept multiple types, like vec3ui and vec4ui for Geometry.index, a status message is shown due to the parameter value being retrieved in one of the types while the other type is passed. E.g. when passing a vec4ui array:

OSPRAY STATUS: ospray::Mesh ignoring 'index' array with wrong element type (should be vec3ui)

Order matters as well, if I pass a vec3ui array the message isn't shown, as that type is tested first.

jeffamstutz commented 4 years ago

We only emit these as OSP_LOG_DEBUG messages, so they technically are not warnings. :)

The only way to get them is to have the most verbose log level enabled. For example, when switching to the cornell_box scene in ospExamples, by default you get no message emitted. However, overriding this will get you the output:

With app-set default of "warning" log level:

./ospExamples

And with a hard environment override to "debug" log level:

OSPRAY_LOG_LEVEL=debug ./ospExamples
...
ospray::Mesh ignoring 'index' array with wrong element type (should be vec3ui)
...

Thus I think we will keep the status quo, as these are useful messages when debugging.

paulmelis commented 4 years ago

Fair enough about these not being true warnings. However, taken at face value the message is misleading, as the array is not ignored, nor is the element type wrong. Plus the "should be ..." is just not true, but one needs to be aware that there's two possible types for the parameter.

Funny you mention that "these are useful messages when debugging". I'm not suggesting these messages should go, but the reason I made an issue for this is that precisely while debugging my own app this message made me search for a problem in the wrong place. It suggests that there's something going on, while there isn't.

jeffamstutz commented 4 years ago

I hear you on the message being misleading. Would rewording the message be helpful? What’s really happening is it’s “checking for array of type X and didn’t find one. It is either optional or has alternatives”.

paulmelis commented 4 years ago

Well, the message in the current form is correct for parameters that have only one possible type. In that case it's indeed the thing you want to know.

But parameters with multiple types would then need to change to your suggestion. However, I think you're testing parameters in isolation right now (i.e. without knowledge of alternative types)?

paulmelis commented 2 years ago

Hmmm, just ran into this issue again 1.5 years later :smile: The message is still quite misleading in the way it is worded

johguenther commented 2 years ago

Tricky. Indeed, the check (and message) happens in isolation, the code has no context that an alternative type of the parameter would be accepted. We need to check at a higher level.

paulmelis commented 2 years ago

I find the should be ... part to be the most troublesome, as it puts me on the wrong foot. Perhaps rewording that to checked for ..., to correctly say that only a check for a single type was done (while leaving open the possibility for another type).

johguenther commented 2 years ago

6df5ab4cb5836c1b08b85c1f5443c6e5d84bfd82 also improves this error message.