Geometry Nodes drop in performance when adding an output attribute #119938

New Issue

Closed

opened 2024-03-26 23:29:13 +01:00 by Monaime · 4 comments

Monaime commented

2024-03-26 23:29:13 +01:00

System Information
Operating system: Windows 10 Pro
Graphics card: NVIDIA GeForce GTX 1080

Blender Version
Broken: Blender 4.1.0 stable
Worked: Blender 4.1.0 Beta ced066e0c8

Short description of error
In Geometry Nodes there is a drop in performance when adding an output attribute to instances, the instances are comming from a collection (storing a named attribute seems to work fine).

Exact steps for others to reproduce the error

Open the attached blend file in blender 4.1.0 stable.
Play the animation.
Remove the output attribute "Attr" and notice the change in the fps.
Open the same file in Blender 4.0 where adding or removing an output attribute does not affect the fps greatly.

**System Information** Operating system: Windows 10 Pro Graphics card: NVIDIA GeForce GTX 1080 **Blender Version** Broken: Blender 4.1.0 stable Worked: Blender 4.1.0 Beta ced066e0c8db **Short description of error** In Geometry Nodes there is a drop in performance when adding an output attribute to instances, the instances are comming from a collection (storing a named attribute seems to work fine). **Exact steps for others to reproduce the error** - Open the attached blend file in blender 4.1.0 stable. - Play the animation. - Remove the output attribute "Attr" and notice the change in the fps. - Open the same file in Blender 4.0 where adding or removing an output attribute does not affect the fps greatly.

bug_report.blend

7.4 MiB

Monaime added the

labels 2024-03-26 23:29:14 +01:00

Pratik Borhade commented

2024-03-27 06:41:43 +01:00

Member

Hi, thanks for the report. Can confirm the regression.

Hi, thanks for the report. Can confirm the regression.

Pratik Borhade added

Nodes & Physics

and removed

labels 2024-03-27 06:42:02 +01:00

YimingWu commented

2024-03-27 07:40:52 +01:00

Member

Can confirm, it does also affect 4.2.

While it is this way, 4.1.0 rc will also run faster than 4.2 even when removed that attribute socket. So there must be other changes

Can confirm, it does also affect 4.2. While it is this way, 4.1.0 rc will also run faster than 4.2 even when removed that attribute socket. So there must be other changes

Pratik Borhade commented

2024-03-27 08:18:22 +01:00

Member

Bisecting points to b020173984
@JacquesLucke ^

Bisecting points to b020173984e4292561a528c5a9eb2a8bffe0122e @JacquesLucke ^

Jacques Lucke commented

2024-03-27 11:21:07 +01:00

Member

Was looking into this a bit now.

The short term fix is to add a special case to store_output_attributes to handle the case when all output attributes are on the instance domain.

I think it may also be possible to fix this at a deeper level which involves changes to the modifier evaluation system on meshes (possibly simplifications overall though). The regression basically happened because now the evaluated instanced meshes are new meshes on every frame, instead of the same mesh all the time (note that each cube in the file is a separate mesh data block, they are not instances of the same mesh). Therefor, the drawing cache has to be recreated for every mesh on every frame. Previously, the drawing cache was only created once per mesh.

This happens for a combination of reasons, but at the core, the mesh copies are created in modifier_modify_mesh_and_geometry_set by calling ensure_owns_direct_data. A copy is made, because the cube meshes are currently not owned but GeometryOwnershipType::Editable/ReadOnly. This is because the evaluated mesh ownership is not using GeometrySet yet (see comments in mesh_build_data).

I believe the situation can be improved by changing mesh object evaluation like so:

Make the object.runtime.geometry_set_eval the owner of the evaluated mesh (this likely has some other implicitions that need to be worked out).
For the case of having multiple objects that share the some mesh (linked duplicates), we can change it so that evaluated MeshComponent is shared between the evaluated objects, instead of sharing the Mesh itself in case there are no modifiers. This may be a prerequisite for the change mentioned above.

This hack also fixes the regression (but breaks other stuff):

diff --git a/source/blender/blenkernel/intern/DerivedMesh.cc b/source/blender/blenkernel/intern/DerivedMesh.cc
index 55fda2dcba8..73a6400dc17 100644
--- a/source/blender/blenkernel/intern/DerivedMesh.cc
+++ b/source/blender/blenkernel/intern/DerivedMesh.cc
@@ -1302,7 +1302,7 @@ static void mesh_build_data(Depsgraph *depsgraph,
                       need_mapping,
                       dataMask,
                       true,
-                      true,
+                      false,
                       &mesh_deform_eval,
                       &mesh_eval,
                       &geometry_set_eval);
@@ -1315,11 +1315,11 @@ static void mesh_build_data(Depsgraph *depsgraph,
    * the final result might be freed prior to object). */
   Mesh *mesh = (Mesh *)ob->data;
   const bool is_mesh_eval_owned = (mesh_eval != mesh->runtime->mesh_eval);
-  BKE_object_eval_assign_data(ob, &mesh_eval->id, is_mesh_eval_owned);
+  BKE_object_eval_assign_data(ob, &mesh_eval->id, false);
 
   /* Add the final mesh as a non-owning component to the geometry set. */
   MeshComponent &mesh_component = geometry_set_eval->get_component_for_write<MeshComponent>();
-  mesh_component.replace(mesh_eval, GeometryOwnershipType::Editable);
+  mesh_component.replace(mesh_eval, GeometryOwnershipType::Owned);
   ob->runtime->geometry_set_eval = geometry_set_eval;
 
   ob->runtime->mesh_deform_eval = mesh_deform_eval;

Was looking into this a bit now. The short term fix is to add a special case to `store_output_attributes` to handle the case when all output attributes are on the instance domain. I think it may also be possible to fix this at a deeper level which involves changes to the modifier evaluation system on meshes (possibly simplifications overall though). The regression basically happened because now the evaluated instanced meshes are new meshes on every frame, instead of the same mesh all the time (note that each cube in the file is a separate mesh data block, they are not instances of the same mesh). Therefor, the drawing cache has to be recreated for every mesh on every frame. Previously, the drawing cache was only created once per mesh. This happens for a combination of reasons, but at the core, the mesh copies are created in `modifier_modify_mesh_and_geometry_set` by calling `ensure_owns_direct_data`. A copy is made, because the cube meshes are currently not owned but `GeometryOwnershipType::Editable/ReadOnly`. This is because the evaluated mesh ownership is not using `GeometrySet` yet (see comments in `mesh_build_data`). I believe the situation can be improved by changing mesh object evaluation like so: * Make the `object.runtime.geometry_set_eval` the owner of the evaluated mesh (this likely has some other implicitions that need to be worked out). * For the case of having multiple objects that share the some mesh (linked duplicates), we can change it so that evaluated `MeshComponent` is shared between the evaluated objects, instead of sharing the `Mesh` itself in case there are no modifiers. This may be a prerequisite for the change mentioned above. ----- This hack also fixes the regression (but breaks other stuff): ```diff diff --git a/source/blender/blenkernel/intern/DerivedMesh.cc b/source/blender/blenkernel/intern/DerivedMesh.cc index 55fda2dcba8..73a6400dc17 100644 --- a/source/blender/blenkernel/intern/DerivedMesh.cc +++ b/source/blender/blenkernel/intern/DerivedMesh.cc @@ -1302,7 +1302,7 @@ static void mesh_build_data(Depsgraph *depsgraph, need_mapping, dataMask, true, - true, + false, &mesh_deform_eval, &mesh_eval, &geometry_set_eval); @@ -1315,11 +1315,11 @@ static void mesh_build_data(Depsgraph *depsgraph, * the final result might be freed prior to object). */ Mesh *mesh = (Mesh *)ob->data; const bool is_mesh_eval_owned = (mesh_eval != mesh->runtime->mesh_eval); - BKE_object_eval_assign_data(ob, &mesh_eval->id, is_mesh_eval_owned); + BKE_object_eval_assign_data(ob, &mesh_eval->id, false); /* Add the final mesh as a non-owning component to the geometry set. */ MeshComponent &mesh_component = geometry_set_eval->get_component_for_write<MeshComponent>(); - mesh_component.replace(mesh_eval, GeometryOwnershipType::Editable); + mesh_component.replace(mesh_eval, GeometryOwnershipType::Owned); ob->runtime->geometry_set_eval = geometry_set_eval; ob->runtime->mesh_deform_eval = mesh_deform_eval; ```

Hans Goudey referenced a pull request that will close this issue

2024-03-27 16:47:55 +01:00

WIP: Mesh: Improve and simplify modifier evaluation logic #119968

Jacques Lucke referenced a pull request that will close this issue

2024-04-02 17:15:34 +02:00

Fix #119938: performance regression when adding output attribute #120177

Jacques Lucke referenced this issue from a commit

2024-04-02 17:48:16 +02:00

Fix #119938: performance regression when adding output attribute

Jacques Lucke closed this issue

2024-04-02 17:48:18 +02:00

Jacques Lucke referenced this issue

2024-04-02 17:49:49 +02:00

4.1: Candidates for corrective releases #119631

Blender Bot added

and removed

labels 2024-04-02 17:51:39 +02:00

Thomas Dinges referenced this issue from a commit

2024-04-09 16:11:47 +02:00

Fix #119938: performance regression when adding output attribute

Hans Goudey referenced this issue

2024-04-15 18:20:20 +02:00

Mesh: Store BMEditMesh in shared pointer #120276

Sign in to join this conversation.

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

4 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#119938

No description provided.