Core: Add ObjectRuntime::last_update transform, geometry and shading #115196

Miguel Pozo changed title from ~~WIP: Core: Add ObjectRuntime::last_update transform, geometry and shading~~ to Core: Add ObjectRuntime::last_update transform, geometry and shading

2023-11-21 17:00:51 +01:00

Miguel Pozo referenced this pull request

2023-11-21 17:06:36 +01:00

EEVEE-Next: Fix ObjectHandle::recalc #115243

Miguel Pozo added a new dependency 2023-11-21 17:08:18 +01:00

#115243 EEVEE-Next: Fix ObjectHandle::recalc

Sergey Sharybin requested changes 2023-11-21 17:49:03 +01:00

Sergey Sharybin left a comment

Materials can be either on object or geometry level. So to cover all cases build_materials should receive an ID of the "owner" of the materials, and the links are to to be added in build_material. Also, a link from geometry's SHADING to object SHADING would need to be added (unless it is already happening).

Materials can be either on object or geometry level. So to cover all cases `build_materials` should receive an `ID` of the "owner" of the materials, and the links are to to be added in `build_material`. Also, a link from geometry's SHADING to object SHADING would need to be added (unless it is already happening).

source/blender/depsgraph/intern/depsgraph_eval.cc Outdated

						
				@ -35,6 +35,7 @@ namespace deg = blender::deg;

				static void deg_flush_updates_and_refresh(deg::Depsgraph *deg_graph)

				{

				  deg_graph->update_count++;

Sergey Sharybin commented

2023-11-21 17:48:58 +01:00

Wouldn't it be better (or even more correct) to do this in the deg_evaluate_on_refresh after the check for whether there are actual update tags?

Wouldn't it be better (or even more correct) to do this in the `deg_evaluate_on_refresh` after the check for whether there are actual update tags?

pragma37 marked this conversation as resolved

Miguel Pozo added 2 commits 2023-11-22 18:16:24 +01:00

9c158a3fd3 Move update_count++ to deg::deg_evaluate_on_refresh

c20264e62f Move shading relation to build_material

Miguel Pozo commented

2023-11-22 18:23:43 +01:00

I couldn't figure out how to get the Geometry Shading -> Object Shading to work without adding a dummy operation_node callback for geometry shading.
Is that the intended way to do it?

Also, something not directly related to this patch, but I've noticed that build_object_data passes the geometry-owned materials to build_materials, while build_object_data_geometry passes the object-owned materials.
I think it doesn't matter in practice, but shouldn't be the other way around?

I couldn't figure out how to get the `Geometry Shading -> Object Shading` to work without adding a dummy `operation_node` callback for geometry shading. Is that the intended way to do it? Also, something not directly related to this patch, but I've noticed that `build_object_data` passes the geometry-owned materials to `build_materials`, while `build_object_data_geometry` passes the object-owned materials. I think it doesn't matter in practice, but shouldn't be the other way around?

Miguel Pozo requested review from Sergey Sharybin 2023-11-24 15:37:09 +01:00

Sergey Sharybin commented

2023-11-24 16:19:09 +01:00

I couldn't figure out how to get the Geometry Shading -> Object Shading to work without adding a dummy operation_node callback for geometry shading.
Is that the intended way to do it?

You indeed need to create operation node, but instead of providing no-op callback you should omit the callback completely. This will give the graph possibility to do some internal optimizations. Something like this should work: add_operation_node((ID *)object->data, NodeType::SHADING, OperationCode::SHADING);

Also, something not directly related to this patch, but I've noticed that build_object_data passes the geometry-owned materials to build_materials, while build_object_data_geometry passes the object-owned materials.
I think it doesn't matter in practice, but shouldn't be the other way around?

The naming seems to be confusing and is to be updated. The build_object_data_geometry builds nodes needed for the object geometry component. While it mentions "data" in the name this seems wrong, as all operations happens on the object level there. The build_object_data builds object->data. So i think what is passed to the build_materials is correct, but indeed it will be nice to have a pass on the naming clarification.

> I couldn't figure out how to get the Geometry Shading -> Object Shading to work without adding a dummy operation_node callback for geometry shading. > Is that the intended way to do it? You indeed need to create operation node, but instead of providing no-op callback you should omit the callback completely. This will give the graph possibility to do some internal optimizations. Something like this should work: `add_operation_node((ID *)object->data, NodeType::SHADING, OperationCode::SHADING);` > Also, something not directly related to this patch, but I've noticed that `build_object_data` passes the geometry-owned materials to `build_materials`, while `build_object_data_geometry` passes the object-owned materials. > I think it doesn't matter in practice, but shouldn't be the other way around? The naming seems to be confusing and is to be updated. The `build_object_data_geometry` builds nodes needed for the object geometry component. While it mentions "data" in the name this seems wrong, as all operations happens on the object level there. The `build_object_data` builds `object->data`. So i think what is passed to the `build_materials` is correct, but indeed it will be nice to have a pass on the naming clarification.

👍 1

Miguel Pozo added 2 commits 2023-11-24 16:44:49 +01:00

cd7b7ec31c Cleanup: Remove no-op callback

buildbot/vexp-code-patch-coordinator Build done. Details

76f535e80f Cleanup: Rename DEG_update_count to DEG_get_update_count

Sergey Sharybin commented

2023-11-24 16:46:45 +01:00

@blender-bot build

Sergey Sharybin approved these changes 2023-11-27 11:42:42 +01:00

Sergey Sharybin left a comment

Tested with Pets animation file, and did not see any issues with the relations or impact on the performance.

🎉 1

Miguel Pozo referenced this issue from a commit

2023-11-27 12:03:27 +01:00

Core: Add ObjectRuntime::last_update transform, geometry and shading

Miguel Pozo merged commit 78de9138e5 into main

2023-11-27 12:03:27 +01:00

Miguel Pozo deleted branch pull-recalc-last_updated

2023-11-27 12:03:29 +01:00

Hans Goudey referenced this issue from a commit

2023-11-27 18:01:11 +01:00

Core: Add ObjectRuntime::last_update transform, geometry and shading

Som1Lse referenced this issue from a commit

2023-11-27 18:49:10 +01:00

Core: Add ObjectRuntime::last_update transform, geometry and shading

Bastien Montagne commented

2023-12-01 12:38:13 +01:00

This commit (78de9138e5) broke 18 unittests in Debug builds!

add_operation: Operation already exists - [Shading Component] : (affects_visible_id: false) has SHADING() at 0x612000823278
BLI_assert failed: source/blender/depsgraph/intern/builder/deg_builder_nodes.cc:261, add_operation_node(), at '0'
  Should not happen!

Please build and run debug before committing changes!

This commit (78de9138e593ee0c967e132c45d7c6d5e66e9e96) broke 18 unittests in Debug builds! ``` add_operation: Operation already exists - [Shading Component] : (affects_visible_id: false) has SHADING() at 0x612000823278 BLI_assert failed: source/blender/depsgraph/intern/builder/deg_builder_nodes.cc:261, add_operation_node(), at '0' Should not happen! ``` Please build and run debug before committing changes!

Bastien Montagne commented

2023-12-01 14:36:08 +01:00

Looks like this is already handled in PR actually: !115615

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Core: Add ObjectRuntime::last_update transform, geometry and shading #115196