Hydra render engine #104712

Closed
Bogdan Nagirniak wants to merge 142 commits from BogdanNagirniak/blender:hydra-render into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
13 changed files with 63 additions and 136 deletions
Showing only changes of commit 6e0c502076 - Show all commits

View File

@ -38,7 +38,6 @@ __all__ = (
"HydraRenderEngine",
"export_mtlx",
"register_plugins",
"get_render_plugins",
)
import os
@ -48,7 +47,7 @@ from pathlib import Path
import bpy
import _bpy_hydra
from _bpy_hydra import register_plugins, get_render_plugins
from _bpy_hydra import register_plugins
class HydraRenderEngine(bpy.types.RenderEngine):
@ -68,7 +67,6 @@ class HydraRenderEngine(bpy.types.RenderEngine):
@classmethod
def register(cls):
_bpy_hydra.init()
root_folder = "blender.shared" if platform.system() == 'Windows' else "lib"
os.environ['PXR_MTLX_STDLIB_SEARCH_PATHS'] = os.pathsep.join([
str(Path(bpy.app.binary_path).parent / f"{root_folder}/materialx/libraries"),

View File

@ -21,7 +21,6 @@ void FinalEngine::render(Depsgraph *depsgraph)
{
/* Release the GIL before calling into hydra, in case any hydra plugins call into python. */
pxr::TF_PY_ALLOW_THREADS_IN_SCOPE();
engine_->Execute(render_index_.get(), &tasks_);
BogdanNagirniak marked this conversation as resolved Outdated

Seems unnecessary since there is already Py_BEGIN_ALLOW_THREADS in engine_render_func.

Seems unnecessary since there is already `Py_BEGIN_ALLOW_THREADS` in `engine_render_func`.
}
@ -143,7 +142,6 @@ void FinalEngineGL::render(Depsgraph *depsgraph)
{
/* Release the GIL before calling into hydra, in case any hydra plugins call into python. */
pxr::TF_PY_ALLOW_THREADS_IN_SCOPE();
engine_->Execute(render_index_.get(), &tasks_);
}

View File

@ -11,7 +11,6 @@ void PreviewEngine::render(Depsgraph *depsgraph)
{
/* Release the GIL before calling into hydra, in case any hydra plugins call into python. */
pxr::TF_PY_ALLOW_THREADS_IN_SCOPE();
engine_->Execute(render_index_.get(), &tasks_);
}

View File

@ -20,16 +20,6 @@
namespace blender::render::hydra {
static PyObject *init_func(PyObject * /*self*/, PyObject *args)
{
CLOG_INFO(LOG_RENDER_HYDRA, 0, "Init");
pxr::PlugRegistry::GetInstance().RegisterPlugins(std::string(BKE_appdir_program_dir()) +
"/blender.shared/usd");
Py_RETURN_NONE;
}
static PyObject *register_plugins_func(PyObject * /*self*/, PyObject *args)
{
PyObject *pyplugin_dirs;
BogdanNagirniak marked this conversation as resolved Outdated

blender.shared is a Windows specific path, on other operating systems it is lib. But I don't think that path should be hardcoded here, it should be retrieved from somewhere.

`blender.shared` is a Windows specific path, on other operating systems it is `lib`. But I don't think that path should be hardcoded here, it should be retrieved from somewhere.

It could be done in several ways here:

  1. #ifdef
  2. extend BKE_appdir.h with something like BKE_appdir_lib_dir
  3. something else?

What would you prefer?

It could be done in several ways here: 1. #ifdef 2. extend `BKE_appdir.h` with something like `BKE_appdir_lib_dir` 3. something else? What would you prefer?

We have existing code for this in ensure_usd_plugin_path_registered, but actually it's not longer used now. Because the usd is located next to the USD library, it automatically loads plugins from that location.

So I think this line can just be removed now?

We have existing code for this in `ensure_usd_plugin_path_registered`, but actually it's not longer used now. Because the `usd` is located next to the USD library, it automatically loads plugins from that location. So I think this line can just be removed now?
@ -64,41 +54,6 @@ static PyObject *register_plugins_func(PyObject * /*self*/, PyObject *args)
Py_RETURN_NONE;
}
static PyObject *get_render_plugins_func(PyObject * /*self*/, PyObject *args)
{
pxr::PlugRegistry &registry = pxr::PlugRegistry::GetInstance();
pxr::TfTokenVector plugin_ids = pxr::UsdImagingGLEngine::GetRendererPlugins();
PyObject *ret = PyTuple_New(plugin_ids.size());
PyObject *val;
for (int i = 0; i < plugin_ids.size(); ++i) {
PyObject *descr = PyDict_New();
PyDict_SetItemString(descr, "id", val = PyUnicode_FromString(plugin_ids[i].GetText()));
Py_DECREF(val);
PyDict_SetItemString(
descr,
"name",
val = PyUnicode_FromString(
pxr::UsdImagingGLEngine::GetRendererDisplayName(plugin_ids[i]).c_str()));
Py_DECREF(val);
std::string plugin_name = plugin_ids[i];
plugin_name = plugin_name.substr(0, plugin_name.size() - 6);
plugin_name[0] = tolower(plugin_name[0]);
std::string path = "";
pxr::PlugPluginPtr plugin = registry.GetPluginWithName(plugin_name);
if (plugin) {
path = plugin->GetPath();
}
PyDict_SetItemString(descr, "path", val = PyUnicode_FromString(path.c_str()));
Py_DECREF(val);
PyTuple_SetItem(ret, i, descr);
}
return ret;
}
static PyObject *engine_create_func(PyObject * /*self*/, PyObject *args)
{
PyObject *pyengine;
@ -267,9 +222,7 @@ static PyObject *engine_set_render_setting_func(PyObject * /*self*/, PyObject *a
}
static PyMethodDef methods[] = {
{"init", init_func, METH_VARARGS, ""},
{"register_plugins", register_plugins_func, METH_VARARGS, ""},
{"get_render_plugins", get_render_plugins_func, METH_VARARGS, ""},
{"engine_create", engine_create_func, METH_VARARGS, ""},
{"engine_free", engine_free_func, METH_VARARGS, ""},

View File

@ -241,9 +241,9 @@ void BlenderSceneDelegate::set_setting(const std::string &key, const pxr::VtValu
pxr::SdfPath BlenderSceneDelegate::prim_id(ID *id, const char *prefix) const
{
/* Making id of object in form like <prefix>_<pointer in 16 hex digits format> */
char str[32];
snprintf(str, 32, "%s_%016llx", prefix, (uintptr_t)id);
return GetDelegateID().AppendElementString(str);
char name[32];
snprintf(name, sizeof(name), "%s_%016llx", prefix, (uintptr_t)id);
return GetDelegateID().AppendElementString(name);
}
pxr::SdfPath BlenderSceneDelegate::object_prim_id(Object *object) const

View File

@ -69,17 +69,16 @@ void CurvesData::update()
pxr::VtValue CurvesData::get_data(pxr::SdfPath const &id, pxr::TfToken const &key) const
{
pxr::VtValue ret;
if (key == pxr::HdTokens->points) {
BogdanNagirniak marked this conversation as resolved Outdated

Remove ret and just return values directly.

Remove `ret` and just return values directly.
ret = vertices_;
return pxr::VtValue(vertices_);
}
else if (key == pxr::HdPrimvarRoleTokens->textureCoordinate) {
ret = uvs_;
return pxr::VtValue(uvs_);
}
else if (key == pxr::HdTokens->widths) {
ret = widths_;
return pxr::VtValue(widths_);
}
return ret;
return pxr::VtValue();
}
bool CurvesData::update_visibility()
@ -110,19 +109,17 @@ pxr::HdPrimvarDescriptorVector CurvesData::primvar_descriptors(
if (!vertices_.empty()) {
primvars.emplace_back(pxr::HdTokens->points, interpolation, pxr::HdPrimvarRoleTokens->point);
}
if (!widths_.empty()) {
primvars.emplace_back(pxr::HdTokens->widths, interpolation, pxr::HdPrimvarRoleTokens->none);
}
}
else if (interpolation == pxr::HdInterpolationFaceVarying) {
else if (interpolation == pxr::HdInterpolationConstant) {
if (!uvs_.empty()) {
primvars.emplace_back(pxr::HdPrimvarRoleTokens->textureCoordinate,
interpolation,
pxr::HdPrimvarRoleTokens->textureCoordinate);
}
}
else if (interpolation == pxr::HdInterpolationConstant) {
if (!widths_.empty()) {
primvars.emplace_back(pxr::HdTokens->widths, interpolation, pxr::HdPrimvarRoleTokens->none);
}
}
return primvars;
}
@ -160,14 +157,12 @@ void CurvesData::write_curves(Curves *curves)
curve_vertex_counts_.push_back(num_points);
/* Set radius similar to Cycles if isn't set */
widths_.push_back(radii ? radii[i] : 0.01f);
for (int j = 0; j < num_points; j++) {
int ind = first_point_index + j;
widths_.push_back(radii ? radii[ind] * 2 : 0.01f);
vertices_.push_back(pxr::GfVec3f(positions[ind][0], positions[ind][1], positions[ind][2]));
BogdanNagirniak marked this conversation as resolved Outdated

This indexes as if the radius is per curve, but it is per curve point.

This indexes as if the radius is per curve, but it is per curve point.
}
}
write_uv_maps(curves);
}
@ -188,8 +183,9 @@ void CurvesData::write_material()
{
Object *object = (Object *)id;
Material *mat = nullptr;
/* TODO: Using only first material. Add support for multimaterial. */
if (BKE_object_material_count_eval(object) > 0) {
mat = BKE_object_material_get_eval(object, object->actcol);
mat = BKE_object_material_get_eval(object, 0);
}
if (!mat) {

View File

@ -21,16 +21,13 @@ static std::string cache_image_file(Image *image,
ImageUser *iuser,
bool check_exist)
{
std::string file_path(FILE_MAX, 0);
char file_path[FILE_MAX];
char file_name[32];
snprintf(file_name, 32, "img_%016llx.hdr", (uintptr_t)image);
BLI_path_join(file_path.data(),
file_path.capacity(),
BKE_tempdir_session(),
"hydra_image_cache",
file_name);
snprintf(file_name, sizeof(file_name), "img_%016llx.hdr", (uintptr_t)image);

Storing by pointer is correct only if changes to the image are reliably detected and this file is overwritten when there were changes. So this could lead to using an old image, though it's will be rare since the same pointer address is not likely to be used again for an image datablock.

Storing by pointer is correct only if changes to the image are reliably detected and this file is overwritten when there were changes. So this could lead to using an old image, though it's will be rare since the same pointer address is not likely to be used again for an image datablock.
BLI_path_join(
file_path, sizeof(file_path), BKE_tempdir_session(), "hydra_image_cache", file_name);
BogdanNagirniak marked this conversation as resolved Outdated

Writing beyond the size of the string and into the capacity may work in practical implementation, but I'm not sure it's strictly correct? I think it's better to keep it a C string here, and only convert to std::string at the end when returning.

Writing beyond the size of the string and into the capacity may work in practical implementation, but I'm not sure it's strictly correct? I think it's better to keep it a C string here, and only convert to `std::string` at the end when returning.
if (check_exist && BLI_exists(file_path.c_str())) {
if (check_exist && BLI_exists(file_path)) {
return file_path;
}
@ -38,18 +35,19 @@ static std::string cache_image_file(Image *image,
ImageSaveOptions opts;
opts.im_format.imtype = R_IMF_IMTYPE_RADHDR;
if (BKE_image_save_options_init(&opts, main, scene_delegate->scene, image, iuser, true, false)) {
STRNCPY(opts.filepath, file_path.c_str());
ReportList reports;
if (BKE_image_save(&reports, main, image, iuser, &opts)) {
CLOG_INFO(LOG_RENDER_HYDRA_SCENE, 1, "%s -> %s", image->id.name, file_path.c_str());
if (BKE_image_save_options_init(&opts, main, scene_delegate->scene, image, iuser, false, false))
{
STRNCPY(opts.filepath, file_path);
if (BKE_image_save(nullptr, main, image, iuser, &opts)) {
BogdanNagirniak marked this conversation as resolved Outdated

true -> false, it does not need to guess a filepath since you are setting it.

true -> false, it does not need to guess a filepath since you are setting it.
CLOG_INFO(LOG_RENDER_HYDRA_SCENE, 1, "%s -> %s", image->id.name, file_path);
}
else {
BogdanNagirniak marked this conversation as resolved Outdated

Pass nullptr for reports if you are not going to do anything with them. Then it will print errors to the console instead of silently ignroing them.

Pass `nullptr` for reports if you are not going to do anything with them. Then it will print errors to the console instead of silently ignroing them.
file_path = "";
memset(file_path, 0, sizeof(file_path));
}
}
BKE_image_save_options_free(&opts);
CLOG_INFO(LOG_RENDER_HYDRA_SCENE, 2, "%s -> %s", image->id.name, file_path);
return file_path;
}

View File

@ -102,11 +102,10 @@ void InstancerData::update()
pxr::VtValue InstancerData::get_data(pxr::TfToken const &key) const
{
ID_LOG(3, "%s", key.GetText());
pxr::VtValue ret;
if (key == pxr::HdInstancerTokens->instanceTransform) {
ret = mesh_transforms_;
return pxr::VtValue(mesh_transforms_);
}
return ret;
return pxr::VtValue();
}
bool InstancerData::update_visibility()
@ -128,7 +127,7 @@ bool InstancerData::update_visibility()
char name[16];
for (auto &l_inst : light_instances_.values()) {
for (int i = 0; i < l_inst.count; ++i) {
snprintf(name, 16, "L_%08x", i);
snprintf(name, sizeof(name), "L_%08x", i);
BogdanNagirniak marked this conversation as resolved Outdated

Use sizeof(name).

Use sizeof(name).
change_tracker.MarkRprimDirty(l_inst.data->prim_id.AppendElementString(name),
pxr::HdChangeTracker::DirtyVisibility);
}
@ -321,15 +320,15 @@ bool InstancerData::is_instance_visible(Object *object)
pxr::SdfPath InstancerData::object_prim_id(Object *object) const
{
/* Making id of object in form like <prefix>_<pointer in 16 hex digits format> */
char str[32];
snprintf(str, 32, "O_%016llx", (uint64_t)object);
return prim_id.AppendElementString(str);
char name[32];
snprintf(name, sizeof(name), "O_%016llx", (uint64_t)object);
return prim_id.AppendElementString(name);
}
pxr::SdfPath InstancerData::light_prim_id(LightInstance const &inst, int index) const
{
char name[16];
snprintf(name, 16, "L_%08x", index);
snprintf(name, sizeof(name), "L_%08x", index);
return inst.data->prim_id.AppendElementString(name);
}

View File

@ -142,20 +142,20 @@ pxr::VtValue LightData::get_data(pxr::TfToken const &key) const
pxr::VtValue ret;
auto it = data_.find(key);
if (it != data_.end()) {
ret = it->second;
return pxr::VtValue(it->second);
}
else {
std::string n = key.GetString();
if (boost::algorithm::contains(n, "object:visibility:")) {
BogdanNagirniak marked this conversation as resolved Outdated

Can you just test for equality with the full token?

Seems like it would be both more efficient and avoid matching tokens that contain these substrings but are not the same.

Can you just test for equality with the full token? Seems like it would be both more efficient and avoid matching tokens that contain these substrings but are not the same.
Fixed in https://projects.blender.org/BogdanNagirniak/blender/pulls/52
if (boost::algorithm::ends_with(n, "camera") || boost::algorithm::ends_with(n, "shadow")) {
ret = false;
return pxr::VtValue(false);
}
else {
ret = true;
return pxr::VtValue(true);
}
}
}
return ret;
return pxr::VtValue();
}
bool LightData::update_visibility()
@ -171,38 +171,32 @@ bool LightData::update_visibility()
pxr::TfToken LightData::prim_type(Light *light)
{
pxr::TfToken ret;
switch (light->type) {
case LA_LOCAL:
case LA_SPOT:
ret = pxr::HdPrimTypeTokens->sphereLight;
break;
return pxr::TfToken(pxr::HdPrimTypeTokens->sphereLight);
case LA_SUN:
ret = pxr::HdPrimTypeTokens->distantLight;
break;
return pxr::TfToken(pxr::HdPrimTypeTokens->distantLight);
case LA_AREA:
switch (light->area_shape) {
case LA_AREA_SQUARE:
case LA_AREA_RECT:
ret = pxr::HdPrimTypeTokens->rectLight;
break;
return pxr::TfToken(pxr::HdPrimTypeTokens->rectLight);
case LA_AREA_DISK:
case LA_AREA_ELLIPSE:
ret = pxr::HdPrimTypeTokens->diskLight;
break;
return pxr::TfToken(pxr::HdPrimTypeTokens->diskLight);
default:
ret = pxr::HdPrimTypeTokens->rectLight;
return pxr::TfToken(pxr::HdPrimTypeTokens->rectLight);
}
break;
default:
ret = pxr::HdPrimTypeTokens->sphereLight;
return pxr::TfToken(pxr::HdPrimTypeTokens->sphereLight);
}
return ret;
}
} // namespace blender::render::hydra

View File

@ -2,6 +2,7 @@
* Copyright 2011-2022 Blender Foundation */
#include <Python.h>
#include <unicodeobject.h>
#include <pxr/imaging/hd/material.h>
#include <pxr/imaging/hd/renderDelegate.h>
@ -74,14 +75,13 @@ void MaterialData::update()
pxr::VtValue MaterialData::get_data(pxr::TfToken const &key) const
{
pxr::VtValue ret;
if (key == scene_delegate_->settings.mx_filename_key) {
if (!mtlx_path_.GetResolvedPath().empty()) {
ret = mtlx_path_;
}
ID_LOG(3, "%s", key.GetText());
if (!mtlx_path_.GetResolvedPath().empty()) {
return pxr::VtValue(mtlx_path_);
}
}
return ret;
return pxr::VtValue();
}
pxr::VtValue MaterialData::get_material_resource() const
@ -112,7 +112,9 @@ void MaterialData::export_mtlx()
std::string path;
if (!PyErr_Occurred()) {
path = PyUnicode_AsUTF8(result);
if (PyUnicode_Check(result)) {
BogdanNagirniak marked this conversation as resolved Outdated

Should check that what was return is actually a unicode object.

Should check that what was return is actually a unicode object.
path = PyUnicode_AsUTF8(result);
BogdanNagirniak marked this conversation as resolved Outdated

Is it certain that if an error occurred, this does not need to be freed? It could be but it's not obvious to me.

Is it certain that if an error occurred, this does not need to be freed? It could be but it's not obvious to me.

result is nullptr if an error occurred

`result` is `nullptr` if an error occurred
}
Py_DECREF(result);
}
else {

View File

@ -26,18 +26,11 @@ void MeshData::init()
ID_LOG(1, "");
Object *object = (Object *)id;
if (object->type == OB_MESH && object->mode == OB_MODE_OBJECT &&
BLI_listbase_is_empty(&object->modifiers))
{
write_submeshes((Mesh *)object->data);
}
else {
Mesh *mesh = BKE_object_to_mesh(nullptr, object, false);
if (mesh) {
write_submeshes(mesh);
}
BKE_object_to_mesh_clear(object);
Mesh *mesh = BKE_object_to_mesh(nullptr, object, false);
if (mesh) {
BogdanNagirniak marked this conversation as resolved Outdated

This bypass to avoid BKE_object_to_mesh is not correct in general. The object may have been modified by e.g. shape keys, or additional attributes may have been added needed for rendering.

Cycles always uses BKE_object_to_mesh, it should not be expensive.

This bypass to avoid `BKE_object_to_mesh` is not correct in general. The object may have been modified by e.g. shape keys, or additional attributes may have been added needed for rendering. Cycles always uses `BKE_object_to_mesh`, it should not be expensive.
write_submeshes(mesh);
}
BKE_object_to_mesh_clear(object);
write_transform();
write_materials();
@ -86,17 +79,16 @@ void MeshData::update()
pxr::VtValue MeshData::get_data(pxr::SdfPath const &id, pxr::TfToken const &key) const
{
pxr::VtValue ret;
if (key == pxr::HdTokens->points) {
ret = vertices_;
return pxr::VtValue(vertices_);
}
else if (key == pxr::HdTokens->normals) {
ret = submesh(id).normals;
return pxr::VtValue(submesh(id).normals);
}
else if (key == pxr::HdPrimvarRoleTokens->textureCoordinate) {
ret = submesh(id).uvs;
return pxr::VtValue(submesh(id).uvs);
}
return ret;
return pxr::VtValue();
}
bool MeshData::update_visibility()
@ -194,7 +186,7 @@ pxr::SdfPathVector MeshData::submesh_paths() const
pxr::SdfPath MeshData::submesh_prim_id(int index) const
{
char name[16];
snprintf(name, 16, "SM_%04x", index);
snprintf(name, sizeof(name), "SM_%04x", index);
return prim_id.AppendElementString(name);
}

View File

@ -131,13 +131,12 @@ void WorldData::update(World *world)
pxr::VtValue WorldData::get_data(pxr::TfToken const &key) const
{
pxr::VtValue ret;
auto it = data_.find(key);
if (it != data_.end()) {
ret = it->second;
ID_LOG(3, "%s", key.GetText());
return pxr::VtValue(it->second);
}
return ret;
return pxr::VtValue();
}
void WorldData::write_transform()

View File

@ -255,7 +255,6 @@ void ViewportEngine::render(Depsgraph *depsgraph, bContext *context)
{
/* Release the GIL before calling into hydra, in case any hydra plugins call into python. */
pxr::TF_PY_ALLOW_THREADS_IN_SCOPE();
engine_->Execute(render_index_.get(), &tasks);
if ((bl_engine_->type->flag & RE_USE_GPU_CONTEXT) == 0) {