Hydra render engine #104712
Closed
Bogdan Nagirniak
wants to merge 142 commits from
When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
BogdanNagirniak/blender:hydra-render
into main
pull from: BogdanNagirniak/blender:hydra-render
merge into: blender:main
blender:main
blender:npr-prototype
blender:lib_macos_png
blender:icons-cleanup
blender:blender-v4.2-release
blender:blender-v3.6-release
blender:blender-v3.3-release
blender:brush-assets-project
blender:pr-extensions-tidy-space
blender:blender-v4.0-release
blender:universal-scene-description
blender:blender-v4.1-release
blender:blender-v3.6-temp_wmoss_animrig_public
blender:temp-sculpt-dyntopo
blender:gpencil-next
blender:anim/animation-id-113594
blender:blender-projects-basics
blender:bridge-curves
blender:sculpt-blender
blender:asset-browser-frontend-split
blender:asset-shelf
blender:tmp-usd-python-mtl
blender:tmp-usd-3.6
blender:blender-v3.5-release
blender:blender-v2.93-release
blender:realtime-clock
blender:sculpt-dev
blender:bevelv2
blender:xr-dev
When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
In this PR we are continuing working under Hydra render engine from https://archive.blender.org/developer/D16799
Current status:
release/scripts/modules/bpy_hydra.py
python module with API for creating other Hydra render addons:class HydraRenderEngine
. Hydra render delegates addons have to useHydraRenderEngine
.bpy_hydra.py
includesexport_mtlx()
function which exports material to.mtlx
file using MaterialX addon. It is called from C++ part._bpy_hydra
module.class BlenderSceneDelegate
which provides geometry data from Blender to Hydra.blender_addons
repository.Notes:
TODOs
Known issues
pxr\usd\usdMtlx\reader.cpp
:ND_standard_surface_surfaceshader_100.outputs:out> missing
Known issues of Storm render delegate
Ref #100892
Code style:
make format
to reformat the existing code.using namespace std
to avoid potential conflicts. You can pull specific types in likeusing std::xyz
. Though in some cases there are native types inBLI_*.hh
are preferred, including for vectors and sets for example.using namespace pxr
, use explicitpxr::
prefix like the other USD code.https://wiki.blender.org/wiki/Style_Guide/C_Cpp
The big question I guess is still code deduplication for geometry, lights, cameras, materials. If it's unclear how to approach this, we could look at one example case and see how best to approach it.
I also think we really should find a way to make Hydra work with the existing USD exporter code as a debugging aid, so we can compare both code paths to verify they are consistent. And this will likely become more important once USD nodes are used to process the scene data before it goes to Hydra.
Some quick comments from skimming the code, by no means a complete review.
@ -794,2 +794,4 @@
mark_as_advanced(POSTCONFIGURE_SCRIPT)
# USD Hydra plugin.
if(WIN32 AND WITH_USD AND WITH_MATERIALX)
We would not land this only for Windows. So remove any WIN32 checks and if it fails to build on other platforms that should be fixed at some point.
@ -902,0 +902,4 @@
if(WITH_MATERIALX)
windows_find_package(MaterialX)
if(NOT MaterialX_FOUND)
include("${LIBDIR}/MaterialX/lib/cmake/MaterialX/MaterialXTargets.cmake")
This looks like a workaround for something but there is no comment explaining it.
windows_find_package
is a no-op out of the box, we offer an option to make it work, but that's off by default, the construct here is to be expected, don't think it needs a comment?including
MaterialXTargets.cmake
rather than hardcoding values our selves, is a bit different from other targets but it's not the worst idea, less for us to maintain, could use a check the file exists perhaps just in case the filename changes in the future.@ -0,0 +3,4 @@
# <pep8 compliant>
import traceback
Don't use this, see below.
@ -0,0 +6,4 @@
import traceback
import bpy
import _hydra
_
would indicate it's a purely Blender internal module and no add-ons would be expected to use it. Is that the case, or would Hydra renderers override render engine methods and call this?We may also want to give it a name like
_bpy_hydra
to avoid potential conflicts even if it's not super likely.I'm a little unsure about the need for this to be a Python module at all, going C -> Python -> C gets a bit messy with GIL and potential deadlocks. We could consider making
HydraRenderEngine
an RNA subclass ofRenderEngine
in C.It depends a bit how we expect add-ons to override things here, how much flexibility is needed. By itself the
_hydra
methods are pretty opaque anyway, it's not so clear to me where exactly add-ons want to hook in and what the best API for that is.@ -0,0 +11,4 @@
from _hydra import register_plugins, get_render_plugins
class HydraRenderEngine(bpy.types.RenderEngine):
This will need some documentation to explain what a subclass is expected to set. Maybe docstrings can be used.
@ -0,0 +67,4 @@
try:
import materialx.utils as mx_utils
material = bpy.data.materials[material_name]
The name is not a unique identifier, different linked libraries can have materials with the same name. So the library name is needed additionally to distinguish this. But really this should be passing a material pointer rather than a material name.
Although regarding MaterialX integration we discussed elsewhere that this should be implemented in C rather than Python, so maybe this is just a placeholder.
@ -0,0 +80,4 @@
print("ERROR: no MaterialX addon available")
except Exception as e:
print("ERROR:", e, traceback.format_exc())
Don't catch all exceptions like this, we should not just print them and move on as if there was no error. There should be a failure reported to the user in the user interface and the operation aborted, not just a message in the console.
You can catch an import error for just the materialx import line, but not the whole thing.
@ -0,0 +15,4 @@
add_definitions(${GFLAGS_DEFINES})
add_definitions(${GLOG_DEFINES})
add_definitions(-DGLOG_NO_ABBREVIATED_SEVERITIES)
add_definitions(-DBLENDER_GFLAGS_NAMESPACE=${GFLAGS_NAMESPACE})
Native Blender code should use CLOG instead of GLOG.
@ -0,0 +40,4 @@
return;
}
set_env_paths("PXR_MTLX_STDLIB_SEARCH_PATHS", {stdlib_path});
Similar comments about modifying environment variables as
PATH
. Is this the only way to set MaterialX search paths?@ -0,0 +85,4 @@
}
if (!path_dirs.empty()) {
set_env_paths("PATH", path_dirs);
Modifying
PATH
seems strange to me, is that really what USD expects?This also doesn't restore the environment variable to its old values, and just keeps adding to it.
It's also not thread safe in general to modify environment variables unless we are certain it only happens in the main thread and other threads are not affected, which doesn't seems like we can guarantee for
PATH
.@ -0,0 +1,324 @@
/* SPDX-License-Identifier: Apache-2.0
* Copyright 2011-2022 Blender Foundation */
#include <epoxy/gl.h>
Don't use OpenGL functions, only use the GPU API. We are moving to Metal and Vulkan. See
intern/cycles/blender/display_driver.cpp
for how Cycles does it.@ -982,1 +982,4 @@
${LIBDIR}/materialx/bin/MaterialXGenShader.dll
${LIBDIR}/materialx/bin/MaterialXRender.dll
${LIBDIR}/materialx/bin/MaterialXRenderHw.dll
${LIBDIR}/materialx/bin/MaterialXRenderGlsl.dll
We intentionally built MaterialX without these libraries. Are they needed for Hydra? For reference:
https://archive.blender.org/developer/D15989
If we need them then we have to ensure that they work with Vulkan and Metal, and that any Vulkan or OpenGL library is loaded with dlopen so that Blender can run in headless mode without a GPU.
@ -1567,0 +1574,4 @@
install(DIRECTORY
${LIBDIR}/usd/plugin/usd/hdStorm
${LIBDIR}/usd/plugin/usd/usdShaders
DESTINATION "./blender.shared/usd"
blender.shared
is Windows specific. Please fold this into the code a few lines above that has conditions for various platforms.Is there any reason just these two plugins are copied, and
hioOiio
andsdrGlslfx
are not? We might as well be complete.It's also not clear to me if the right thing is to put these plugins in the same folder as the other ones? It's unclear to me why USD put them in a separate folder to begin with, maybe there's a good reason though maybe there's also not and this is fine.
This discussion has been done in a few other places, but I will summarize it here so we are all on the same page.
The code should be structured so that as much logic as possible can be shared with the USD export code, and where that's not possible:
set_mesh
towrite_mesh
.write_normals
.CD_NORMAL
exists. In USD it will additionally compute normals if they do not exist. Both should work the same way, otherwise there can be discrepancies between normals.Not doing this will add more technical debt than the Blender project should accept for a patch like this. This has been raised various times over the past 6 months. If it's still unclear how exactly this should be done, please let me know and we can discuss it.
Additionally.
Reviewed most of the code outside the scene delegate, for which I need better alignment with the USD export code to verify correctness without having to make a very complicated comparison.
@ -0,0 +12,4 @@
namespace blender::render::hydra {
CLG_LOGREF_DECLARE_GLOBAL(LOG_EN, "rhd.en");
LOG_EN -> LOG_RENDER_HYDRA
rhd.en -> render.hydra
No need to have an obscure name for this.
@ -0,0 +23,4 @@
pxr::HdDriverVector hd_drivers;
if (bl_engine->type->flag & RE_USE_GPU_CONTEXT) {
hgi = pxr::Hgi::CreatePlatformDefaultHgi();
This will need an addition for Vulkan support. It seems there is no way to pass a parameter, but this should work:
This should be relatively thread safe, as Blender can not switch between OpenGL and Vulkan at runtime, so it should always get set to the same thing.
@ -0,0 +32,4 @@
render_index.reset(pxr::HdRenderIndex::New(render_delegate.Get(), hd_drivers));
free_camera_delegate = std::make_unique<pxr::HdxFreeCameraSceneDelegate>(
render_index.get(), pxr::SdfPath::AbsoluteRootPath().AppendElementString("freeCamera"));
Should this name get some unique prefix so it's unlikely to clash with other elements? Or is this at a level in the namespace where this can't happen?
Yes, this is at level in the namespace where this can't happen.
@ -0,0 +35,4 @@
render_index.get(), pxr::SdfPath::AbsoluteRootPath().AppendElementString("freeCamera"));
render_task_delegate = std::make_unique<RenderTaskDelegate>(
render_index.get(), pxr::SdfPath::AbsoluteRootPath().AppendElementString("renderTask"));
if (render_delegate->GetRendererDisplayName() == "GL") {
This seems OpenGL specific, can this be made to work for Metal and Vulkan too, or is it already working somehow?
This is display name of Storm render delegate (default hydra built in renderer)
Seems it can be "Metal" too:
https://github.com/PixarAnimationStudios/USD/blob/release/pxr/usdImaging/usdImagingGL/engine.cpp#L93
Maybe comparing with the display name is not the best solution, and
GetCurrentRendererId
is better?@ -0,0 +53,4 @@
render_index = nullptr;
render_delegate = nullptr;
engine = nullptr;
hgi = nullptr;
Setting to null here seems unnecessary? If it's to guarantee a particular order of destruction, the order of these members in the class should already specify that.
We did that because we need order of destruction. We'll change it to order of members in the class
@ -0,0 +5,4 @@
#include <chrono>
#include <Python.h>
Is a direct include of Python.h needed? I don't see Python API calls.
@ -0,0 +30,4 @@
void FinalEngine::render(Depsgraph *depsgraph)
{
Scene *scene = DEG_get_input_scene(depsgraph);
ViewLayer *view_layer = DEG_get_input_view_layer(depsgraph);
Use
DEG_get_evaluated_scene
andDEG_get_evaluated_view_layer
instead.Rendering should use the scene with all animation and drivers evaluated.
Also add
const
if possible, though it may not be if some functions don't work with it.@ -0,0 +36,4 @@
BKE_id_full_name_get(scene_name.data(), (ID *)scene, 0);
std::string layer_name = view_layer->name;
RenderData &r = scene->r;
Add
const
@ -0,0 +45,4 @@
r.border.ymax - r.border.ymin);
}
pxr::GfVec2i image_res(r.xsch * r.size / 100, r.ysch * r.size / 100);
pxr::GfVec2i res(int(image_res[0] * border[2]), int(image_res[1] * border[3]));
Can this call
get_resolution()
to deduplicate code?@ -0,0 +147,4 @@
RE_engine_update_stats(bl_engine, title.c_str(), info.c_str());
}
void FinalEngineGL::render(Depsgraph *depsgraph)
It looks like there is some opportunity to deduplicate some (but not all) code with
FinalEngine::render
here?@ -0,0 +196,4 @@
std::vector<float> &pixels = render_images["Combined"];
GPUFrameBuffer *framebuffer = GPU_framebuffer_create("fbHydraRenderFinal");
GPUTexture *texture = GPU_texture_create_2d("texHydraRenderFinal",
The convention for framebuffer and texture names seems to be snake_case. So could be
fb_hdyra_render_final
andtex_hydra_render_final
.@ -0,0 +18,4 @@
instance->render_delegate->Stop();
/* Register timer for schedule free PreviewEngine instance */
BLI_timer_register((uintptr_t)instance.get(),
This is not used anywhere else in Blender internally, only in the Python API. Why is it needed here?
Preview rendering alraedy runs in its own thread as far as I know, it should be fine to run in a blocking loop waiting for the render to be done while the main thread continues.
@ -0,0 +70,4 @@
}
}
void PreviewEngine::render(Depsgraph *depsgraph)
Also perhaps some of this code can be deduplicated with final render.
@ -0,0 +11,4 @@
namespace blender::render::hydra {
CLG_LOGREF_DECLARE_GLOBAL(LOG_BSD, "rhd.bsd");
Use a less obscure name, something like:
LOG_RENDER_HYDRA_SCENE
render.hydra.scene
@ -0,0 +20,4 @@
engine_type(engine_type),
depsgraph(nullptr),
context(nullptr),
view3d(nullptr)
Null points can be initialized in the class definition directly instead of here.
@ -0,0 +29,4 @@
return ret;
}
std::string format_duration(std::chrono::milliseconds millisecs)
Can you use
BLI_timecode_string_from_time
instead of something custom for Hydra?@ -0,0 +58,4 @@
return ss.str();
}
std::string cache_image(Main *bmain,
Can you explain why this function is needed?
If for example its purpose is to primarily deal with generated or packed images, it would be much more efficient to only use it for that case.
Using PNG also means you can't use float precision images for example, which can be important for HDR images, displacement maps, etc.
Also it seems these images continue to take up disk space for the duration of the Blender process? It should be cleaned up at least when opening a new .blend.
Also I guess this is writing a new image whenever the shader is updated, which seems inefficient?
@ -0,0 +87,4 @@
image_name.append(default_format);
BLI_path_join(tempfile, sizeof(tempfile), BKE_tempdir_session(), image_name.c_str());
image_name
is not unique when an image datablock with the same name exists in multiple library .blend files.For a unique filename it may be simpler to convert the image datablock pointer value to a hexadecimal string.
I'd also suggest to put these files in a subdirectory of
BKE_tempdir_session
, e.g.hydra_image_cache
.@ -0,0 +12,4 @@
#include "BKE_camera.h"
#include "BLI_math_matrix.h"
#include "DEG_depsgraph_query.h"
#include "GPU_matrix.h"
Add empty line between includes with different prefix. Also that way you don't have to use
clang-format off
.@ -0,0 +109,4 @@
if not doc:
return ""
mtlx_file = mx_utils.get_temp_file(".mtlx", material.name)
materials can have the same name in different libraries, so this temp file name is not unique.
@ -0,0 +3,4 @@
#pragma once
#include <pxr/imaging/hd/sceneDelegate.h>
To build on Mac, it was necessary to
#include <pxr/base/gf/vec2f.h>
before including sceneDelegate.h, otherwise the compiler complained about an incomplete type deep in the USD headers.@ -0,0 +40,4 @@
#define ID_LOG(level, msg, ...) \
CLOG_INFO( \
LOG_RENDER_HYDRA_SCENE, level, "%s (%s): " msg, prim_id.GetText(), id->name, __VA_ARGS__);
The
__VAR_ARGS__
is problematic for clang, and so all invocations ofID_LOG
fail to compile. I have not yet explored the correct solution, so just turnedID_LOG
into a no-op for now.@ -0,0 +246,4 @@
int InstancerData::light_prim_id_index(pxr::SdfPath const &id) const
{
int index;
sscanf_s(id.GetName().c_str(), "L_%x", &index);
sscanf_s
does not exist in std lib on macOS, so this fails to compile there.However, the safe version of sscanf is not providing any additional benefit in this scenario, so using
sscanf
is suitable here.@ -0,0 +8,4 @@
#include <string>
struct pxr::HdMaterialNetworkMap;
This forward declaration does not work in clang, due to the
pxr
namespace, and so we hit compile errors.Instead, just use
#include <pxr/imaging/hd/material.h>
.For now, not commenting on any code other than the minimal changes required to build on macOS with Xcode 14 and clang 14.
@BrianSavery @BogdanNagirniak in the meeting yesterday it was mentioned that there were improvements to in aligning the code between Hydra and the USD exporter. However I don't see any commits related to that after I made the comment above.
I've updated PR description:
Also want to say that all review comments in code are resolved. Please review.
Sorry for misunderstanding, I didn't mention this, I mentioned only about review comments in code, that they are resolved.
About aligning Hydra and USD exporter, so investigating this topic a lot, I don't see good solution of this, because those projects are too different and solve different purposes. I also mentioned this in USD chat, there was no opinions how it could be done. We can raise this topic again.
I think Brian mention it before you joined the meeting.
I gave some concrete suggestions in my earlier comment, some of which are simple changes to naming, splitting of code into functions with matching names, and adding comments for known differences. Surely those kinds of changes are straightforward, or is it unclear how they would be done, or why it is useful?
And for converting lights and cameras, it should be possible to look at the files side by side and ensure properties are converted the same way. They may have a different purpose, but it's still converting basically the same data with a different API.
If you put the mesh export code side by side, you also see that for example multiple UV maps or per-vertex (not corner) color attributes are missing in Hydra, but not mentioned in the to do list. If you compare the light export, you'll see that there are different scaling constants. The reason these differences exist is because they were written by different people at different times, not because there is a different purpose.
Just one comment, I didn't re-review the code yet.
@ -0,0 +42,4 @@
instance_->render_delegate_->Stop();
/* Register timer for schedule free PreviewEngine instance */
BLI_timer_register((uintptr_t)&instance_, free_instance, nullptr, nullptr, LIFETIME, true);
This is still using a timer, it should not be used at all. Other render engines don't need one either to free data.
We use timer because it is not efficient to recreate Preview render engine each time when material is changes, including render of material thumbnail. Therefore we decided to free Preview engine after some time when it wasn't used. Can you propose another solution without timer or maybe it should be moved to python part?
Why is it inefficient, and how inefficient are we talking about? Is there anything that can be done to make it more efficient, maybe on the add-on side to reduce startup time or cache some slow library initialization? For Cycles and Eevee the startup time is not so much an issue.
Using a timer is and a single global instance is just not safe. There can be multiple previews at the same time, multiple threads can create a preview engine, the add-on may be disabled after which it's not longer safe to free the delegate, the instance is freed as part of static deinitialization which is too late, etc.
There are safer ways to do this, perhaps by Blender holding onto
RenderEngine
instances longer. But before we add that complexity, I'd like to understand why there is a performance issue in the first place.Initializing of render delegate - it could be a complex task including GPU initializing and memory reservation etc and could take some time (till to several seconds). Therefore recreating render delegate each time when material is changed to render material preview and thumbnail takes some time, for example in RPR delegate.
Probably this optimization has to be moved to addon side, what do you think?
Yes, this seems like it can be handled in the add-on. And then enabling viewport rendering or pressing F12 to render would likely be faster too. In Cycles this doesn't take several seconds though, not sure why it would be so slow in Hydra or RPR.
@BogdanNagirniak if this is an hydra RPR issue (not an issue in HDStorm), it should not be worked around here.
We've implemented multiple materials on mesh, where we have to split mesh to submeshes to have one material per mesh, because Hydra doesn't support multimaterials on mesh. In this side mesh export code became more different comparing to USD export.
Updated description:
We splitted and renamed some function to have matching names, comments with known differences we didn't add, we'll do this.
Yes, this is true. Comparing scaling constants for light intensity in https://projects.blender.org/blender/blender/src/branch/main/source/blender/io/usd/intern/usd_writer_light.cc#L112 and https://projects.blender.org/BogdanNagirniak/blender/src/branch/hydra-render/source/blender/render/hydra/scene_delegate/light.cc#L45 in Hydra-render we experimentally found every scaling constant depending of light type, by comparing render scenes between Cycles, Storm and RPR. So, they are different for each light type, while in bf-usd it is just divided by 100. It doesn't suit to Hydra.
Also bf-usd doesn't support spot light, while Hydra does.
I think we can do some code unification between USD export and Hydra but I propose to do this in separate PR after this PR will be merged. It requires discussion with USD export team, agree design, etc. Also I see that USD export can use some code from Hydra project. @brecht what do you think?
Thanks, but I gave just the two most obvious missing things, if you compare the mesh export code there is more. I can probably spot a bunch more things looking at objects and instancing for example. What I hoped is that you would make this comparison, not me putting the code side by side and trying to find issues.
I rather have wrong scaling in Hydra and no spot light, than an inconsistency. And then there can be PR to fix both USD and Hydra at the same time. Or a PR beforehand to fix USD, since 4.0 is a good opportunity to break compatibility.
To be honest, I have no faith that it will happen if we postpone this. And then we are likely stuck with two inconsistent implementations in 4.0.
.. can do some code unification between USD export and Hydra but I propose to do this in separate PR after this PR will be merged. It requires discussion with USD export team, agree design, etc. Also I see that USD export can use some code from Hydra project. @brecht what do you think?
To be frank, the code reviews to USD export have been incredibly slow even for adding minor features. Even if we were to refactor that side of things there, would it possibly get merged? It seems we're going to have this bifurcation no matter what we do. Unless we just make the Hydra export work exactly like the USD export and then we are hampered by limitations
I looked at the scene delegate in more detail now. The main issue I see is with instancing, there's a whole bunch of logic there that works different than Cycles and Eevee, likely does not work well with geometry nodes instances, and is soon to be outdated when parent based instancing is removed in 4.0.
The update and dirty tagging mechanism is also difficult to follow, it would help to have a comment somewhere explaining the general idea behind it.
@ -0,0 +21,4 @@
{
/* Release the GIL before calling into hydra, in case any hydra plugins call into python. */
pxr::TF_PY_ALLOW_THREADS_IN_SCOPE();
Seems unnecessary since there is already
Py_BEGIN_ALLOW_THREADS
inengine_render_func
.@ -0,0 +53,4 @@
{
/* Release the GIL before calling into hydra, in case any hydra plugins call into python. */
pxr::TF_PY_ALLOW_THREADS_IN_SCOPE();
Seems unnecessary since there is already
Py_BEGIN_ALLOW_THREADS
inengine_render_func
.@ -0,0 +22,4 @@
CLOG_INFO(LOG_RENDER_HYDRA, 1, "Init");
pxr::PlugRegistry::GetInstance().RegisterPlugins(std::string(BKE_appdir_program_dir()) +
"/blender.shared/usd");
blender.shared
is a Windows specific path, on other operating systems it islib
. 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:
BKE_appdir.h
with something likeBKE_appdir_lib_dir
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 theusd
is located next to the USD library, it automatically loads plugins from that location.So I think this line can just be removed now?
@ -0,0 +366,4 @@
pxr::SdfPath id = instancer_prim_id(object);
InstancerData *i_data = instancer_data(id);
if (i_data) {
if ((object->transflag & OB_DUPLI) == 0) {
Not quite sure what this is doing, but checking
OB_DUPLI
directly is probably wrong andBKE_object_visibility
should be used instead.@ -0,0 +69,4 @@
pxr::VtValue CurvesData::get_data(pxr::SdfPath const &id, pxr::TfToken const &key) const
{
pxr::VtValue ret;
Remove
ret
and just return values directly.@ -0,0 +160,4 @@
curve_vertex_counts_.push_back(num_points);
/* Set radius similar to Cycles if isn't set */
widths_.push_back(radii ? radii[i] : 0.01f);
This indexes as if the radius is per curve, but it is per curve point.
@ -0,0 +189,4 @@
Object *object = (Object *)id;
Material *mat = nullptr;
if (BKE_object_material_count_eval(object) > 0) {
mat = BKE_object_material_get_eval(object, object->actcol);
The active material is not what defines which materials is used for rendering, only which one is being edited. If you don't support multiple materials, used the first material.
@ -0,0 +23,4 @@
{
std::string file_path(FILE_MAX, 0);
char file_name[32];
snprintf(file_name, 32, "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.
@ -0,0 +25,4 @@
char file_name[32];
snprintf(file_name, 32, "img_%016llx.hdr", (uintptr_t)image);
BLI_path_join(file_path.data(),
file_path.capacity(),
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.@ -0,0 +38,4 @@
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)) {
true -> false, it does not need to guess a filepath since you are setting it.
@ -0,0 +41,4 @@
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)) {
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.@ -0,0 +67,4 @@
BKE_image_user_file_path_ex(main, iuser, image, file_path.data(), false, true);
if (!pxr::HioImageRegistry::GetInstance().IsSupportedImageFile(file_path)) {
file_path = cache_image_file(image, scene_delegate, iuser, true);
It's unclear why some check for the existence of the file, and why those cases.
In all cases the original image could have changed. But it's not easy to detect this here, would need tighter integration with the image system so maybe this is ok for now. Still there could be a comment that this is not entirely reliable.
@ -0,0 +25,4 @@
case OB_MESH:
case OB_SURF:
case OB_FONT:
case OB_CURVES_LEGACY:
OB_CURVES
is missing here.Instancing of
OB_CURVES
is not supported yet. It'll be implemented soon@ -0,0 +49,4 @@
/* If some of parent object is instancer, then currenct object as instancer
* is invisible in Final render */
for (Object *ob = object->parent; ob != nullptr; ob = ob->parent) {
if (ob->transflag & OB_DUPLI) {
This custom logic for checking visibility is not correct in general.
BKE_object_visibility(ob, deg_mode) & OB_VISIBLE_SELF
would be a more reliably check that applies to both the viewport and instancing.For the viewport you do need to check
BKE_object_is_visible_in_viewport
as well.I see various other places checking
OB_DUPLI
,BKE_object_visibility
is probably want you want to use instead also for those cases.@ -0,0 +127,4 @@
char name[16];
for (auto &it : light_instances_) {
for (int i = 0; i < it.second.count; ++i) {
snprintf(name, 16, "L_%08x", i);
Use sizeof(name).
@ -0,0 +233,4 @@
bool do_write_instances = false;
ListBase *lb = object_duplilist(
scene_delegate_->depsgraph, scene_delegate_->scene, (Object *)id);
LISTBASE_FOREACH (DupliObject *, dupli, lb) {
Generating duplis just for this check looks very expensive and
O(n^2)
time complexity as well.@ -0,0 +350,4 @@
it.second.transforms.clear();
}
ListBase *lb = object_duplilist(
Cycles and Eevee don't use this function directly, but instead use a depsgraph iterator over all instances in the scene. For Hydra it looks like a different mechanism is used, and most likely wrong in various cases.
If at all possible it should really use the iterator over all instances in the scene instead. It's very hard to verify that this generating the same instances and visibility.
@ -0,0 +146,4 @@
else {
std::string n = key.GetString();
if (boost::algorithm::contains(n, "object:visibility:")) {
if (boost::algorithm::ends_with(n, "camera") || boost::algorithm::ends_with(n, "shadow")) {
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 BogdanNagirniak/blender#52
@ -0,0 +112,4 @@
std::string path;
if (!PyErr_Occurred()) {
path = PyUnicode_AsUTF8(result);
Should check that what was return is actually a unicode object.
@ -0,0 +113,4 @@
if (!PyErr_Occurred()) {
path = PyUnicode_AsUTF8(result);
Py_DECREF(result);
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
isnullptr
if an error occurred@ -0,0 +27,4 @@
Object *object = (Object *)id;
if (object->type == OB_MESH && object->mode == OB_MODE_OBJECT &&
BLI_listbase_is_empty(&object->modifiers))
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.@ -0,0 +97,4 @@
void ObjectData::update_parent()
{
Object *object = (Object *)id;
if (parent_ != object->parent) {
Parenting based duplication is planned to be removed in Blender 4.0, and replaced by geometry nodes and modifiers. It already is not the only way for instancing to happen, as geometry nodes can generate instances as well.
Render engines are not supposed to care about the detail of how instances are generated, the depsgraph iterator and object_duplilist tell you what the instances are. I'm not sure what this code does exactly, but it's probably either wrong or incomplete.
@ -0,0 +83,4 @@
if (!color_input.directly_linked_links().is_empty()) {
bNode *color_input_node = color_input.directly_linked_links()[0]->fromnode;
if (color_input_node->type == SH_NODE_TEX_IMAGE) {
In Cycles and Eevee, the world typically has an Environment Texture node rather than an Image Texture node, to get the right image mapping. If this is going to be just an approximation, that may be a better guess. Or it could do both of course.
This is in our TODOs list to support more nodes for World including Environment Texture node
I didn't review the scene delegate part, because I was waiting for it to be implemented with the right approach, in a way that makes it practical to review and verify correctness.
Let me quote myself from when this was initially submitted:
And instead of taking that approach or at least getting a reply that disagreed with this, it was seemingly ignored.
Anyway, we can try to find some compromise in a discussion outside this PR.
main
branch c20f181fabWe figured out the light units mapping to USD in #109404 and #109795. The Hydra code should be changed to match the USD export code here:
https://projects.blender.org/blender/blender/src/branch/main/source/blender/io/usd/intern/usd_writer_light.cc
Can I push commits to the branch for this pull request (
BogdanNagirniak/blender:hydra-render
), or is that going to cause problems for you to merge things?Sure, you can do pull request to https://projects.blender.org/BogdanNagirniak/blender/pulls, I added you to callaborators.
A few issue I found testing this with Storm:
Thanks, I submitted a few pull requests now.
Thank you!
Currently I've been working and investigating this issues. This is happen because of using
GL_POINT_SMOOTH
inhttps://github.com/PixarAnimationStudios/OpenUSD/blob/release/pxr/imaging/hgiGL/scopedStateHolder.cpp#L142
https://github.com/PixarAnimationStudios/OpenUSD/blob/release/pxr/imaging/hgiGL/scopedStateHolder.cpp#L289
and some other OpenGL functionality. @brecht do you know why this cause GL error
invalid enum
in Blender? Inusdview
tool there is no such errorThis is next my task for investigation.
Blender uses the OpenGL core profile, and GL_POINT_SMOOTH is not part of that.
Thanks, that helped me to find reason of bug. Just tried to use
WGL_CONTEXT_COMPATIBILITY_PROFILE_BIT_ARB
here https://projects.blender.org/blender/blender/src/branch/main/intern/ghost/intern/GHOST_SystemWin32.cc#L307 and no such GL warning. Also that fixed issue:This is already works in this way
I think this is now close to ready to merge. The one blocking thing I think is macOS support from BogdanNagirniak/blender#70.
The MaterialX part I will leave out of the initial commit, as I think that should be changed to use blender/blender#108823.
I would have liked the scene delegate to be more aligned to the USD code, but let's keep them as to do items for afterwards. The main things I know of:
BKE_camera.h
. It should also be more aligned to USD export, however that one is known to have various issues also so both would need changes.@blender-bot build
I tried to merge main, but it seems to break viewport renders. Why I did not have time to figure out today. But it would be good to merge so we can check it all builds correctly.
@blender-bot build
Only blender organization members with write access can start builds. See documentation for details.
Merged with
main
branch, tested from my side - works as expected. In last commits there were fixed blocking issues and made code polishing where possible. I think we are close to merge. @brecht what do you think?@blender-bot build
@blender-bot package
Package build started. Download here when ready.
WIP: Hydra render engineto Hydra render engine