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.
Contributor

In this PR we are continuing working under Hydra render engine from https://archive.blender.org/developer/D16799

Current status:

  1. Python part:
    • Created release/scripts/modules/bpy_hydra.py python module with API for creating other Hydra render addons: class HydraRenderEngine. Hydra render delegates addons have to use HydraRenderEngine.
    • Also bpy_hydra.py includes export_mtlx() function which exports material to .mtlx file using MaterialX addon. It is called from C++ part.
  2. C++ part: created render_hydra project in source/blender/render/hydra:
    • Provides python API for Hydra render engine: _bpy_hydra module.
    • Implemented Blender scene delegate class BlenderSceneDelegate which provides geometry data from Blender to Hydra.
    • Currently BlenderSceneDelegate exports following types of objects: Mesh (including editable mesh), Metaboll, Curve, Surface, Text, Light (supports all types af light).
    • Implemented object's instancing: vertices and faces for Mesh object, collection for Empty object.
    • Implemented environment light.
    • Implemented multiple materials on mesh. It requires to split mesh to submeshes to have one material per mesh, because Hydra doesn't support multimaterials on mesh.
    • Implemented rendering: Final (F12), Viewport, Material preview.
    • Viewport rendering supports all updates including: light/mesh/object changes, transform, add/remove objects, material changes/assignments, objects visibility, instancing changes.
  3. Material export currently is implemented via MaterialX addon. PR to 'blender-addons' repo exists PR for MaterialX addon, but it is not going to be merged. Without this addon, scene would be without materials.
  4. Created PR for Storm Hydra render addon in blender_addons repository.
  5. Here is implementation of RPR Hydra render addon.

Notes:


TODOs

  • Curves (hair) export
  • Volumes export
  • Support more nodes for Environment (World) light
  • Build for Mac OS
  • Build for Linux
  • Multiple UV maps
  • Per-vertex color attributes
  • Implement Viewport render with material preview

Known issues

  • Caching generated image leads to infinite loop in Preview render
  • Warning in console with pxr\usd\usdMtlx\reader.cpp: ND_standard_surface_surfaceshader_100.outputs:out> missing

Known issues of Storm render delegate

  • Final render returns empty image
  • Viewport render could return emtpy image after changing object's selection
  • UVs aren't exported: no texture mapping
  • GL warnings in console
  • DomeLight warning in console

Ref #100892

In this PR we are continuing working under Hydra render engine from https://archive.blender.org/developer/D16799 **Current status:** 1. Python part: - Created `release/scripts/modules/bpy_hydra.py` python module with API for creating other Hydra render addons: `class HydraRenderEngine`. Hydra render delegates addons have to use `HydraRenderEngine`. - Also `bpy_hydra.py` includes `export_mtlx()` function which exports material to `.mtlx` file using [MaterialX addon](https://projects.blender.org/BogdanNagirniak/blender-addons/src/branch/materialx-addon/materialx). It is called from C++ part. 2. C++ part: created render_hydra project in source/blender/render/hydra: - Provides python API for Hydra render engine: `_bpy_hydra` module. - Implemented Blender scene delegate `class BlenderSceneDelegate` which provides geometry data from Blender to Hydra. - Currently BlenderSceneDelegate exports following types of objects: Mesh (including editable mesh), Metaboll, Curve, Surface, Text, Light (supports all types af light). - Implemented object's instancing: vertices and faces for Mesh object, collection for Empty object. - Implemented environment light. - Implemented multiple materials on mesh. It requires to split mesh to submeshes to have one material per mesh, because Hydra doesn't support multimaterials on mesh. - Implemented rendering: Final (F12), Viewport, Material preview. - Viewport rendering supports all updates including: light/mesh/object changes, transform, add/remove objects, material changes/assignments, objects visibility, instancing changes. 3. Material export currently is implemented via [MaterialX addon](https://projects.blender.org/BogdanNagirniak/blender-addons/src/branch/materialx-addon/materialx). PR to 'blender-addons' repo exists [PR for MaterialX addon](https://projects.blender.org/blender/blender-addons/pulls/104594), but it is not going to be merged. Without this addon, scene would be without materials. 4. Created PR for [Storm Hydra render addon](https://projects.blender.org/blender/blender-addons/pulls/104597) in `blender_addons` repository. 5. Here is implementation of [RPR Hydra render addon](https://github.com/bnagirniak/RPRHydraRenderBlenderAddon). ----- **Notes:** * Currenty we tested hydra render on Windows only, but build on Mac should be working. * Most of the functionality was tested with [RPR Hydra render addon](https://github.com/bnagirniak/RPRHydraRenderBlenderAddon). [Storm Hydra render addon](https://projects.blender.org/blender/blender-addons/pulls/104597) has issues and working in Viewport render only. ----- **TODOs** - [x] Curves (hair) export - [x] Volumes export - [x] Support more nodes for Environment (World) light - [x] Build for Mac OS - [x] Build for Linux - [ ] Multiple UV maps - [ ] Per-vertex color attributes - [x] Implement Viewport render with material preview **Known issues** - [x] Caching generated image leads to infinite loop in Preview render - [ ] Warning in console with `pxr\usd\usdMtlx\reader.cpp`: `ND_standard_surface_surfaceshader_100.outputs:out> missing` **Known issues of Storm render delegate** - [x] Final render returns empty image - [x] Viewport render could return emtpy image after changing object's selection - [x] UVs aren't exported: no texture mapping - [ ] GL warnings in console - [x] DomeLight warning in console Ref #100892
Bogdan Nagirniak added 29 commits 2023-02-13 19:05:34 +01:00
16bb823b36
BLEN-262: Initial implementation (#52)
Added basic code
Updated addon structure
Fixing libs and build
e011e2a842
BLEN-263: Python API for new render delegates with HdStorm render engine (#53)
Simplified HydraRenderEngine.
Added storm/properties.py, storm/ui.py. Fixed/cleanuped python code.
Made small adjustments of c++ code.
6107598d23
BLEN-275: Adjust c++ code (#54)
Renamed session.h/cpp -> engine.h/cpp.
Created classes Engine, FinalEngine, ViewportEngine. Moved code from BlenderSession to Engine classes.
Simplified python and c++ code.
ad4d536411
BLEN-276: Refactor/cleanup code: split Engine classes into several files (#55)
Refactor:
- Moved code from camera.cpp, view_settings.cpp to viewportEngine.cpp
- Moved related code to finalEngine.cpp and viewportEngine.cpp
- Removed stage.cpp/h, camera.cpp/h, view_settings.cpp/h.

Removed unused includes.
Removed unused functions.
Added stage_export_to_str_func() to engine.cpp.
c6e98e7fd5
BLEN-266: Create new HdRPR USDHydra render addon (#56)
Added more properties to Storm engine, renamed some parts.
Updated register_plugins_func().
Fixed showing some panels.
b0220cf008
BLEN-279: Create initial BlenderSceneDelegate. BLEN-280: Implement syncing of mesh, light and camera (#58)
Added initial code for Blender scene delegate.
Made UsdImagingLite to use BlenderSceneDelegate.
Basic implementation providing mesh data in BlenderSceneDelegate.
2b960a1c1e
BLEN-281: Render with BlenderSceneDelegate (#60)
Refactoring render process:
- Removed UsdImagingLite, moved render code into Engines directly.
- Simplifying rendering process.

Added sceneDelegate/scene.h(.cpp).

Fixed bug BLEN-293: Fix normals in BlenderSceneDelegate.
67eafc8ff1 BLEN-287: Create build against vfx-platform-2023 branch from blender libs (#57)
- Fixed registering USD plugins
- Added copying necessary usd/plugin folders

Co-authored-by: markyandex@gmail.com <markyandex@gmail.com>
70f4f2af83 BLEN-288: Fix debug build of Blender with usdhydra (#59)
Fix definitons and linked libs for debug build

Co-authored-by: markyandex@gmail.com <markyandex@gmail.com>
1f8e9641bd
BLEN-297: Viewport render with BlenderSceneDelegate (#61)
Make code to use HdxRenderTask and HdxRenderTaskParams.
Implemented viewport render for RPR delegate.
Updated RenderTaskDelegate to support updates.
Splitted engine.h -> engine.h, finalEngine.h, viewportEngine.h
Removed unused code related to USD stages.
cb878d38c1
BLEN-301: Implement updates in viewport render (#62)
Implemented updates for mesh objects: transform, full mesh, add, remove.
11593c9e74
BLEN-295: Light export (#63)
Added light export to Hydra. Made light to be updated, added/removed in viewport.
Added panel with Light properties.

Co-authored-by: Bogdan Nagirniak <bnagirniak@luxoft.com>
e3baebc96f
BLEN-298: Storm render delegate with BlenderSceneDelegate (#64)
Implemented HdStorm renderer plugin for viewport
2bd848b5c5
BLEN-294: Assign MaterialX materials (#65)
Implemented assignment of materials in BlenderSceneDelegate.
Made material conversion to .mtlx using MaterialX plugin by calling python code from c++.
Implemented material update/add/remove in viewport.
Added UVs export.
Made code improvements.
48ed9a340a
BLEN-321: Fix export to mtlx with dependencies. (#66)
Adjusted code due to changes in materialx.utils.export_to_file().
1a8674f175
BLEN-308: Storm render delegate with BlenderSceneDelegate for final render (#67)
Implemented Storm render delegate with BlenderSceneDelegate for final render
20857a4a19
BLEN-305: Refactor project due to suggestments in D16799 (#69)
Refactor c++ part:
- moved project extern_usdhydra to blender/source/render/hydra and renamed to bf_render_hydra
- renamed cmake key: WITH_USDHYDRA -> WITH_HYDRA
- renamed namespace to blender::render::hydra
- moved code from finalEngineGL.cpp/.h to finalEngine.cpp/.h
- moved py api code from engine.cpp to python.cpp
- Renamed *.cpp -> *.cc.

Refactor python part:
- moved python code to release/scripts/modules/hydra.py
- removed storm and addon code, as it became a module.
e2d09d1be0
BLEN-325: Refactor project due to suggestments in D16799. Part 2 (#70)
1. Changed syncing logic for objects, now they'll be stored in ObjectData class which holds required data for object like mesh (+meshables), light, camera. Not everything is implemented yet. Export of object data is moved to object.cc.
2. Iterating through depsgraph is still use RNA API.
3. Improved working with objects for future implementations.
4. BlenderSceneDelegate::Populate() funcions splitted to several functions for better support.
2217d8a49e
BLEN-296: Export other geometry: metaball, text, nurbs (#71)
Added export of: metaball, text, curves, surface.
Added export of editable mesh.
13e131b519
BLEN-334: Export objects visibility (#72)
Added taking into accout objects visibility.

Co-authored-by: Bogdan Nagirniak <bnagirniak@luxoft.com>
09aa07f2ee
BLEN-331: Cleanup code with Camera creation in viewport + change RNA->DNA (#73)
Moved camera export code to CameraData class in camera.cc/.h.
Changed camera export API to DNA.
Made code improvements.
Bogdan Nagirniak requested review from Brecht Van Lommel 2023-02-13 19:06:06 +01:00
Brecht Van Lommel added this to the USD project 2023-02-13 19:08:59 +01:00
Brecht Van Lommel added the
Interest
Render Pipeline
Interest
USD
labels 2023-02-13 19:09:21 +01:00
Bogdan Nagirniak added 1 commit 2023-02-17 14:47:01 +01:00
df4c717f8e BLEN-335: Export environment light
### Purpose
Add initial implementation of export environment light to hydra. Both color and images supported.

### Technical steps

Added caching images.
Added new class `WorldData`.
How it works:
1. find output node for word light;
2. find linked node and read inputs;
3. if `color` linked with image - cache image on disk;

Use with https://github.com/bnagirniak/RPRHydraRenderBlenderAddon/pull/6

Co-authored-by: georgiy.m.markelov@gmail.com <georgiy.m.markelov@gmail.com>
Pull Request #1
Bogdan Nagirniak added 1 commit 2023-02-18 08:00:43 +01:00
5570d04563 BLEN-345-Refactor export process
### Purpose
Move geometry export, working with HdRenderIndex to ObjectData classes, to be more flexible in future.
Fix area light visibility + normalize intensity.

### Technical steps
1. Refactored ObjectData class: separated code to hierarchic classes IdData, ObjectData, MaterialData, MeshData, LightData.
2. Moved working with HdRenderIndex (Insert/remove/mark_dirty) from BlenderSceneDelegate to \*Data classes.
3. Fixed area light visibility, normalized intensity.

Co-authored-by: Bogdan Nagirniak <bnagirniak@luxoft.com>
Pull Request #2
Bogdan Nagirniak added 1 commit 2023-02-21 09:48:46 +01:00
7dcae7f50c BLEN-333: Use DNA instead RNA blender API for export/update depsgraph
### Purpose
RNA API supposed to use in python bindings. DNA API is much more flexible and powerful.

### Technical steps
1. Moved depsgraph export and updates to DNA API.
2. Moved update_visibility() implementation to update_collection().
3. Adjusted depsgraph and context pointers.
4. Preparations for export instances: added InstanceData class.
Bogdan Nagirniak added 1 commit 2023-03-02 12:35:59 +01:00
85864b39a4 BLEN-326: Pass MaterialX to Hydra via hdMaterialNetwork
Created mtlxHydraAdapter.h\cc. Adjusted MaterialData.

Storm does not work correctly. It'll be fixed in a separate PR because they are unrelated to the mtlx integration.

Pull Request #5
Bogdan Nagirniak added 1 commit 2023-03-03 16:07:45 +01:00
Bogdan Nagirniak added 1 commit 2023-03-10 17:38:13 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
6b83ff9ed4
BLEN-352: Move to use DNA API in engines
### Purpose
There is still part of code which use RNA API instead of native DNA API.

### Technical steps
1. Changed to DNA API in: camera.cc/.h, engine files.
2. Added set_env_paths() to utils.
3. Made some renamings.

Pull Request #12

Code style:

  • Use snake_case for filenames, not camelCase.
  • Use clang-format. Run make format to reformat the existing code.
  • Group #includes by prefix like DNA, BKE, BLI, etc.
  • Do not use using namespace std to avoid potential conflicts. You can pull specific types in like using std::xyz. Though in some cases there are native types in BLI_*.hh are preferred, including for vectors and sets for example.
  • Don't use using namespace pxr, use explicit pxr:: prefix like the other USD code.

https://wiki.blender.org/wiki/Style_Guide/C_Cpp

Code style: * Use snake_case for filenames, not camelCase. * Use [clang-format](https://wiki.blender.org/wiki/Tools/ClangFormat). Run `make format` to reformat the existing code. * Group #includes by prefix like DNA, BKE, BLI, etc. * Do not use `using namespace std` to avoid potential conflicts. You can pull specific types in like `using std::xyz`. Though in some cases there are native types in `BLI_*.hh` are preferred, including for vectors and sets for example. * Don't use `using namespace pxr`, use explicit `pxr::` 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.

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.
Brecht Van Lommel requested changes 2023-03-11 01:17:44 +01:00
Brecht Van Lommel left a comment
Owner

Some quick comments from skimming the code, by no means a complete review.

Some quick comments from skimming the code, by no means a complete review.
CMakeLists.txt Outdated
@ -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.

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.
BogdanNagirniak marked this conversation as resolved
@ -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.

This looks like a workaround for something but there is no comment explaining it.
Member

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.

`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.
BogdanNagirniak marked this conversation as resolved
@ -0,0 +3,4 @@
# <pep8 compliant>
import traceback

Don't use this, see below.

Don't use this, see below.
BogdanNagirniak marked this conversation as resolved
@ -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 of RenderEngine 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.

`_` 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 of `RenderEngine` 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.
BogdanNagirniak marked this conversation as resolved
@ -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.

This will need some documentation to explain what a subclass is expected to set. Maybe docstrings can be used.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
BogdanNagirniak marked this conversation as resolved
@ -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.

Native Blender code should use CLOG instead of GLOG.
BogdanNagirniak marked this conversation as resolved
@ -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?

Similar comments about modifying environment variables as `PATH`. Is this the only way to set MaterialX search paths?
BogdanNagirniak marked this conversation as resolved
@ -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.

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`.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
BogdanNagirniak marked this conversation as resolved
@ -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 and sdrGlslfx 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.

`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` and `sdrGlslfx` 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.
BogdanNagirniak marked this conversation as resolved
Bogdan Nagirniak requested review from Brian Savery (AMD) 2023-03-11 06:16:18 +01:00
Bogdan Nagirniak added 1 commit 2023-03-15 09:42:25 +01:00
b6e420f8b2 BLEN-367: Fix code style
1. renamed files
2. files formated with `make format`
3. camelCase -> snake_case for variables
4. capitalized `enum DirtyBits`
5. removed `using namespace`
6. rewritten `gf_matrix_from_transform`

Pull Request #14
Bogdan Nagirniak added 1 commit 2023-03-20 10:45:29 +01:00
9af1f0ad0d BLEN-369: Change GLOG to CLOG
### Purpose
CLOG logger should be used.

### Technical steps
Changed logger from GLOG to CLOG, adjusted log messages. Improved some logs.

### Notes
CLOG_INFO can be tested by running `blender.exe --log-level 3 --log rhd.* <--log-show-timestamp> <--log-show-basename>`

Pull Request #16
Bogdan Nagirniak added 1 commit 2023-03-24 18:11:38 +01:00
269deec0f6 BLEN-372: Fix copying hdStorm and usdShaders libraries
### Purpose
Fix build code due to code review comments.

### Technical steps
1. Set WITH_HYDRA option ON for all platforms.
2. Removed unnecessary build flags.
3. Removed copying plugin/usd/hdStorm, it has to be moved to Hydra storm addon

### Notes
Storm is working with BogdanNagirniak/blender-addons#6

Pull Request #18
Bogdan Nagirniak added 2 commits 2023-03-27 12:42:07 +02:00
38378e3db4 Adjusted MeshData::set_mesh()
Removed unused includes from CMakeLists.txt and fixed warning C4100.
Bogdan Nagirniak reviewed 2023-04-06 08:42:14 +02:00
Bogdan Nagirniak added 1 commit 2023-04-07 13:28:58 +02:00
aa002fec8e Move to use the GPU API instead OpenGL
### Purpose
We have to use GPU API instead OpenGL

### Technical steps
1. Rewritten code in GLTexture, renamed GLTexture -> DrawTexture.
   Improved: moved GPUBatch creation to constructor instead creating it in draw().
2. Rewritten code in FinalEngineGL.
3. Cleaned up CMakeLists.txt.

### Notes
FinalEngineGL still shows empty picture, this bug isn't fixed here.

Pull Request [#23](BogdanNagirniak/blender#23)
Bogdan Nagirniak added 1 commit 2023-04-10 12:44:57 +02:00
3f97872f21 BLEN-365: Improve creation algorithm of PreviewEngine
**Purpose**
`PreviewEngine` shouldn’t be recreated every time when material is changed.

**Technical steps**
Python:
1. Creation and sync of engine moved to `def update`

C++:
1. Added delayed deletion for `PreviewEngine` (180 sec for now). Timer is refreshed with every change;
2. Moved creation of `BlenderSceneDelegate` from every `Engine`'s inheritor to `Engine::Engine`;
3. `PreviewEngine::sync` clears `render_index` and `scene_delegate`'s data;

Co-authored-by: Bogdan Nagirniak <bodyan@gmail.com>
Pull Request BogdanNagirniak/blender#21
Bogdan Nagirniak added 2 commits 2023-04-10 18:09:40 +02:00

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:

  • Use matching file names, same function names and same order in the code where possible. For example:
    • Rename set_mesh to write_mesh.
    • Move normal export to a separate function named write_normals.
  • Match the implementation as much as possible, use the same variable names, utility functions, etc. For example:
    • In Hydra it will export normals only if 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.
  • Where the Hydra implementation is incompletely or intentionally different, there should be code comments explaining this. We do not want to find out bug report by bug report where those problems are, we can know this in advance.

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.

  • There should be a developer option to export the scene with the USD exporter, loading the stage into Hydra. This will help verifying that the results match.
  • There should be a good overview that on a user level clarifies what level of compatibility there is expected to be, and what is known to not be supported yet.
  • For shaders the situation is unclear to me. It seems like USD export is geared around USD preview surface, while this is based on MaterialX. We should aim to also have compatibility here. Perhaps this part is also not as performance critical, and code sharing is possible here?
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: * Use matching file names, same function names and same order in the code where possible. For example: * Rename `set_mesh` to `write_mesh`. * Move normal export to a separate function named `write_normals`. * Match the implementation as much as possible, use the same variable names, utility functions, etc. For example: * In Hydra it will export normals only if `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. * Where the Hydra implementation is incompletely or intentionally different, there should be code comments explaining this. We do not want to find out bug report by bug report where those problems are, we can know this in advance. 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. * There should be a developer option to export the scene with the USD exporter, loading the stage into Hydra. This will help verifying that the results match. * There should be a good overview that on a user level clarifies what level of compatibility there is expected to be, and what is known to not be supported yet. * For shaders the situation is unclear to me. It seems like USD export is geared around USD preview surface, while this is based on MaterialX. We should aim to also have compatibility here. Perhaps this part is also not as performance critical, and code sharing is possible here?
Brecht Van Lommel requested review from Brecht Van Lommel 2023-04-13 18:52:32 +02:00
Brecht Van Lommel requested changes 2023-04-13 20:29:31 +02:00
Brecht Van Lommel left a comment
Owner

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.

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.

LOG_EN -> LOG_RENDER_HYDRA rhd.en -> render.hydra No need to have an obscure name for this.
BogdanNagirniak marked this conversation as resolved
@ -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:

if (GPU_backend_get_type() == GPU_BACKEND_VULKAN) {
  setenv HGI_ENABLE_VULKAN=1
}

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.

This will need an addition for Vulkan support. It seems there is no way to pass a parameter, but this should work: ``` if (GPU_backend_get_type() == GPU_BACKEND_VULKAN) { setenv HGI_ENABLE_VULKAN=1 } ``` 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.
BogdanNagirniak marked this conversation as resolved
@ -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?

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?
Author
Contributor

Yes, this is at level in the namespace where this can't happen.

Yes, this is at level in the namespace where this can't happen.
BogdanNagirniak marked this conversation as resolved
@ -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 seems OpenGL specific, can this be made to work for Metal and Vulkan too, or is it already working somehow?
Author
Contributor

This is display name of Storm render delegate (default hydra built in renderer)

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?

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?
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
Author
Contributor

We did that because we need order of destruction. We'll change it to order of members in the class

We did that because we need order of destruction. We'll change it to order of members in the class
BogdanNagirniak marked this conversation as resolved
@ -0,0 +5,4 @@
#include <chrono>
#include <Python.h>

Is a direct include of Python.h needed? I don't see Python API calls.

Is a direct include of Python.h needed? I don't see Python API calls.
BogdanNagirniak marked this conversation as resolved
@ -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 and DEG_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.

Use `DEG_get_evaluated_scene` and `DEG_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.
BogdanNagirniak marked this conversation as resolved
@ -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

Add `const`
BogdanNagirniak marked this conversation as resolved
@ -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?

Can this call `get_resolution()` to deduplicate code?
BogdanNagirniak marked this conversation as resolved
@ -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?

It looks like there is some opportunity to deduplicate some (but not all) code with `FinalEngine::render` here?
BogdanNagirniak marked this conversation as resolved
@ -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 and tex_hydra_render_final.

The convention for framebuffer and texture names seems to be snake_case. So could be `fb_hdyra_render_final` and `tex_hydra_render_final`.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
BogdanNagirniak marked this conversation as resolved
@ -0,0 +70,4 @@
}
}
void PreviewEngine::render(Depsgraph *depsgraph)

Also perhaps some of this code can be deduplicated with final render.

Also perhaps some of this code can be deduplicated with final render.
BogdanNagirniak marked this conversation as resolved
@ -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

Use a less obscure name, something like: LOG_RENDER_HYDRA_SCENE render.hydra.scene
BogdanNagirniak marked this conversation as resolved
@ -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.

Null points can be initialized in the class definition directly instead of here.
BogdanNagirniak marked this conversation as resolved
@ -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?

Can you use `BLI_timecode_string_from_time` instead of something custom for Hydra?
BogdanNagirniak marked this conversation as resolved
@ -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.

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?

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?
BogdanNagirniak marked this conversation as resolved
@ -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.

`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`.
BogdanNagirniak marked this conversation as resolved
@ -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.

Add empty line between includes with different prefix. Also that way you don't have to use `clang-format off`.
BogdanNagirniak marked this conversation as resolved
Brecht Van Lommel requested changes 2023-04-14 12:09:18 +02:00
@ -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.

materials can have the same name in different libraries, so this temp file name is not unique.
BogdanNagirniak marked this conversation as resolved
Bogdan Nagirniak added 1 commit 2023-04-19 02:46:33 +02:00
40e3c2eb8c Support several instancers on one object
### Purpose
Instancer object could provide instances for its children objects, but we support only one instance object. Also removing/changing/adding of instancer object isn't working correctly.

### Technical steps
1. Restructured `InstancerData`:
    - now it could hold several `ObjectData` which are instances
    - restructured order of instancer and instance objects in `RenderIndex`.
2. Refactoring:
    - improved logging in `IdData` children classes
    - splitted `populate()` and `update_collection()` in `BlenderSceneDelegate` to several functions for more flexibility.
3. Improved/fixed:
    - object updates (transform, mesh update, remove) inside instancer
    - instancer updates (transform, update, remove).

Pull Request BogdanNagirniak/blender#24
Bogdan Nagirniak added 1 commit 2023-04-21 15:02:54 +02:00
3fba132714 Fix review comments
### Purpose
Refactoring to match Blender's team requirements and code style.

### Technical steps
* Replaced `utils.h/format_duration` with `BLI_timecode_string_from_time_simple`
* Made material names unique
* `LOG_EN` -> `LOG_RENDER_HYDRA`, `rhd.en` -> `render.hydra`
* `LOG_BSD` -> `LOG_RENDER_HYDRA_SCENE`, `rhd.bsd` -> `render.hydra.scene`
* Cached images now saved under `BKE_tempdir_session()//hydra_image_cache` folder

Pull Request BogdanNagirniak/blender#27
Bogdan Nagirniak added 1 commit 2023-04-28 08:49:14 +02:00
Bogdan Nagirniak added 1 commit 2023-04-28 11:44:06 +02:00
486b6ea406 Fix wrong path for PXR_MTLX_STDLIB_SEARCH_PATHS
Fix setting `PXR_MTLX_STDLIB_SEARCH_PATHS` depending on the platform.

Pull Request BogdanNagirniak/blender#30
Bogdan Nagirniak added 1 commit 2023-04-28 19:09:31 +02:00
Bogdan Nagirniak added 1 commit 2023-05-03 21:15:56 +02:00
b9cd5b597e Move bpy_hydra.export_mtlx() exception handling to C code
### Purpose
Currently we catch all exceptions in `bpy_hydra.export_mtlx()`, because of memory issues.

### Technical steps
1. Removed `except:` and `traceback.print_exc()` from `bpy_hydra.export_mtlx()`.
2. Applied `PyErr_Fetch()` instead `PyErr_Print()` to correctly free exception data.

Pull Request BogdanNagirniak/blender#28
Bogdan Nagirniak added 1 commit 2023-05-05 14:49:33 +02:00
a95359896d Recreate PreviewEngine when render_delegate_name is changed
### Purpose
Recreate `PreviewEngine` when `render_delegate_name` is changed.

### Technical  steps
Added comparison of `render_delegate_name` and `PreviewEngine` re-creation  on `PreviewEngine::update`
Added/updated some comments.

Pull Request BogdanNagirniak/blender#36
Matt McLin reviewed 2023-05-09 02:22:47 +02:00
@ -0,0 +3,4 @@
#pragma once
#include <pxr/imaging/hd/sceneDelegate.h>
Member

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.

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.
BogdanNagirniak marked this conversation as resolved
Matt McLin reviewed 2023-05-09 02:24:20 +02:00
@ -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__);
Member

The __VAR_ARGS__ is problematic for clang, and so all invocations of ID_LOG fail to compile. I have not yet explored the correct solution, so just turned ID_LOG into a no-op for now.

The `__VAR_ARGS__` is problematic for clang, and so all invocations of `ID_LOG` fail to compile. I have not yet explored the correct solution, so just turned `ID_LOG` into a no-op for now.
BogdanNagirniak marked this conversation as resolved
Matt McLin reviewed 2023-05-09 02:26:32 +02:00
@ -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);
Member

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.

`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.
BogdanNagirniak marked this conversation as resolved
Matt McLin reviewed 2023-05-09 02:28:40 +02:00
@ -0,0 +8,4 @@
#include <string>
struct pxr::HdMaterialNetworkMap;
Member

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>.

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>`.
BogdanNagirniak marked this conversation as resolved
Matt McLin requested changes 2023-05-09 02:30:30 +02:00
Dismissed
Matt McLin left a comment
Member

For now, not commenting on any code other than the minimal changes required to build on macOS with Xcode 14 and clang 14.

For now, not commenting on any code other than the minimal changes required to build on macOS with Xcode 14 and clang 14.
Bogdan Nagirniak added 2 commits 2023-05-12 18:42:37 +02:00

@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.

@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.
Author
Contributor

I've updated PR description:

  • updated current status
  • added TODOs, known issues and known issues for Storm render delegate

Also want to say that all review comments in code are resolved. Please review.

@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.

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've updated PR description: - updated current status - added TODOs, known issues and known issues for Storm render delegate Also want to say that all review comments in code are resolved. **Please review**. > @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. 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.

Sorry for misunderstanding, I didn't mention this, I mentioned only about review comments in code, that they are resolved.

I think Brian mention it before you joined the meeting.

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 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.

> Sorry for misunderstanding, I didn't mention this, I mentioned only about review comments in code, that they are resolved. I think Brian mention it before you joined the meeting. > 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 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.
Brecht Van Lommel reviewed 2023-05-26 20:35:25 +02:00
Brecht Van Lommel left a comment
Owner

Just one comment, I didn't re-review the code yet.

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.

This is still using a timer, it should not be used at all. Other render engines don't need one either to free data.
Author
Contributor

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?

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.

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.
Author
Contributor

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?

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.

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.

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.

> 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.
BogdanNagirniak marked this conversation as resolved
Author
Contributor

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.

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:

  • added multiple UV maps or per-vertex (not corner) color attributes to the TODO's list
  • added to status: Implemented multiple materials on mesh.

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?

We splitted and renamed some function to have matching names, comments with known differences we didn't add, we'll do this.

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.

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?

> 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. 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: - added multiple UV maps or per-vertex (not corner) color attributes to the TODO's list - added to status: Implemented multiple materials on mesh. > 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? We splitted and renamed some function to have matching names, comments with known differences we didn't add, we'll do this. > 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. 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?
Bogdan Nagirniak added 1 commit 2023-05-29 11:45:45 +02:00
03ac728432 Export curves
### Purpose
Implement Curves export.

### Technical steps
New class CurvesData.

### Note for reviewers
Applying any material on Curves causes wrong behavior and crash on RPR side (HdStorm work fine). Will discuss it with RPR team.

Pull Request BogdanNagirniak/blender#46

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:

  • added multiple UV maps or per-vertex (not corner) color attributes to the TODO's list
  • added to status: Implemented multiple materials on mesh.

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.

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 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.

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?

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.

> 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: > - added multiple UV maps or per-vertex (not corner) color attributes to the TODO's list > - added to status: Implemented multiple materials on mesh. 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. > 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 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. > 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? 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 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.

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

.. 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 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. 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
Brecht Van Lommel requested changes 2023-05-31 22:33:10 +02:00
Brecht Van Lommel left a comment
Owner

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.

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 in engine_render_func.

Seems unnecessary since there is already `Py_BEGIN_ALLOW_THREADS` in `engine_render_func`.
BogdanNagirniak marked this conversation as resolved
@ -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 in engine_render_func.

Seems unnecessary since there is already `Py_BEGIN_ALLOW_THREADS` in `engine_render_func`.
BogdanNagirniak marked this conversation as resolved
@ -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 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?
BogdanNagirniak marked this conversation as resolved
@ -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 and BKE_object_visibility should be used instead.

Not quite sure what this is doing, but checking `OB_DUPLI` directly is probably wrong and `BKE_object_visibility` should be used instead.
BogdanNagirniak marked this conversation as resolved
@ -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.

Remove `ret` and just return values directly.
BogdanNagirniak marked this conversation as resolved
@ -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.

This indexes as if the radius is per curve, but it is per curve point.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.

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.
BogdanNagirniak marked this conversation as resolved
@ -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.

true -> false, it does not need to guess a filepath since you are setting it.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.

`OB_CURVES` is missing here.
Author
Contributor

Instancing of OB_CURVES is not supported yet. It'll be implemented soon

Instancing of `OB_CURVES` is not supported yet. It'll be implemented soon
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
BogdanNagirniak marked this conversation as resolved
@ -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).

Use sizeof(name).
BogdanNagirniak marked this conversation as resolved
@ -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.

Generating duplis just for this check looks very expensive and `O(n^2)` time complexity as well.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
Author
Contributor
Fixed in https://projects.blender.org/BogdanNagirniak/blender/pulls/52
BogdanNagirniak marked this conversation as resolved
@ -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.

Should check that what was return is actually a unicode object.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
Author
Contributor

result is nullptr if an error occurred

`result` is `nullptr` if an error occurred
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
BogdanNagirniak marked this conversation as resolved
@ -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.

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.
Author
Contributor

This is in our TODOs list to support more nodes for World including Environment Texture node

This is in our TODOs list to support more nodes for World including Environment Texture node
BogdanNagirniak marked this conversation as resolved
Bogdan Nagirniak added 1 commit 2023-06-02 12:02:51 +02:00
00167e6693 Move to use blender::Map container instead std::unordered_map
### Purpose
Move to use blender::Map container instead std::unordered_map.
Move to use blender::Set container instead std::set.

### Technical steps
Adjusted code according to `BLI_map.hh`, `BLI_set.hh`and `BLI_hash.hh` implementations.
Refactored `InstancerDataMap`, `ObjectDataMap`, `MaterialDataMap`, `mesh_instances_`, `light_instances_`, `available_objects`, and `available_materials`.

Pull Request BogdanNagirniak/blender#47

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 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:

I assume we agree that direct Hydra export is effectively an optimization only. And so this means exporting a USD stage and populating a Hydra scene delegate must give 100% the same result. Not through making two separate implementations that we hope will eventually match, but in a deliberate way so we can verify and maintain this.

With that in mind, the first thing to do should be to get this working with the existing USD export code. That will give you an (unoptimized) baseline to compare to. It also makes code review more manageable, breaking it into smaller steps. I want to avoid reviewing a patch that has both the render engine integration and significant refactoring of the existing USD code.
https://archive.blender.org/developer/D16799

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.

> 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 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: > I assume we agree that direct Hydra export is effectively an optimization only. And so this means exporting a USD stage and populating a Hydra scene delegate must give 100% the same result. Not through making two separate implementations that we hope will eventually match, but in a deliberate way so we can verify and maintain this. > > With that in mind, the first thing to do should be to get this working with the existing USD export code. That will give you an (unoptimized) baseline to compare to. It also makes code review more manageable, breaking it into smaller steps. I want to avoid reviewing a patch that has both the render engine integration and significant refactoring of the existing USD code. https://archive.blender.org/developer/D16799 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.
Bogdan Nagirniak added 2 commits 2023-06-14 16:41:10 +02:00

We 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

We 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?

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?
Author
Contributor

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.

> 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:

  • Final render is broken, but seems it's being fixed in BogdanNagirniak/blender#61
  • Doing a final render a second time crashes. Maybe this is related to the previous point.
  • When selecting an object, the render is blank with only overlays for a moment until you e.g. move an object. Selection changes should not update the Hydra render.
  • On Linux + NVIDIA, I get tons of warnings like this in the console:
Runtime Error: in HgiGLPostPendingGLErrors at line 91 of /src/build_linux/deps/build/usd/src/external_usd/pxr/imaging/hgiGL/diagnostic.cpp -- GL error: invalid enum, reported from pxrInternal_v0_23__pxrReserved__::HgiGL_ScopedStateHolder::~HgiGL_ScopedStateHolder()
A few issue I found testing this with Storm: * Final render is broken, but seems it's being fixed in BogdanNagirniak/blender#61 * Doing a final render a second time crashes. Maybe this is related to the previous point. * When selecting an object, the render is blank with only overlays for a moment until you e.g. move an object. Selection changes should not update the Hydra render. * On Linux + NVIDIA, I get tons of warnings like this in the console: ``` Runtime Error: in HgiGLPostPendingGLErrors at line 91 of /src/build_linux/deps/build/usd/src/external_usd/pxr/imaging/hgiGL/diagnostic.cpp -- GL error: invalid enum, reported from pxrInternal_v0_23__pxrReserved__::HgiGL_ScopedStateHolder::~HgiGL_ScopedStateHolder() ```

Thanks, I submitted a few pull requests now.

Thanks, I submitted a few pull requests now.
Author
Contributor

Thank you!

On Linux + NVIDIA, I get tons of warnings like this in the console

Currently I've been working and investigating this issues. This is happen because of using GL_POINT_SMOOTH in
https://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? In usdview tool there is no such error

When selecting an object, the render is blank with only overlays for a moment until you e.g. move an object. Selection changes should not update the Hydra render

This is next my task for investigation.

Thank you! > On Linux + NVIDIA, I get tons of warnings like this in the console Currently I've been working and investigating this issues. This is happen because of using `GL_POINT_SMOOTH` in https://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? In `usdview` tool there is no such error > When selecting an object, the render is blank with only overlays for a moment until you e.g. move an object. Selection changes should not update the Hydra render This is next my task for investigation.

Currently I've been working and investigating this issues. This is happen because of using GL_POINT_SMOOTH in
https://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? In usdview tool there is no such error

Blender uses the OpenGL core profile, and GL_POINT_SMOOTH is not part of that.

> Currently I've been working and investigating this issues. This is happen because of using `GL_POINT_SMOOTH` in > https://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? In `usdview` tool there is no such error Blender uses the OpenGL core profile, and GL_POINT_SMOOTH is not part of that.
Author
Contributor

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:

When selecting an object, the render is blank with only overlays for a moment until you e.g. move an object

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: > When selecting an object, the render is blank with only overlays for a moment until you e.g. move an object
Author
Contributor

Selection changes should not update the Hydra render.

This is already works in this way

> Selection changes should not update the Hydra render. This is already works in this way
Brecht Van Lommel added 1 commit 2023-07-12 17:29:23 +02:00
Bogdan Nagirniak added 1 commit 2023-07-12 23:18:08 +02:00
bd3f747044 Fix rendering of Final render for Storm delegate
### Purpose
Final render for Storm delegate returns empty image.

### Technical steps
1. Added comment in `bpy_hydra.py` that if `bl_use_gpu_context` is enabled then `update()` should be empty and engine cretaion and syncing has to moved to `render()`. Corresponded PR for Storm Hydra addon BogdanNagirniak/blender-addons#17.
2. Renamed `FinalEngineGL -> FinalEngineGPU` as more related.
3. Fixed `FinalEngineGPU::render()` to works with Storm delegate.
4. Fixed CullStyle for mesh and material.
5. Improved logging.

Pull Request BogdanNagirniak/blender#61
Bogdan Nagirniak added 1 commit 2023-07-13 01:15:10 +02:00
Brecht Van Lommel added 1 commit 2023-07-13 20:18:50 +02:00
ee62b0b320 Add automated tests for Storm, with both Hydra and USD export methods
Needs WITH_OPENGL_RENDER_TESTS CMake option to be enabled.

Pull Request #66
Brecht Van Lommel added 1 commit 2023-07-14 19:41:23 +02:00
85470c770f Add scene setting to export through either Hydra or USD, for debugging
The Hydra Storm add-on already had a setting to use a USD file, with
associated sync_usd API function. But this way works for all Hydra
engines and doesn't require manually exporting a USD file each time.

Pull Request #65
Brecht Van Lommel added 11 commits 2023-07-27 15:06:02 +02:00
83219387fd Refactor: move Hydra scene delegate to io/usd module
To make it more clear that these are closely related and should stay in
sync as much as possible.

Also
* Rename BlenderSceneDelegate to HydraSceneDelegate, to go along with
  with USDSceneDelegate.
* Move MaterialX code into material.cc.

Pull Request: BogdanNagirniak/blender#75
f9a7410305 Change license to GPL v2
This code is part of Blender itself, and we want to be able to freely
copy and paste code from other parts of Blender.
5d537092c1 Make update happen at appropriate time, by binding GPU context
Otherwise the UI draw lock functionality does not work. Also renames
engine_sync to engine_update for consistency.
6b07d1b251 Initialize PXR_MTLX_STDLIB_SEARCH_PATHS in same place as MATERIALX_SEARCH_PATH
More consistent, and avoids paths being added multiple times if multiple
hydra render engines are registered.
0c9d643b2d Move HydraRenderEngine to bpy.types, update Python API docs
* Makes it more consistent with other types that can be subclassed.
* Give bl_ prefix to delegate_id.
* Lazily import _bpy_hydra.
* Tweaks to API docs example.

Pull Request: BogdanNagirniak/blender#77
buildbot/vexp-code-patch-coordinator Build done. Details
12b40425a3
Refactor: remove need to use .as_pointer() for _bpy_hydra
And remove some unnecessary includes.

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:

  • Mesh export should be aligned to export the same attributes.
  • Camera code should share more code with other parts of Blender, using the utility functions in 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.
  • Material export should be unified, so both USD and Hydra can use either preview surface or MaterialX.
  • Image caching doesn't integrate well with updates, partially because they are not part of the depsgraph and so it's so easy to track changes. Further there are multi layer images, stereo, color spaces and various other things that are not handled carefully.
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: * Mesh export should be aligned to export the same attributes. * Camera code should share more code with other parts of Blender, using the utility functions in `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. * Material export should be unified, so both USD and Hydra can use either preview surface or MaterialX. * Image caching doesn't integrate well with updates, partially because they are not part of the depsgraph and so it's so easy to track changes. Further there are multi layer images, stereo, color spaces and various other things that are not handled carefully.

@blender-bot build

@blender-bot build
Brecht Van Lommel added 1 commit 2023-07-27 20:22:55 +02:00

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.

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.
Bogdan Nagirniak added 1 commit 2023-07-29 11:45:11 +02:00
eca3438772 Render fixes after refactoring
### Purpose
After refactoring PRs there are render issues such as crash with RPR delegate in viewport.

### Technical steps
1. Moved binding of framebuffer and VAO to `GPURenderTaskDelegate` and use it in Viewport render.
2. Made more correct viewport render for Storm delegate by rendering it to separate framebuffer.

Pull Request BogdanNagirniak/blender#79
Bogdan Nagirniak added 1 commit 2023-08-01 21:35:33 +02:00
debaa39dfe Hydra code cleanup
### Purpose
After code refactor in #75 there is good to adjust logging + do code polishing and improvement.

### Technical steps
1. Renamed logger `LOG_RENDER_HYDRA_SCENE -> LOG_HYDRA_SCENE` as "hydra.scene".
2. Renamed logger `LOG_RENDER_HYDRA -> LOG_HYDRA_RENDER` as "hydra.render".
3. Simplified `Engine` classes by adding `depsgraph_`, `context_` and `scene_` to class fields. Unified structure of `Engine` classes.
4. Moved AOV tokens map to `FinalEngine` as more related.
5. Made other code cleanups and improvements.

Pull Request BogdanNagirniak/blender#81
Bogdan Nagirniak added 2 commits 2023-08-02 23:42:09 +02:00
Author
Contributor

@blender-bot build

@blender-bot build
Member

Only blender organization members with write access can start builds. See documentation for details.

Only blender organization members with write access can start builds. See [documentation](https://projects.blender.org/infrastructure/blender-bot/src/branch/main/README.md) for details.
Author
Contributor

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?

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?
Member

@blender-bot build

@blender-bot build

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR104712) when ready.
Brecht Van Lommel added 3 commits 2023-08-03 16:14:27 +02:00
77b60d04e9 Tests: use separate directories for USD/Hydra references renders
Not yet at a stage where these can be the same.
buildbot/vexp-code-patch-coordinator Build done. Details
0675bd324c
Remove MaterialX and set_sync_settings
The implementation of this is not ready yet, and we'll merge initial support
without it.
Brecht Van Lommel changed title from WIP: Hydra render engine to Hydra render engine 2023-08-03 16:14:46 +02:00
Brecht Van Lommel approved these changes 2023-08-03 16:15:14 +02:00

I plan to merge this tomorrow.

I removed the MaterialX changes, you might want to put those in a new branch once this is merged.

@blender-bot build

I plan to merge this tomorrow. I removed the MaterialX changes, you might want to put those in a new branch once this is merged. @blender-bot build
Brecht Van Lommel dismissed Matt-McLin’s review 2023-08-03 16:44:54 +02:00
Reason:

Comments were addressed

Brecht Van Lommel added 2 commits 2023-08-04 15:09:24 +02:00

@blender-bot build

@blender-bot build

This now landed in main in a few separate commits. Thanks to everyone who helped make this possible.

For the remaining work, there is #110765. If I forgot anything or you are planning more things, please add them there.

This now landed in main in a few separate commits. Thanks to everyone who helped make this possible. For the remaining work, there is #110765. If I forgot anything or you are planning more things, please add them there.
Bastien Montagne removed this from the USD project 2023-08-25 23:52:09 +02:00
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.

Pull request closed

Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
7 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#104712
No description provided.