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

Cannot use ospray_cpp in 2.1 binaries due to missing headers #416

Closed paulmelis closed 4 years ago

paulmelis commented 4 years ago
melis@juggle 19:18:~/concepts/blender-ospray-engine$ cat t.cc 
#include <ospray/ospray.h>
#include <ospray/ospray_testing/ospray_testing.h>

melis@juggle 19:18:~/concepts/blender-ospray-engine$ g++ -c -I ~/software/ospray-2.1.0.x86_64.linux/include/ t.cc
In file included from /home/melis/software/ospray-2.1.0.x86_64.linux/include/ospray/ospray_cpp/Camera.h:6,
                 from /home/melis/software/ospray-2.1.0.x86_64.linux/include/ospray/ospray_cpp.h:8,
                 from /home/melis/software/ospray-2.1.0.x86_64.linux/include/ospray/ospray_testing/ospray_testing.h:6,
                 from t.cc:2:
/home/melis/software/ospray-2.1.0.x86_64.linux/include/ospray/ospray_cpp/ManagedObject.h:12:10: fatal error: ospcommon/math/AffineSpace.h: No such file or directory
   12 | #include "ospcommon/math/AffineSpace.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

melis@juggle 19:19:~/concepts/blender-ospray-engine$ find ~/software/ospray-2.1.0.x86_64.linux/include/ -name AffineSpace.h
melis@juggle 19:19:~/concepts/blender-ospray-engine$ 
johguenther commented 4 years ago

It looks like you do not have (the headers of) https://github.com/ospray/ospcommon available?

paulmelis commented 4 years ago

But there's no separate binary distribution for ospcommon is there? The point is that the ospray binary distribution is incomplete as it doesn't contain those headers, but does contain headers that reference ospcommon.

johguenther commented 4 years ago

partly related: #411

Main issue is that one cannot use the C++ API wrapper ospray_cpp not ospray_testing without having ospcommon installed.

paulmelis commented 4 years ago

Ah, you're right, it's indeed ospray_cpp that's the culprit here. I'll update the title

johguenther commented 4 years ago

We discussed internally, and hesitate a bit to ship ospcommon headers as well together with OSPRay. Probably the better solution is to make ospray_cpp independent on the short vector types defined in ospcommon, which however will take a bit (i.e. not ready for the soon coming bugfix release v2.1.1).

paulmelis commented 4 years ago

Fair enough, although it seems (grepping the headers in the binary distribution) that ospray_cpp isn't the only thing using ospcommon headers? E.g.

melis@juggle 19:09:~/software/ospray-2.1.0.x86_64.linux/include/ospray$ grep -r ospcommon *
...
ospray_testing/ospray_testing.h:using namespace ospcommon;
ospray_testing/ospray_testing.h:using namespace ospcommon::math;
ospray_testing/builders/Builder.h:// ospcommon
ospray_testing/builders/Builder.h:#include "ospcommon/memory/IntrusivePtr.h"
ospray_testing/builders/Builder.h:#include "ospcommon/utility/ParameterizedObject.h"
ospray_testing/builders/Builder.h:using namespace ospcommon;
ospray_testing/builders/Builder.h:using namespace ospcommon::math;
SDK/fb/tile_ops/ToneMapper.h:using namespace ospcommon;
SDK/fb/frame_ops/SSAO.h:// ospcommon
SDK/fb/frame_ops/SSAO.h:#include "ospcommon/tasking/parallel_for.h"
SDK/fb/frame_ops/Blur.h:// ospcommon
SDK/fb/frame_ops/Blur.h:#include "ospcommon/tasking/parallel_for.h"
SDK/common/OSPCommon.h:// ospcommon
SDK/common/OSPCommon.h:#include "ospcommon/math/AffineSpace.h"
SDK/common/OSPCommon.h:#include "ospcommon/memory/RefCount.h"
SDK/common/OSPCommon.h:#include "ospcommon/memory/malloc.h"
SDK/common/OSPCommon.h:using namespace ospcommon;
SDK/common/OSPCommon.h:using namespace ospcommon::math;
SDK/common/OSPCommon.h:using namespace ospcommon::memory;
SDK/common/Library.h:#include "ospcommon/os/library.h"
SDK/common/Library.h:using ospcommon::getSymbol;
SDK/common/Library.h:using ospcommon::loadLibrary;
SDK/common/Managed.h:// ospcommon
SDK/common/Managed.h:#include "ospcommon/utility/Any.h"
SDK/common/Managed.h:#include "ospcommon/utility/Optional.h"
SDK/common/Managed.h:#include "ospcommon/utility/ParameterizedObject.h"
SDK/common/Managed.h:namespace ospcommon {
SDK/common/Managed.h:} // namespace ospcommon
SDK/api/Device.h:// ospcommon
SDK/api/Device.h:#include "ospcommon/memory/IntrusivePtr.h"
SDK/api/Device.h:#include "ospcommon/utility/Optional.h"
SDK/api/Device.h:#include "ospcommon/utility/ParameterizedObject.h"
SDK/render/RenderTask.h:// ospcommon
SDK/render/RenderTask.h:#include "ospcommon/tasking/AsyncTask.h"
SDK/lights/SunSkyLight.h:#include "ospcommon/tasking/parallel_for.h"
johguenther commented 4 years ago

Extending OSPRay via the SDK, by only using the binary distribution, won't work: Not only because of ospcommon, but also headers of Open VKL and Embree are needed (which are also not shipped by OSPRay). Which actually means that shipping the SDK header is a bit, hmm, not really useful.

paulmelis commented 4 years ago

Maybe I'm misunderstanding the term SDK here, which I usually interpret as "the necessary set of development files to use package X". But you're suggesting that usage of OSPRay is supported through the C and C++ API and any details on the underlying libraries should stay hidden. And the SDK is actually something else?

johguenther commented 4 years ago

So, yes:

jeffamstutz commented 4 years ago

Yes, the history of the term "SDK" here is a bit odd and should probably be renamed (maybe "extension headers"?). Because the OSPRay "product" has always been just a library, it therefore exports a set of headers to be consumed by C/C++ projects...mainly just the usage of ospray.h. For a long long time prior to v1.0, the only way one could extend OSPRay backends is to compile from source directly, which pushed people into valuing stability where we never intended it (ex: the repo directory structure). This pushed us in the direction to use "installs" instead of "source + build directories" as what applications should consume.

When we went to v1.0, we used the term "SDK" to say "these are the things necessary to extend OSPRay from an install", so there's an SDK directory which contains these headers. However, if one wants to extend OSPRay, you must have dependencies correctly installed on the system so any internal headers can be used as they are during the build process. We, at least, reflect this in the CMake targets we export (ospray::ospray vs. ospray::ospray_sdk).

This is somewhat orthogonal, though unfortunately overlaps with, binary packaging. Our binaries that we post here on GitHub serve a very specific purpose: they allow people to "try out" OSPRay without needing to build anything. To reduce noise, we only package what is necessary to use ospray.h, which boils down to a normal OSPRay install + runtime libs of dependencies (TBB, ospcommon, Embree, Open VKL, OIDN). The overlap with the SDK here is that we explicitly omit everything else necessary to use all those dependent libraries, namely headers and CMake exported targets. We build our packages for learners of OSPRay on any host OS because other deployments of OSPRay, such as ParaView builds or in Linux/Homebrew package managers, OSPRay isn't packaged by us directly. Furthermore, we have oneAPI builds from Intel which are also separate from GitHub. In the end, it's all the same library being built, but packaging decisions is up to the distributer, not us.

This leaves us with some questions around what is the best value for us to provide: for example on Arch linux, you can get OSPRay by:

All of those are different, owned by different packagers, updated at different intervals, and sometimes configured/tailored to particular use cases. Given that we have a superbuild, we could probably post superbuild binaries that have everything, but composing that with CPack for Windows installers is quite tricky...we are having band-and-forth discussions about what provides the best value and what we (the OSPRay team) is responsible for, and what we should leave up to 3rd party packagers (e.g. installers).

We are working on an overhaul to the C++ wrappers for v2.2 that will divorce the requirement to use ospcommon and make that optional. I almost had this ready for v2.1.1, but ran into the issue that cpp::Data() constructors are based on ospcommon types. The ideal situation is for ospray_cpp to compose with any C++ vector math types (ex: glm), which I think is possible. Getting this right will take some thought, but it will likely break existing ospray_cpp code for the sake of this flexibility.

Anyway, hopefully this helps with understanding the numerous things involved, that we are solving one at a time. :)

paulmelis commented 4 years ago

Thanks for the extensive info, makes sense.

If the ospcommon::math vector types are not to be used with ospray_cpp in the current 2.1 API, what alternative is there, other than resorting to using the low-level C call ospSetVecX and friends?

Edit: Hmmm, there's setParam(<name>, OSP_VEC3F, ...) and such as well, of course.

johguenther commented 4 years ago

Oh, you can use ospcommon::math vector types with ospray_cpp in v2.1, "just" not out-of-the-box with the binary packages. For example, if you compile OSPRay yourself (with the superbuild) all should be good (and this also avoids the ABI problems of #419).

paulmelis commented 4 years ago

Yes, I know (and am using that locally), but was just wondering if there is a non-obtrusive workaround until the binary packages become fully usable with ospray_cpp again. But maybe I should not worry about that too much.

johguenther commented 4 years ago

Maybe copy the ospcommon headers to the unpacked package beside the ospray headers?

paulmelis commented 4 years ago

Sorry for leaving out part of the context, which was mostly of how to make it easiest for a user to build my app (blospray) based on the ospray binaries. But I should really look into distributing blospray with the ospray binaries included, so users don't have to build anything. Not relevant for this issue, please ignore.

jeffamstutz commented 4 years ago

In case you haven't seen this, most of the magic to get ospray_cpp working with non ospcommon::math types is already there in Traits.h. You can use this macro to say "C++ type T should be interpreted as this OSPDataType". This trait specialization then makes cpp::ManagedObject::setParam(string, T) work....so this could be hooked up to whatever native math types are being used by the calling app (such as Blender).

Using ospcommon types will continue to work, and we will still work on packaging improvements given your use case...but the above could come in handy?

jeffamstutz commented 4 years ago

And yes, I did implement an overload to setParam() that takes the OSPDataType explicitly to handle corner cases where there needs to be a parameter set without the need/ability to write the trait specialization.

jeffamstutz commented 4 years ago

Curious question @paulmelis: would you continue to use ospcommon::math types if ospray_cpp did not require them?

paulmelis commented 4 years ago

Hmm, I guess in this particular case I would use glm's vector types, as I use those for defining and passing matrices anyway.

But a lot of the ospray parameters in blospray are set based on protobuf values, which don't have vector-based access methods. So I need to extract the values separately (e.g. x, y, z) and then stuff them in a vec3f for passing to ospray. In the C API I could use ospSetVec3f, but the C++ API does not seem to have an equivalent, so the vector values there are merely an intermediate form.

By the way, I had indeed seen the method used in the traits header for type detection, but how would the actual passing of values work for, say, making a glm::vec3 passable to setParam? For this to work a glm::value_ptr(v) call would need to be inserted somewhere to go from a vector object to pointer-to-array-of-values.

jeffamstutz commented 4 years ago

The strategy ospray_cpp uses is to get the right ospSetParam() call, so the key point is to make sure that T looks like the OSPDataType in memory. For example, you could locally define:

struct MyVec4f { float x, y, z, w; };
OSPTYPEFOR_SPECIALIZATION(MyVec4f, OSP_VEC4F);

// ...

void func(cpp::Renderer r) {
  MyVec4f blah = /* ... */;
  r.setParam("bgColor", blah); // interpreted as OSP_VEC4F
}

The key here is that r.setParam("bgColor", blah) is transformed into ospSetParam(r.handle(), "bgColor", OSP_VEC4F, &blah). We rely on object addresses correctly looking like the target type we pass through the C API, so my assumption is that glm would look the same:

#include "glm/vec4.hpp"

OSPTYPEFOR_SPECIALIZATION(glm::vec4, OSP_VEC4F);

// ...

void func(cpp::Renderer r) {
  glm::vec4 blah = /* ... */;
  r.setParam("bgColor", blah); // interpreted as OSP_VEC4F
}

For the case you mention needing component-wise setParam(), we instead rely on constructing a temporary inline using ospcommon::math (or whatever other math libraries that you might use), as a component-wise setParam() would already have to do this:

r.setParam("bgColor", vec4f(r, g, b, a));

...which would be the same as:

ospSetParamVec4f(r.handle(), "bgColor", r, g, b, a);

So we've decided to go with less overloads/functions in favor of creative use of the generic setParam(string, T) version.

paulmelis commented 4 years ago

Right, I don't think glm vectors can be used then.

I was actually working just now on porting blospray to use ospray_cpp instead of the C API. Most of it is straightforward but the use of the ospcommon vector types everywhere in ospray_cpp does make it a bit unclear how to handle data coming from non-vector sources (e.g. block of bytes coming from the network representing an array of 3-floats). For example, I currently store a transfer function in two 1D arrays of float (one for colors, one for opacities) and create OSPData's for these (one OSP_VEC3F and one OSP_FLOAT) and pass them as normal parameters. But I don't see the equivalent Data constructor in ospray_cpp that explicitly takes the type of value (e.g. OSP_VEC3F). I also can't create a color Data object as a 2D (N,3) array of float, as that doesn't match the expected vec3f array and makes setParam complain. So do I really need to construct a vector of vec3f in order to pass those kinds of values in the C++ API if I want to use ospray::cpp::Data? I feel I'm missing something.

jeffamstutz commented 4 years ago

FWIW, here's a version of ospTutorial that replaces usage of ospcommon::math with glm for setting parameters...there's still a few spots that ospcommon::math has to be used (FB construction and picking), but it still demonstrates what could be possible.

ospTutorialGLM.zip

You are correct in observing we are missing a cpp::Data() constructor overload that allow direct specification of the OSPDataType like we have for setParam(). We can fix that!

I also see the gap in std:: containers not having overloads for shaped data (2D/3D). It looks like v2.2 will have a major overhaul to the C++ wrappers for better consistency. :)

As a workaround for now, cpp::Data() constructors key off entirely how T from the passed in T* is inferred to OSPDataType from traits. Thus you can cast to any pointer type that will be interpreted as the right OSPDataType. So the following will get you a 2D cpp::Data of OSP_VEC3F:

float *myArray = /* ... */;
ospray::cpp::Data myData(vec2ul(/*...*/), (vec3f*)(myArray));

Alternatively, you can also rely on the "move construction from C handles" feature of the C++ wrappers and call the C API inline:

cpp::Data myData(ospNewSharedData(/*...*/));

In the end, the result is the same.

We will still make sure that there's a true data-agnostic constructor for v2.2 in the effort to divorce the ospcommon::math requirement from ospray_cpp.

I think this whole discussion merits a full write up of the C++ wrappers once and for all. :)

jeffamstutz commented 4 years ago

(sorry I was a bit lazy in the GLM example, you have to see the type aliases at the top to realize it's actually GLM throughout...)

paulmelis commented 4 years ago

Thanks again for the example and info. Using GLM istead of ospcommon::math types could be interesting, but perhaps not all types of values needed are supported, like an unsigned vec3 of the right integer size (haven't checked this yet, actually). But good to know other vec/mat libraries can be integrated.

Some comments and observations after porting part of blospray from the C API to ospray_cpp:

jeffamstutz commented 4 years ago

Thanks for all this valuable feedback! This is good stuff to help make things better. :)

EDIT: really, these iterations here are very helpful!

perhaps not all types of values needed are supported, like an unsigned vec3 of the right integer size

Yes, GLM implements only a subset of types that OSPRay supports, so there are gaps.

As the type of a parameter being set is derived implicitly from the passed value some values did not work without casting, compared to the C API where the type is chosen by the call. E.g. ospray_renderer.setParam("maxPathLength", (int)render_settings.max_path_length()) needed the cast to int as it is a uint32 value.

Sometimes this can be addressed by explicitly passing the template parameter type. So for example, you could do:

ospray_renderer.setParam<int>("maxPathLength", renderer_settings.max_path_length());

Now if that is "better" to you or not, I'm not sure...but it is a thing. :)

I'll think about the cpp::Data() constructor use case too while I refactor it, as that is also common.

Or is the underlying memory block in the Data object passed to the object receiving the parameter?

There's some nuance here in this, so I can describe concretely what actually happens, which might help?

OSPData can either be shared or opaque. In other words, an OSPData (and thus cpp::Data) when shared actually points back to the original memory the application passed (i.e. no copy). Copying the handle around, like any other handle, is the same semantics of std::shared_ptr<>: no copies of the underlying data are made, only the handle "descriptor" of this data array. The same is true for opaque data too, but instead the data is copied into a memory buffer private to OSPRay on handle construction, so the application is permitted to free the memory separately from the lifetime of the handle. Note that cpp::Data() defaults the shared option to be false as the decoupled lifetime due to copying data is safer to be "correct"...the more dangerous shared data semantic is intentionally "opt-in".

Thus the only way to get OSPRay to free arrays are opaque OSPData which have been released with no other object references to it. As soon as an object needs to be recommitted because the contents of an OSPData have changed, you can just as easily construct another OSPData and set it...recreation semantics keeps OSPData from being stateful to updates (which if we want to implement that, we will do it for everything, not just arrays). OSPData arrays are trivially cheap to construct if shared, and are no more expensive to copy if opaque than any copies you would already have to make with "refilling the array" update semantics. FWIW, you can use ospCopyData() using a temporary shared data to just do copies into an opaque OSPData...though that seems to be missing in cpp::Data()! Noted...

The initial values of the C++ objects when default constructed is different and so is something to watch out for. E.g. Geometry, Camera and Instance default to nullptr, while Group and World point to a newly created object.

Yes, this is tricky to get right without using factory functions because default constructing a handle is always "valid", though you only get a valid underlying object if the object type does not require a string type. Do you think static factory functions are better here? Maybe something like:

auto myGeometry = ospray::cpp::Geometry::New("mesh");

Related to the above is that I seem to get a null handle provided to void ospRetain(OSPObject) when one of the C++ objects that initializes to nullptr is assigned a value later on.

We can fix that with checking if the handle isn't NULL when doing ospRetain().

Affine values seem to have no explicit support and can only be set using instance.setParam("xfm", OSP_AFFINE3F, affine_xform)?

What does "explicit support" mean? FWIW, we have affine3f from ospcommon (now rkcommon) in math/AffineSpace.h.

EDIT: ospcommon::math::affine3f will get interpreted as OSP_AFFINE3F out of the box.

paulmelis commented 4 years ago

Thanks for all this valuable feedback! This is good stuff to help make things better. :)

EDIT: really, these iterations here are very helpful!

No problem, happy to think along!

As the type of a parameter being set is derived implicitly from the passed value some values did not work without casting, compared to the C API where the type is chosen by the call. E.g. ospray_renderer.setParam("maxPathLength", (int)render_settings.max_path_length()) needed the cast to int as it is a uint32 value.

Sometimes this can be addressed by explicitly passing the template parameter type. So for example, you could do:

ospray_renderer.setParam<int>("maxPathLength", renderer_settings.max_path_length());

Now if that is "better" to you or not, I'm not sure...but it is a thing. :)

Good to know, but indeed a bit suboptimal to (have to) use that on all calls. I guess the main reason for not enforcing a fixed type for a parameter is to allow different types of values for certain parameters, e.g. vertex.color on a Mesh accepting either a vec3 or a vec4 array.

Somewhat on a side-note, but maybe interesting in this context is that in ospray-python I'm actually working "at the center" of multiple complex typing behaviours:

  1. The OSPRay C++ wrappers that allow (within limits) any type to be passed as a parameter value, but only a small subset of types per parameter actually make sense. Together with the lack of type casting of passed values and no reliable/usable check if a passed value was set successfully makes this a bit suboptimal in its usage.
  2. Needing to use a concrete parameter type in order to call the correct setParam variant
  3. Python methods allowing any type of value to be passed, hence runtime checks are necessary to see what comes in, in order to handle 1 and 2
  4. The used wrapper library, Pybind11, cannot bind to a templated method so I need to wrap all needed variants of setParam separately

All in all not much compile-time type checking of parameter values is possible here, but writing out all the possible concrete types allowed is tedious and quite some effort to keep up-to-date. And this is not only for point 2/3, but also for 1 and 4, as the types might evolve over time.

So I had been experimenting with describing the API parameters separately (in a YAML file) in order to do automatic checks on parameter values passed on the Python side, instead of relying on the parameter type mismatch messages OSPRay provides (which cannot reliably be turned into Python exceptions, plus it's ugly). More importantly such a description, in theory, allows automatic code generation for part of the wrappers saving me quite some time. Although my C++ templating skills would need some more work to do the latter in an efficient way :smile:. Here's an excerpt of the parameter description, listing per parameter which types of values are allowed. The subtype is there to handle the different strings used in constructing a Geometry object and the type would actually need to be stored somewhere in the Python-side instance to do correct checks are run-time:

Geometry:
  has_subtype: true

  box:
    box:
      - box3f[]

  isosurface:
    isovalue:
      - float
      - float[]
    volume:
      - VolumetricModel

  mesh:
    vertex.position:
      - vec3f[]
    vertex.normal:
      - vec3f[]
    vertex.color:
      - vec3f[]
      - vec4f[]
    vertex.texcoord:
      - vec2f[]
    index:
      - vec3ui[]
      - vec4ui[]
...

This is all work in progress (slowly), but I wanted to mention it here. In fact, having a separate description of parameters and their types might be interesting on the C++ side as well, or for other projects based on OSPRay.

Or is the underlying memory block in the Data object passed to the object receiving the ... missing in cpp::Data()! Noted...

Again, thanks for the extensive info, makes sense. I need to look into my Data usage a bit more it seems

The initial values of the C++ objects when default constructed is different and so is something to watch out for. E.g. Geometry, Camera and Instance default to nullptr, while Group and World point to a newly created object.

Yes, this is tricky to get right without using factory functions because default constructing a handle is always "valid", though you only get a valid underlying object if the object type does not require a string type. Do you think static factory functions are better here? Maybe something like:

auto myGeometry = ospray::cpp::Geometry::New("mesh");

I'm using different class fields, like Geometry and Instance, and this is where it matters most as some of those fields get set to actual instances later, while others can be set in the constructor. So factory methods would not really make a difference in those places. If the operation is cheap I could simply make sure all fields are set to a valid object instances in the constructor and overwrite them where needed later, but this feels suboptimal from a performance perspective.

Affine values seem to have no explicit support and can only be set using instance.setParam("xfm", OSP_AFFINE3F, affine_xform)?

What does "explicit support" mean? FWIW, we have affine3f from ospcommon (now rkcommon) in math/AffineSpace.h.

EDIT: ospcommon::math::affine3f will get interpreted as OSP_AFFINE3F out of the box.

Hmm, I guess I'm not clear on the "explicit support" myself now that I think further on it. I guess it's mostly a feeling that the affine types, although essential for setting up a scene in instances, are not really explicitly specified somewhere. It's mostly trial and error to figure out what their contents needs to be and they're slightly non-conventional in graphics in not representing a 4x4 matrix ( although dropping the right row (or is it column? ;-)) is enough to convert, of course).

johguenther commented 4 years ago

On the allowed / supported parameters and types: We have plans for an API to ask OSPRay for something similar to your "YAML" list for each object, additionally including more meta information like allowed ranges of parameters (e.g., for sun-sky light: has param turbidity of type float with range 1.0–10.0).

paulmelis commented 4 years ago

Cool, that's really good to know!

Edit: any idea for a timeframe for this addition? I will hold off on any YAML-style work for now anyway I think

johguenther commented 4 years ago

Implemented in 3b4fd74d2f328f54ad2ab403919c20358784ad91

johguenther commented 4 years ago

Now in v2.2.0.