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

Incorrect data dimensions lead to somewhat misleading error message #498

Closed paulmelis closed 3 years ago

paulmelis commented 3 years ago

While updating some code from 2.3.0 to 2.7.0 I noticed I was apparently incorrectly passing vertex data to Mesh's vertex.position: the dimensions set on the data array included the vector dimension itself, e.g. for 5 vertices I would pass {5, 3, 1}. This leads to the error (ospray::Mesh must have 'vertex.position' array with element type vec3f), even though the correct element type was set. So it took me some time to figure out what the actual issue was as the error reported isn't accurate (and the dimensions don't seem to be checked in this particular case).

Here's an example:

#include <ospray/ospray_cpp.h>

static void
error_func(void* /*userdata*/, OSPError error, const char *details)
{
    printf("OSPRAY ERROR: %d (%s)\n", error, details);
}

static void
status_func(void* /*userdata*/, const char *message)
{
    printf("OSPRAY STATUS: %s\n", message);    
}

struct vec3ul { 
    uint64_t x, y, z; 

    vec3ul(): x(0), y(0), z(0) {}
    vec3ul(uint64_t xx, uint64_t yy, uint64_t zz): x(xx), y(yy), z(zz) {}
};

namespace ospray {
OSPTYPEFOR_SPECIALIZATION(vec3ul, OSP_VEC3UL);
}

ospray::cpp::CopiedData
dup(ospray::cpp::CopiedData& d)
{
    return d;
}

int main(int argc, const char **argv)
{
    ospInit(&argc, argv);

    ospray::cpp::Device device(ospGetCurrentDevice());
    ospDeviceSetErrorCallback(device.handle(), error_func, nullptr);
    ospDeviceSetStatusCallback(device.handle(), status_func, nullptr);

    vec3ul num_vertices { 4, 3, 1 };
    vec3ul byte_stride { 0, 0, 0 };

    float vertex[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12 };

    ospray::cpp::CopiedData v(vertex, OSP_VEC3F, num_vertices, byte_stride);

    ospray::cpp::Geometry mesh("mesh");
    mesh.setParam("vertex.position", v);
    mesh.commit();
}
melis@juggle 21:38:~/concepts/ospray-python$ g++ -I ~/software/ospray-2.7.0.x86_64.linux/include/ t_data.cpp  -L ~/software/ospray-2.7.0.x86_64.linux/lib/ -lospray -ot_data
melis@juggle 21:41:~/concepts/ospray-python$ ./t_data 
OSPRAY ERROR: 1 (ospray::Mesh must have 'vertex.position' array with element type vec3f)

If I change num_vertices to be {4, 1, 1} the error goes away.

johguenther commented 3 years ago

Thanks for improving OSPRay's warning messages, loosely related to #396. Actually the dimensions are checked, leading to the error message: A one-dimensional array for vertex.position is expected, whereas you passed a 2D array of size 4×3 of type vec3f (note the element type is the vector, not float). However, this is not clear from the message. How about

ospray::Mesh must have 'vertex.position' 1D array with element type vec3f
paulmelis commented 3 years ago

Yes, that would be an improvement. What would be even better in cases where a check on data type/size has failed is to also (if that's feasible) report what was erroneously passed, to provide even more information. E.g. ospray::Mesh must have 'vertex.position' 1D array with element type vec3f, got 2D array of vec3f instead.