WIP: Geometry Nodes: Port triangulate node from BMesh to Mesh #112264

Draft
Hans Goudey wants to merge 182 commits from HooglyBoogly/blender:mesh-triangulate into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Member

Add a Mesh implementation of triangulation, which is currently just
implemented for BMesh. The main benefit of this is performance and
decreased memory usage. The benefit is especially large because the
node currently converts to BMesh, triangulates, and then converts back.
But the BMesh implementation is entirely single threaded, so it will
always be much slower.

The new implementation uses the principle of "never just process a
single element at a time" but it also tries to avoid processing too
many elements at once, to decrease the size of temporary buffers.
Practically that means the work is organized into chunks of selected
faces, but within each chunk, each task is done in a separate loop.
Arguably I went a bit far with some optimizations, and some of the
complexity isn't necessary, but I hope everything is clear anyway.

Unlike some other Mesh ports like the extrude node or the split edges
code, this generates a new mesh. I still go back and forth on that
aspect, but reusing the same mesh would have required reallocating
face attributes from scratch anyway. Implicit sharing is used to
avoid copying vertex attributes though.

The result mesh is reorganized a bit. Unselected face data comes first,
then selected triangles, then triangulated NGons, then triangulated
quads. This makes attribute interpolation and internal calculations
more efficient.

The "Minimum Vertices" socket is replaced with versioning. In the new
implementation it would have an impact on code complexity, and for a
builtin "atomic" node it makes more sense as part of the selection.

The performance difference depends on the number of CPU threads, the
number of attributes, and the selection size. As all of those numbers
go up, the benefit will grow. The "modes" also affect the performance,
obviously.

With a Ryzen 7950x, I tested performance in a few situations (runtime in ms):

Selection Before After Change
1.4 m quads (fixed) 50% 1533 15.9 96x
10 m quads (shortest) 100% 9700 165.0 59x
1 m 10-side Ngons (beauty) 90% 3785 115.9 33x
1 m quads many attributes 100% 54600 332.3 164x

In follow-up commits, I'll move other areas to use mesh triangulation
instead of BMesh. This is the last geometry node using BMesh besides
the Ico Sphere primitive.

TODO

Resolve face deduplication

New triangles can be created that are the same as triangles that already exist.
They need to be deduplicated or otherwise not created. This work can be skipped
when the input mesh doesn't have any triangles. Ideas:

  • Hash all input triangle vertices, check hashes when creating a new triangle
  • Use topology caches to only check triangles in neighborhood
  • Add a mesh cache for meshes that don't have overlapping faces (how to generalize?)

Add edge deduplication

Add a topology cache to meshes that allows us to skip this.


Test for the versioning code:
image

Three new regression tests are attached below.

Add a Mesh implementation of triangulation, which is currently just implemented for BMesh. The main benefit of this is performance and decreased memory usage. The benefit is especially large because the node currently converts to BMesh, triangulates, and then converts back. But the BMesh implementation is entirely single threaded, so it will always be much slower. The new implementation uses the principle of "never just process a single element at a time" but it also tries to avoid processing _too_ many elements at once, to decrease the size of temporary buffers. Practically that means the work is organized into chunks of selected faces, but within each chunk, each task is done in a separate loop. Arguably I went a bit far with some optimizations, and some of the complexity isn't necessary, but I hope everything is clear anyway. Unlike some other Mesh ports like the extrude node or the split edges code, this generates a new mesh. I still go back and forth on that aspect, but reusing the same mesh would have required reallocating face attributes from scratch anyway. Implicit sharing is used to avoid copying vertex attributes though. The result mesh is reorganized a bit. Unselected face data comes first, then selected triangles, then triangulated NGons, then triangulated quads. This makes attribute interpolation and internal calculations more efficient. The "Minimum Vertices" socket is replaced with versioning. In the new implementation it would have an impact on code complexity, and for a builtin "atomic" node it makes more sense as part of the selection. The performance difference depends on the number of CPU threads, the number of attributes, and the selection size. As all of those numbers go up, the benefit will grow. The "modes" also affect the performance, obviously. With a Ryzen 7950x, I tested performance in a few situations (runtime in ms): | | Selection | Before | After | Change | | -------------------------- | --------- | ------ | ----- | ------ | | 1.4 m quads (fixed) | 50% | 1533 | 15.9 | 96x | | 10 m quads (shortest) | 100% | 9700 | 165.0 | 59x | | 1 m 10-side Ngons (beauty) | 90% | 3785 | 115.9 | 33x | | 1 m quads many attributes | 100% | 54600 | 332.3 | 164x | In follow-up commits, I'll move other areas to use mesh triangulation instead of BMesh. This is the last geometry node using BMesh besides the Ico Sphere primitive. ## TODO **Resolve face deduplication** New triangles can be created that are the same as triangles that already exist. They need to be deduplicated or otherwise not created. This work can be skipped when the input mesh doesn't have any triangles. Ideas: - Hash all input triangle vertices, check hashes when creating a new triangle - Use topology caches to only check triangles in neighborhood - Add a mesh cache for meshes that don't have overlapping faces (how to generalize?) **Add edge deduplication** Add a topology cache to meshes that allows us to skip this. --- Test for the versioning code: ![image](/attachments/571f3662-7827-4fb5-b6d1-dc461b561cd6) Three new regression tests are attached below.
Hans Goudey added 23 commits 2023-09-12 04:29:32 +02:00
Hans Goudey requested review from Jacques Lucke 2023-09-12 04:31:58 +02:00
Hans Goudey requested review from Wannes Malfait 2023-09-12 04:31:59 +02:00
Hans Goudey added this to the Nodes & Physics project 2023-09-12 04:32:31 +02:00
Hans Goudey added 1 commit 2023-09-12 04:42:13 +02:00
Falk David reviewed 2023-09-12 12:01:00 +02:00
Falk David left a comment
Member

Was intrigued by the PR and left some comments :) Nice improvement!

Was intrigued by the PR and left some comments :) Nice improvement!
@ -177,0 +181,4 @@
const float v4[3],
const bool lock_degenerate)
{
/* not a loop (only to be able to break out) */
Member

Am I missing something here, or could you just return FLT_MAX instead of the breaks and the loop? And since at the end of the loop body, it returns anyway, the final return is no longer needed.

Am I missing something here, or could you just `return FLT_MAX` instead of the `break`s and the loop? And since at the end of the loop body, it returns anyway, the final return is no longer needed.
Author
Member

Yeah, I have no clue. This code is moved from the BMesh module so it can be shared. I'd rather not change it in this commit though, since I'm just moving it.

Yeah, I have no clue. This code is moved from the BMesh module so it can be shared. I'd rather not change it in this commit though, since I'm just moving it.
Member

Makes sense if you're just moving it.

Makes sense if you're just moving it.
@ -0,0 +300,4 @@
const OffsetIndices<int> faces,
const int edges_start,
const int corner_map_offset,
MutableSpan<int> corner_map,
Member

These should probably prefixed with r_

These should probably prefixed with `r_`
@ -0,0 +476,4 @@
});
}
std::optional<Mesh *> mesh_triangulate(
Member

Why return an optional pointer, when you can just return nullptr?

Why return an optional pointer, when you can just return `nullptr`?
Author
Member

A couple other mesh operations do this, since they sometimes need to differentiate between the two. For example, "copy mesh selection" uses it to return a "no changes to input mesh". optional isn't necessary here I suppose, I can remove it.

A couple other mesh operations do this, since they sometimes need to differentiate between the two. For example, "copy mesh selection" uses it to return a "no changes to input mesh". `optional` isn't necessary here I suppose, I can remove it.
Member

Either way, there needs to be some comment for what the return value means. The optional doesn't help me here, definitely not on its own.

Either way, there needs to be some comment for what the return value means. The `optional` doesn't help me here, definitely not on its own.
HooglyBoogly marked this conversation as resolved
Member

I started with some testing, and noticed that beauty mode gave different results:
The two triangulations are different

I don't think that this change of output is "expected".

The reason the node looks slightly different is that for easier testing I added some code to be able to access both the old and new version of the node:

diff --git a/source/blender/nodes/geometry/nodes/node_geo_triangulate.cc b/source/blender/nodes/geometry
index b08ea4574be..87d71252f1f 100644
--- a/source/blender/nodes/geometry/nodes/node_geo_triangulate.cc
+++ b/source/blender/nodes/geometry/nodes/node_geo_triangulate.cc
@@ -2,12 +2,18 @@
  *
  * SPDX-License-Identifier: GPL-2.0-or-later */

+#include "BKE_customdata.h"
 #include "BKE_mesh.hh"

 #include "NOD_rna_define.hh"

 #include "GEO_mesh_triangulate.hh"
+#include "NOD_rna_define.hh"
+
+#include "bmesh.h"
+#include "bmesh_tools.h"

+#include "DNA_mesh_types.h"
 #include "UI_interface.hh"
 #include "UI_resources.hh"

@@ -18,6 +24,7 @@ namespace blender::nodes::node_geo_triangulate_cc {
 static void node_declare(NodeDeclarationBuilder &b)
 {
   b.add_input<decl::Geometry>("Mesh").supported_type(GeometryComponent::Type::Mesh);
+  b.add_input<decl::Bool>("Old Version").default_value(false);
   b.add_input<decl::Bool>("Selection").default_value(true).field_on_all().hide_value();
   b.add_output<decl::Geometry>("Mesh").propagate_all();
 }
@@ -34,9 +41,41 @@ static void geo_triangulate_init(bNodeTree * /*tree*/, bNode *node)
   node->custom2 = GEO_NODE_TRIANGULATE_NGON_BEAUTY;
 }

+static Mesh *triangulate_mesh_selection(const Mesh &mesh,
+                                        const int quad_method,
+                                        const int ngon_method,
+                                        const IndexMask &selection,
+                                        const int min_vertices)
+{
+  CustomData_MeshMasks cd_mask_extra = {
+      CD_MASK_ORIGINDEX, CD_MASK_ORIGINDEX, 0, CD_MASK_ORIGINDEX};
+  BMeshCreateParams create_params{false};
+  BMeshFromMeshParams from_mesh_params{};
+  from_mesh_params.calc_face_normal = true;
+  from_mesh_params.calc_vert_normal = true;
+  from_mesh_params.cd_mask_extra = cd_mask_extra;
+  BMesh *bm = BKE_mesh_to_bmesh_ex(&mesh, &create_params, &from_mesh_params);
+
+  /* Tag faces to be triangulated from the selection mask. */
+  BM_mesh_elem_table_ensure(bm, BM_FACE);
+  selection.foreach_index([&](const int i_face) {
+    BM_elem_flag_set(BM_face_at_index(bm, i_face), BM_ELEM_TAG, true);
+  });
+
+  BM_mesh_triangulate(bm, quad_method, ngon_method, min_vertices, true, nullptr, nullptr, nullptr);
+  Mesh *result = BKE_mesh_from_bmesh_for_eval_nomain(bm, &cd_mask_extra, &mesh);
+  BM_mesh_free(bm);
+
+  /* Positions are not changed by the triangulation operation, so the bounds are the same. */
+  result->runtime->bounds_cache = mesh.runtime->bounds_cache;
+
+  return result;
+}
+
 static void node_geo_exec(GeoNodeExecParams params)
 {
   GeometrySet geometry_set = params.extract_input<GeometrySet>("Mesh");
+  const bool use_old = params.extract_input<bool>("Old Version");
   Field<bool> selection_field = params.extract_input<Field<bool>>("Selection");
   const AnonymousAttributePropagationInfo &propagation_info = params.get_output_propagation_info(
       "Mesh");
@@ -59,14 +98,23 @@ static void node_geo_exec(GeoNodeExecParams params)
       return;
     }

-    std::optional<Mesh *> mesh = geometry::mesh_triangulate(
-        *src_mesh,
-        selection,
-        geometry::TriangulateNGonMode(ngon_method),
-        geometry::TriangulateQuadMode(quad_method),
-        propagation_info);
-    if (mesh) {
-      geometry_set.replace_mesh(*mesh);
+    if (!use_old) {
+      std::optional<Mesh *> mesh = geometry::mesh_triangulate(
+          *src_mesh,
+          selection,
+          geometry::TriangulateNGonMode(ngon_method),
+          geometry::TriangulateQuadMode(quad_method),
+          propagation_info);
+      if (mesh) {
+        geometry_set.replace_mesh(*mesh);
+      }
+    }
+    else {
+      Mesh *mesh_out = triangulate_mesh_selection(
+          *src_mesh, quad_method, ngon_method, selection, 4);
+      if (mesh_out) {
+        geometry_set.replace_mesh(mesh_out);
+      }
     }
   });
I started with some testing, and noticed that beauty mode gave different results: ![The two triangulations are different](/attachments/fecfcc27-e7cf-41a4-9e89-e5a78c2b669c) I don't think that this change of output is "expected". The reason the node looks slightly different is that for easier testing I added some code to be able to access both the old and new version of the node: ```diff diff --git a/source/blender/nodes/geometry/nodes/node_geo_triangulate.cc b/source/blender/nodes/geometry index b08ea4574be..87d71252f1f 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_triangulate.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_triangulate.cc @@ -2,12 +2,18 @@ * * SPDX-License-Identifier: GPL-2.0-or-later */ +#include "BKE_customdata.h" #include "BKE_mesh.hh" #include "NOD_rna_define.hh" #include "GEO_mesh_triangulate.hh" +#include "NOD_rna_define.hh" + +#include "bmesh.h" +#include "bmesh_tools.h" +#include "DNA_mesh_types.h" #include "UI_interface.hh" #include "UI_resources.hh" @@ -18,6 +24,7 @@ namespace blender::nodes::node_geo_triangulate_cc { static void node_declare(NodeDeclarationBuilder &b) { b.add_input<decl::Geometry>("Mesh").supported_type(GeometryComponent::Type::Mesh); + b.add_input<decl::Bool>("Old Version").default_value(false); b.add_input<decl::Bool>("Selection").default_value(true).field_on_all().hide_value(); b.add_output<decl::Geometry>("Mesh").propagate_all(); } @@ -34,9 +41,41 @@ static void geo_triangulate_init(bNodeTree * /*tree*/, bNode *node) node->custom2 = GEO_NODE_TRIANGULATE_NGON_BEAUTY; } +static Mesh *triangulate_mesh_selection(const Mesh &mesh, + const int quad_method, + const int ngon_method, + const IndexMask &selection, + const int min_vertices) +{ + CustomData_MeshMasks cd_mask_extra = { + CD_MASK_ORIGINDEX, CD_MASK_ORIGINDEX, 0, CD_MASK_ORIGINDEX}; + BMeshCreateParams create_params{false}; + BMeshFromMeshParams from_mesh_params{}; + from_mesh_params.calc_face_normal = true; + from_mesh_params.calc_vert_normal = true; + from_mesh_params.cd_mask_extra = cd_mask_extra; + BMesh *bm = BKE_mesh_to_bmesh_ex(&mesh, &create_params, &from_mesh_params); + + /* Tag faces to be triangulated from the selection mask. */ + BM_mesh_elem_table_ensure(bm, BM_FACE); + selection.foreach_index([&](const int i_face) { + BM_elem_flag_set(BM_face_at_index(bm, i_face), BM_ELEM_TAG, true); + }); + + BM_mesh_triangulate(bm, quad_method, ngon_method, min_vertices, true, nullptr, nullptr, nullptr); + Mesh *result = BKE_mesh_from_bmesh_for_eval_nomain(bm, &cd_mask_extra, &mesh); + BM_mesh_free(bm); + + /* Positions are not changed by the triangulation operation, so the bounds are the same. */ + result->runtime->bounds_cache = mesh.runtime->bounds_cache; + + return result; +} + static void node_geo_exec(GeoNodeExecParams params) { GeometrySet geometry_set = params.extract_input<GeometrySet>("Mesh"); + const bool use_old = params.extract_input<bool>("Old Version"); Field<bool> selection_field = params.extract_input<Field<bool>>("Selection"); const AnonymousAttributePropagationInfo &propagation_info = params.get_output_propagation_info( "Mesh"); @@ -59,14 +98,23 @@ static void node_geo_exec(GeoNodeExecParams params) return; } - std::optional<Mesh *> mesh = geometry::mesh_triangulate( - *src_mesh, - selection, - geometry::TriangulateNGonMode(ngon_method), - geometry::TriangulateQuadMode(quad_method), - propagation_info); - if (mesh) { - geometry_set.replace_mesh(*mesh); + if (!use_old) { + std::optional<Mesh *> mesh = geometry::mesh_triangulate( + *src_mesh, + selection, + geometry::TriangulateNGonMode(ngon_method), + geometry::TriangulateQuadMode(quad_method), + propagation_info); + if (mesh) { + geometry_set.replace_mesh(*mesh); + } + } + else { + Mesh *mesh_out = triangulate_mesh_selection( + *src_mesh, quad_method, ngon_method, selection, 4); + if (mesh_out) { + geometry_set.replace_mesh(mesh_out); + } } }); ```
440 KiB
Jacques Lucke requested changes 2023-09-12 17:31:03 +02:00
Jacques Lucke left a comment
Member

Didn't read everything yet.

Can you maybe add some regression tests for the triangulate node that don't break by this patch? Think it should be possible to use the sorting of the Points to Curve node to get rid of the index dependence. Ideally, the versioning code is tested by those tests as well.

I wonder if there is a reason for sometimes using Ngons and sometimes ngons.

Didn't read everything yet. Can you maybe add some regression tests for the triangulate node that don't break by this patch? Think it should be possible to use the sorting of the Points to Curve node to get rid of the index dependence. Ideally, the versioning code is tested by those tests as well. I wonder if there is a reason for sometimes using `Ngons` and sometimes `ngons`.
@ -0,0 +14,4 @@
namespace blender::geometry {
enum class TriangulateNGonMode {
Member

Would be nice to have a short description for what these mean in practice.

Would be nice to have a short description for what these mean in practice.
Author
Member

Add some here, but the "beauty" and "polyfill" algorithms remain a bit vague to me, they're just in functions called by the old and new code.

Add some here, but the "beauty" and "polyfill" algorithms remain a bit vague to me, they're just in functions called by the old and new code.
HooglyBoogly marked this conversation as resolved
@ -0,0 +30,4 @@
std::optional<Mesh *> mesh_triangulate(
const Mesh &src_mesh,
const IndexMask &selection,
const TriangulateNGonMode ngon_mode,
Member

No need for const.

No need for `const`.
HooglyBoogly marked this conversation as resolved
@ -0,0 +518,4 @@
const OffsetIndices edges_by_ngon = calc_edges_by_ngon(src_faces, ngons, edge_offset_data);
/* Unselected faces are moved to the beginning of the new mesh. */
const IndexRange unselected_range(0, unselected.size());
Member

The naming of these variables can probably be unified since they are conceptually very similar.

The naming of these variables can probably be unified since they are conceptually very similar.
HooglyBoogly marked this conversation as resolved
@ -0,0 +535,4 @@
/* All newly created edges (from Ngons and quads). */
const IndexRange inner_edges(ngon_edges.start(), ngon_edges.size() + quad_edges.size());
/* Create a mesh with no face corners, since we don't know how many there will be yet. */
Member

Why is that not known yet? Can't that be easily computed based on the information above?

Why is that not known yet? Can't that be easily computed based on the information above?
Author
Member

We know the total number of corners from the final NGon triangles, but we didn't count the number of corners in the original NGons. Maybe there's a way to go quickly calculate it from the information we have anyway, not sure though.

This pattern of creating a mesh with no attributes is also helpful since it allows using implicit sharing when copying vertex attributes. (The same thing is done in mesh_copy_selection.cc).

We know the total number of corners from the final NGon triangles, but we didn't count the number of corners in the original NGons. Maybe there's a way to go quickly calculate it from the information we have anyway, not sure though. This pattern of creating a mesh with no attributes is also helpful since it allows using implicit sharing when copying vertex attributes. (The same thing is done in `mesh_copy_selection.cc`).
@ -97,0 +62,4 @@
std::optional<Mesh *> mesh = geometry::mesh_triangulate(
*src_mesh,
selection,
geometry::TriangulateNGonMode(ngon_method),
Member

Looks like this should static-assert that the values are actually all the same and/or there should be a comment on both enums that they need to be kept in sync, or we just get rid of GeometryNodeTriangulateNGons.

Looks like this should static-assert that the values are actually all the same and/or there should be a comment on both enums that they need to be kept in sync, or we just get rid of `GeometryNodeTriangulateNGons`.
HooglyBoogly marked this conversation as resolved
Author
Member

I started with some testing, and noticed that beauty mode gave different results:

Thanks! Found the issue. Source was confusing variable names in the BMesh code. Added a comment about that.

Can you maybe add some regression tests for the triangulate node that don't break by this patch?

I added regression tests to the PR. But I didn't get to the index independence, or regression tests for the versioning.

I wonder if there is a reason for sometimes using Ngons and sometimes ngons.

Hmm, I thought I was consistent about saying "NGon" in comments and "ngon" as variable names.

>I started with some testing, and noticed that beauty mode gave different results: Thanks! Found the issue. Source was confusing variable names in the BMesh code. Added a comment about that. >Can you maybe add some regression tests for the triangulate node that don't break by this patch? I added regression tests to the PR. But I didn't get to the index independence, or regression tests for the versioning. >I wonder if there is a reason for sometimes using Ngons and sometimes ngons. Hmm, I thought I was consistent about saying "NGon" in comments and "ngon" as variable names.
Hans Goudey requested review from Jacques Lucke 2023-09-13 00:27:38 +02:00
Hans Goudey added 10 commits 2023-09-13 00:30:40 +02:00
Hans Goudey added 1 commit 2023-09-13 01:36:37 +02:00
Jacques Lucke requested changes 2023-09-13 10:51:44 +02:00
Jacques Lucke left a comment
Member

Edge attributes are currently propagated differently in main vs. this patch.

Edge attributes are currently propagated differently in `main` vs. this patch.
Hans Goudey added 2 commits 2023-09-13 20:03:13 +02:00
Hans Goudey requested review from Jacques Lucke 2023-09-13 20:03:19 +02:00
Hans Goudey added 2 commits 2023-09-14 02:27:29 +02:00
Hans Goudey changed title from Geometry Nodes: Port triangulate node from BMesh to Mesh to Geometry Nodes: Port triangulate node from BMesh to Mesh 2023-09-14 14:12:42 +02:00
Hans Goudey changed title from Geometry Nodes: Port triangulate node from BMesh to Mesh to WIP: Geometry Nodes: Port triangulate node from BMesh to Mesh 2023-09-14 14:12:50 +02:00
Hans Goudey added 6 commits 2023-09-18 22:11:48 +02:00
Hans Goudey added 6 commits 2023-09-20 16:48:02 +02:00
Falk David reviewed 2023-09-20 16:54:58 +02:00
@ -180,3 +179,1 @@
* \note The result may be an empty span.
*/
static MutableSpan<int> get_orig_index_layer(Mesh &mesh, const eAttrDomain domain)
static std::optional<MutableSpan<int>> get_orig_index_layer(Mesh &mesh, const eAttrDomain domain)
Member

Are the changes to the node_geo_extrude_mesh.cc intentional? Seems strange to include them in this PR.

Are the changes to the `node_geo_extrude_mesh.cc` intentional? Seems strange to include them in this PR.
Author
Member

Yeah, that's just something I noticed when trying to share code between multiple nodes. I'll commit a bunch of the changes here separately before requesting review again. Thanks for looking :)

Yeah, that's just something I noticed when trying to share code between multiple nodes. I'll commit a bunch of the changes here separately before requesting review again. Thanks for looking :)
Hans Goudey added 1 commit 2023-09-20 17:09:26 +02:00
Hans Goudey added 1 commit 2023-10-03 14:18:35 +02:00
Hans Goudey added 2 commits 2023-11-21 18:23:08 +01:00
Hans Goudey added 1 commit 2023-11-22 20:42:28 +01:00
Hans Goudey added 1 commit 2023-11-22 20:43:29 +01:00
Hans Goudey added 107 commits 2023-11-27 18:01:19 +01:00
Fix: add region padding to the NLA track(s) if markers are present.

Pull Request: #115213
Enum menus that do not have categories should be able to wrap to
multiple columns is there is enough horizontal space.

Pull Request: #115283
This allows for some optimization when we know
some effects are not present. Also this is needed
to detect the case when world contains absorption
in order to disable distant lighting.

Related #114062
Add a function that copies selected values to groups of values in the
result array. Add a runtime-typed version and a version for affecting
all attributes. Also make the "gather_group_to_group" follow the
same pattern.
Formatting changes resulting from running Make Format
The 'NULL' identifier within the `FModifier.type` enum was acidently
renamed to 'nullptr' during the C to C++ source conversion in 3ece6876af.
Since b1526dd2c6, viewport renderer sets `G.is_rendering`, but VSE scene
rendering function used this to decide whether to do offscreen opengl
render or use render API. This logic was quite weak.

Use `SeqRenderData::for_render` instead of `G.is_rendering`, since it
explicitly defines whether strip rendering is invoked by F12 render job.

Since offscreen gl rendering rely on `BLI_thread_is_main()` being
true, use this to initialize `do_seq_gl` variable for clarity.

The use of render job path was further conditioned on `G.is_rendering`,
with exception of running headless, but this condition was incorrect.
This condition was reformulated as precondition, which if true, returns
`nullptr` instead of crashing.

Pull Request: #115079
These drivers crash on startup caused by a driver bug. This include the
latest drivers for legacy Intel CPUs with a HD 4000/HD 5000 series GPU.

To be on the safe side all drivers with version 20.19.15.51* will be marked
unsupported as we don't have the platforms to identify the precise driver
versions that fail.

See #113124 for more information.

Pull Request: #115228
No functional changes.

Rename the `eAutokey_Flag` to `eKeyInsert_Flag`
to indicate that it is not only used for auto keying.
Also rename the enum items to better reflect what they are used for.

Pull Request: #115295
OpenImageDenoise has two modes, high quality and balanced. This now exposes the modes as user parameters, with viewport denoising defaulting to balanced and final frame rendering set to high quality.

Ref #115045

Co-authored-by: Werner, Stefan <stefan.werner@intel.com>
Pull Request: #115265
This reverts commit 2e2291dd83.
Motivation: When discussing with @Jeroen-Bakker and @aras_p about how to
approach sorting when rendering Gaussian splats in Blender, we realised that
compute shaders could help there (and have many other use cases), and that also
due to Blender 4.0 being on OpenGL >= 4.3, we can now rely on compute shaders
existing.

This PR is an initial pass for that functionality. It comes with a Python example, which
runs a compute shader and saves the output to a texture, which is then rendered in the
viewport.

There is no exposed support for storage buffers yet, but I expect I'll be able to work on
them soon if this is accepted.

The newly added parts are:
1. `gpu.compute.dispatch()`
2. a way set the compute source to `GPUShaderCreateInfo`: `shader_info.compute_source()`
3. a way to set the image store for `GPUShaderCreateInfo`: `shader_info.image()`
4. a way to set the `local_group_size` for `GPUShaderCreateInfo`: `shader_info.local_group_size(x,y,z)`
5. a way to get `max_work_group_size` from capabilities: `gpu.capabilities.max_work_group_size_get(index)`
6. a way to get `max_work_group_count` from capabilities: `gpu.capabilities.max_work_group_count_get(index)`

Pull Request: #114238
When in scene mode, operation searches the object in master collection
to unlink. But object can be in any collection. So iterate through all
collections present in scene to unlink the object

Pull Request: #112955
While it may have been working from a practical PoV (not certain though,
since some bug would prevent clearing runtime data when writing embedded
Scene collection in current code), this is semantically wrong.

The owner of an embedded ID is a critical piece of information in
Blender data structure and ID management code. Having it written in
.blend files is also a potential good source of data for investigating
issues.

Further more, this handling of `owner` ID data is somewhat generic now
in ID management, so if this data should be considered runtime, then the
change should also be made in NodeTree and Key IDs.

This commit partially reverts 44dd3308a5, in the future I'd like to
be involved in the review of changes affecting ID management.

NOTE: fix for embedded collection runtime data not being cleared on
write will be committed separately.
Conversion of GPv2 operator.

Related to #114997

Pull Request: #115272
After renaming "NLA Channels" to "NLA Tracks" user facing, doing a cleanup pass to internally rename things.

Pull Request: #115011
This is the conversion of existing operators in GPv2.

Related to #114997

Pull Request: #115041
Add missing includes and remove the attributes_lib from shaders
that don't need them.

Pull Request: #114598
Fix Texture Coordinates in World material and screen_coordinates
when using overscan.

Pull Request: #115057
Rename `isect_data_setup` functions to avoid shader compiler errors.

Pull Request: #115175
This operators hide all materials except the active one.

There is a parameter to hide only active as it was in GPv2.

Related to #114997

Pull Request: #115038
# Issue
Having a lot of keys in your scene can dramatically slow down the Dope Sheet.
That is because everytime the Dope Sheet is redrawn,
the Keylists have to be recomputed.
In the case of the summary channel, that means going through
all keyframes of all the `FCurves` and adding them to the keylist.
That eats up 95% of the time it takes to draw a frame.

# This PR
It's not a perfect solution, rather it solves the performance issue
for the case when you are not displaying all keys.
Instead of going through all the keys, only add the
keys visible in the view to the keylist.
This speeds up the Dope Sheet significantly
depending on the zoom level.

This also improves the responsiveness when selecting and transforming keyframes.

# Performance changes

The function measured is `ED_channel_list_flush`
which is responsible for building the keylists and drawing them.
The test setup contains 62 bones with all
10 channels keyed (location, rot quaternion, scale) on 6000 frames.
So 3.720.000 keys. The heavier the dataset the bigger the performance impact.

The data was recorded with only the Dope Sheet open, and 3 channels visible
* Summary
* Object
* Action

The more channels are visible, the greater the performance gain. This can be seen in the video.

| visible range | before | after |
| - | - | - |
| 200f | 250ms | 10ms |
| 400f | 250ms | 18ms |
| 3000f | 250ms | 130ms |
| 6000f | 250ms | 250ms |

Pull Request: #114854
Load VFont curves using BLF API rather than local FreeType calls.

Pull Request: #110187
Use a compute shader to copy non float4 position vertex buffers into the
`geometry_steps` buffer.

Fix #113730
Fix #114775
Fix #114144

Pull Request: #115309
The flag was incorrectly removed in 440f39f2e5
BLF needs to initialized to properly set up Freetype, caching, etc.

Pull Request: #115318
Harfbuzz and FriBiDi are included in our external libraries for all
platforms. This PR adds the glue to make them available as optional
build components (off by default).

Pull Request: #114947
`vertex` is the name of the function entry point and is
defined as a macro. Renaming fixes the issue.
Formatting changes resulting from Make Format
Simplification of formatted text output in the Text Editor, removing
duplication and redundancies. Removes 35 lines.

Pull Request: #110174
vcpkg somewhat forcefully injects itself into the build system
causing some work stations to use the vcpkg headers/libs rather than
the headers from our lib folder,we added some mitigation against this
in 34b3a9583a but still the occasional
user with build issues showed up, this should nip that in the bud once
and for all.
channel_index was renamed to track_index in f9b9ec3b26. Commit missed
one change in `ANIM_channel_draw_widgets`, hence the error in console
when mouse is over NLA editor.

Pull Request: #115332
Having this node with only a single input is not useful.
Not properly initialized booleans cause problems due to the branchless increment.

Pull Request: #115338
The problem here was that `RNA_parameter_set_lookup` was used incorrectly.
Instead of passing in the `PointerRNA`, one can pass in the value directly. If one
wanted to pass the `PointerRNA`, one would have to use `PARM_RNAPTR`, but
that's not necessary here.

Pull Request: #115347
Caused by 3dc119a440
No functional changes.

The function is useful for autokeying as well, which is why
it is being moved out of the editors.

Pull Request: #115349
When pressing `I` in the viewport, the colors of the FCurve channels were no longer set correctly.

Caused by #113504

Fix by removing the flag that determined that in the first place,
and read straight from the user preferences. Then move the code
that sets the mode on the FCurve into the function that actually creates it.

For 99% of cases the code went to the user preference flag `AUTOKEY_FLAG_XYZ2RGB`
and if that was set, the `INSERTKEY_XYZ2RGB` would be set. The only case where this
was not from the user preferences was from custom keying sets.
There was an override flag for FCurve colors on custom keying sets.
I removed that with this patch since the use case is not apparent
and custom keying sets are hardly used.

Pull Request: #115297
The issue was that RNA_path_from_ID_to_property
returns a char * that the caller is expected to free.
But the code was storing it in a std::string
Some test cases inside the vulkan backend don't rely on an initialized
Vulkan Context and can always be run.

This PR enables those test cases when `WITH_GTEST` and
`WITH_VULKAN_BACKEND` is on. Allowing some tests to run on the
buildbot.

Pull Request: #114574
This PR adds the default vulkan pipeline cache to when creating
pipelines.

Pipeline caching reuses pipelines that have already been created.
Pipeline creation can be somewhat costly - it has to compile the
shaders at creation time for example.

The cache is recreated at each run of Blender.

Pull Request: #115346
This restrict the usage of distant light if the world shader
contains absorption phenomenon.

Why this is needed is described in #114062.

At the same time, this removes the parameter for enabling
volume light as this is now an auto-detected optimization.

This also contains a few small cleanups.

Pull Request: #115284
Currently all buffer types were stored in host memory, which is visible to the GPU as well.
This is typically slow as the data would be transferred over the PCI bus when used.

Most of the time Index and Vertex buffers are written once and read many times so it makes
more sense to locate them on the GPU. Storage buffers typically require quick access as they
are created for shading/compute purposes.

This PR will try to store vertex buffers, index buffers and storage buffers on device memory
to improve the performance.

Uniform buffers are still located on host memory as they can be uploaded during binding process.
This can (will) reset the graphics pipeline triggering draw calls using unattached resources.

In future this could be optimized further as in:
* using different pools for allocating specific buffers, with a fallback when buffers cannot be
  stored on the GPU anymore.
* store uniform buffers in device memory

Pull Request: #115343
Move tagging from `flush_editors_id_update` to `deg::id_tag_update`.

Since ID relationships cannot always be trusted when tagging for update
happens (can happen e.g. during liboverride apply itself, when deleting
IDs, and so on), embedded IDs are simply tagged here too, instead of
directly tagging their owner ID.

Propagation from embedded to owner ID then happens when checking and
'consuming' the `LIB_TAG_LIBOVERRIDE_AUTOREFRESH` ID tag, in
`BKE_lib_override_library_main_operations_create`.

Pull Request: #115319
Texture usage flag `GPU_TEXTURE_USAGE_MIP_SWIZZLE_VIEW`
was originally implemented and used too conservatively for many
cases in which the underlying API flags were not required.

Renaming to `GPU_TEXTURE_USAGE_FORMAT_VIEW` to reflect
the only essential use case for when a texture view is initialized with
a different texture format to the source texture. Texture views can
still be created without this flag when mip range or base level is
adjusted,

This flag is still required by stencil views and internally by the Metal
backend for certain feature support such as SRGB render toggling.

Patch also includes some small changes to the Metal backend to
adapt to this new compatibility and correctly capture all texture view
use-cases.

Related to #115269

Authored by Apple: Michael Parkin-White

Pull Request: #115300
This change adds timeline semaphores to track submissions. The previous
implementation used a fence.

Timeline semaphores can be tracked in more detail as it is an counter.
For each submission the counter can be stored locally and when waiting
for completion the counter can be retrieved again and checked if is
known to be succeeded by a higher value.

The timeline semaphore is stored next to the queue and can also be used
to synchronize between multiple contexts.

Pull Request: #115357
The same data was essentially defined twice.

Pull Request: #115353
`action_fcurve_ensure` can be passed a nullptr for
the PointerRNA, which the function didn't check for
after the refactor.
Note: Everything here is read-only, this is expected to be used as
testing/validating/debug info access for the time being, not as an
actual way to edit data.
This is caused by some path not setting the proper usage flag
on the main depth stencil texture making impossible to use
stencil view of this texture.

Make this flag mandatory for offscreen buffer fixes the issue.

Pull Request: #115361
Pretty straightforward

- Remove any bf_intern_clog paths from INC
- Add a dependency though LIB when missing

context: https://devtalk.blender.org/t/cmake-cleanup/30260

Pull Request: #115323
Pretty straightforward

- Remove any bf_blenfont paths from INC
- Add a dependency though LIB when missing

context: https://devtalk.blender.org/t/cmake-cleanup/30260

Pull Request: #115365
Remove leading space in window title for saved files.

Pull Request: #114704
Use vector instead of malloc, make cost function a callback, tweak names
and comments for clarity.
Don't draw marker line through marker. Draw Selected markers in a color
that matches selected line and text.

Pull Request: #115252
Improvements to the drawing of shadows, used with blocks, menus, nodes,
etc. Improvements to shape, especially at the top corner or at extremes
of widget roundness. Allows transparent objects to have shadows. This
is a nice refactor that removes a lot of code.

Pull Request: #111794
Improve corner rounding of menus and popup blocks.

Pull Request: #111554
Share code of geometry processing of mesh selection.
For now, `mesh_copy_selection` is only user of this code,
but in #115142 this will also be used.

Pull Request: #115376
Caused by 07b5e1bd80

Error in the early return logic.
The goal here is to make it easier to make node links. Previously, it was quite
easy to accidentally start box selection or to trigger the link-drag-search when
that was not intended.

Now, the tolerances are a bit easier to work with. Also, instead of trying to use
the first socket that is close enough, it will find the closest socket instead. It feels
much better in my testing already, but obviously the values can be tuned more
with some testing.

Also we have to make sure to not accidentally make other things like resizing
nodes harder.

Pull Request: #115010
Pretty straightforward

- Remove any bf_depsgraph paths from INC
- Add a dependency though LIB when missing

context: https://devtalk.blender.org/t/cmake-cleanup/30260

Pull Request: #115422
Pretty straightforward

- Remove any bf_imbuf paths from INC
- Add a dependency though LIB when missing

context: https://devtalk.blender.org/t/cmake-cleanup/30260

Pull Request: #115425
To avoid this, clear `ME_AUTOSMOOTH_LEGACY` flag once auto-smooth node is
added to object from legacy files.

Pull Request: #115394
Make size inference consistent with the viewport compositor (from a user's perspective). This patch uses constat folding to create a constant output out of constant inputs. This is consistent with the results of the realtime compositor.

Nodes not included in this patch require further refactoring or discussion. They will be addressed in future patches.

Pull Request: #114755
Pull Request: #115377
Ref: !115256
This reverts commit fe50189193.

Pull Request: #115458
Using a shared type-agnostic attribute propagation function gives a 32
KB reduction in binary size with no observable performance difference.
The factor needs to be calculated against the evaluated bounds rather
than the bounds of the original geometry. Error in 1cbd0f5a85.
Silence warning `'call' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]`

Pull Request: #115440
Due to recent changes the local variables where not explicitly cast, that failed
when compiling on Metal.

Pull Request: #115463
Caused by 74dd1e044b.
Fixes an issue where the sheen would render incorrectly when
the normal of the surface is parallel to the incoming direction.

Co-authored-by: Lukas Stockner <lukas.stockner@freenet.de>

Pull Request: #115286
Recent change in commit 3f778150a9
caused compilation errors in Metal due to type ambiguity. Updating call to
explicitly utilise floats where appropriate.

Authored by Apple: Michael Parkin-White

Co-authored-by: Michael Parkin-White <mparkinwhite@apple.com>
Pull Request: #115301
Track the last time an object or its dependencies were evaluated by the
dependency graph.
These values can be compared against DEG_get_update_count().
Implemented following the design from #114112

Pull Request: #115196
We should use explicit casting. Although it is not always needed it
is a best practise in order to support the shaders on Metal.

* `float max(float, int)` is not supported on Metal and fails with a compilation error

Pull Request: #115464
Implement the EEVEE side of #114112.
Use ObjectRuntime::last_update to compute the ObjectHandle::recalc
flags.

Fix #114135
Fix #114696
Fix #114170

Pull Request: #115243
The `eval_frame` was not initialized to a default value,
causing a crash in some situations on windows.
This at least avoids a crash even if the outcome of
whatever uses the `eval_frame` might not be correct.
The `paint_init_pivot` calculates the `location` based
on the midpoint of the bounds of the object. For grease pencil
the bounds depend on the current frame (for original data).

This fixes the issue by making the `bounds_min_max` function
dependent on the `frame` and by introducing a `bounds_min_max_eval`
for evaluated data.
Nowadays we have the loose edge cache which can tell us if there will be
any loose edges to subdivide. This means the topology map creation that
included multi-threading doesn't need to be behind a lock anymore.

Pull Request: #115480
The ImplicitSharingPtr has an implicit constructor for raw pointers.
This has unintended effects when comparing an ImplicitSharingPtr to a
raw pointer: The raw pointer is implicitly converted to the shared
pointer (without change in refcount) and when going out of scope will
decrement user count, eventually freeing the data.

Conversion from raw pointer to shared pointer should not happen
implicitly. The constructor is made explicit now. This requires a little
more boilerplate when constructing a sharing pointer. A special
constructor for the nullptr is added so comparison with nullptr can
still happen without writing out a constructor.

Pull Request: #115476
This adds a new function, `compare_meshes`,
as a replacement for `BKE_mesh_cmp`.

The main benefits of the new version are the following:
- The code is written in c++, and makes use of the new attributes API.
- It adds an additional check, to see if the meshes only differ by
  their indices. This is useful to verify correctness of new algorithmic
  changes in mesh code, which might produce mesh elements in a different
  order than the original algorithm. The tests will still fail, but the
  error will show that the indices changed.

Some downsides:
- The code is more complex, due to having to be index-independent.
- The code is probably slower due to having to do comparisons "index-
  independently". I have not tested this, as correctness was my priority
  for this patch. A future update could look to improve the speed,
  if that is desired.
- This is technically a breaking API change, since it changes the
  returned values of `rna_Mesh_unit_test_compare`. I don't think that
  there are many people (if any) using this, besides our own unit tests.

All tests that pass with `BKE_mesh_cmp` still pass with the new version.

**NOTE:**
Currently, mesh edge indices are allowed to be different in the
comparison, because `BKE_mesh_cmp` also allowed this. There are some
tests which would fail otherwise. These tests should be updated, and
then the corresponding code as well.

I wrote up a more detailed explanation of the algorithm here:
https://hackmd.io/@bo-JY945TOmvepQ1tAWy6w/SyuaFtay6

Pull Request: #112794
Implement the next phases of bounds improvement design #96968.
Mainly the following changes:

Don't use `Object.runtime.bb` for performance caching volume bounds.
This is redundant with the cache in most geometry data-block types.
Instead, this becomes `Object.runtime.bounds_eval`, and is only used
where it's actually needed: syncing the bounds from the evaluated
geometry in the active depsgraph to the original object.

Remove all redundant functions to access geometry bounds with an
Object argument. These make the whole design confusing, since they
access geometry bounds at an object level.

Use `std::optional<Bounds<float3>>` to pass and store bounds instead
of an allocated `BoundBox` struct. This uses less space, avoids
small heap allocations, and generally simplifies code, since we
usually only want the min and max anyway.

After this, to avoid performance regressions, we should also cache
bounds in volumes, and maybe the legacy curve and GP data types
(though it might not be worth the effort for those legacy types).

Pull Request: #114933
needed bf::imbuf got missed in a recent cmake cleanup
needed shaderc_shared.dll to be available in the path
The node is still a bit non-standard in that it resizes an existing
mesh rather than creating a new one, but this commit makes the extrude
node a bit more similar to other mesh operations and makes other
miscellaneous improvements, including:
- Less use of intermediate states (compared to initial or final)
- Topology map building is no longer reimplemented for the node
- Attribute interpolation happens in a more familiar way
- Some topology maps can be skipped if a domain is empty
- More use of `IndexMask` instead of an array of indices
- Logarithmic cost index mask lookup is avoided
- Build index maps instead of implementing attribute propagation
  separately for every type

Overall these changes might improve performance in a few cases, and
they reduce Blender's binary size by 58 KB. Edge indices are different
in some cases of the edge mode, so the test files are updated.
Hans Goudey added 1 commit 2023-11-27 18:09:52 +01:00
Hans Goudey added 3 commits 2023-11-27 23:41:59 +01:00
Hans Goudey added 2 commits 2023-12-19 01:28:02 +01:00
Hans Goudey added 2 commits 2023-12-20 00:55:25 +01:00
Hans Goudey added 1 commit 2023-12-27 17:14:22 +01:00
Hans Goudey added 2 commits 2023-12-27 18:18:05 +01:00
Hans Goudey added 4 commits 2023-12-30 16:28:44 +01:00
Hans Goudey added 2 commits 2024-01-11 21:28:15 +01:00
The only remaining todo is to do some final performance instrumentation
just to double check some assumptions
Hans Goudey added 1 commit 2024-01-12 14:34:03 +01:00
This pull request has changes conflicting with the target branch.
  • source/blender/blenlib/BLI_offset_indices.hh
  • source/blender/geometry/CMakeLists.txt

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u mesh-triangulate:HooglyBoogly-mesh-triangulate
git checkout HooglyBoogly-mesh-triangulate
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
Asset Browser Project
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 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#112264
No description provided.