Page MenuHome
Differential D15786

BLI_array_utils::gather()
ClosedPublic
Actions

Authored by Mattias Fredriksson (Osares) on Aug 26 2022, 1:17 AM.

Details

Reviewers
Hans Goudey (HooglyBoogly)
Jacques Lucke (JacquesLucke)
Commits
rBce54f4855674: BLI: Add generic utlity for gathering values with indices
Summary

Threaded representations of GVArray::materialize_compressed_to_uninitialized(..) has been reimplemented locally in multiple geometry node contexts. The purpose of this patch is therefore to:

  • Assemble these implementations in a single file.
  • Provide a naming convention that is easier to recognize in comparison to materialize_compressed_to_uninitialized().

It is likely the function has been reimplemented in more places but this is a starting point for assembling them in one place.

Diff Detail

Repository
rB Blender
Branch
gather (branched from master)
Build Status
Buildable 23790
Build 23790: arc lint + arc unit

Event Timeline

Mattias Fredriksson (Osares) requested review of this revision.Aug 26 2022, 1:17 AM
Mattias Fredriksson (Osares) created this revision.
Harbormaster completed remote builds in B23492: Diff 54985.Aug 26 2022, 1:17 AM
Mattias Fredriksson (Osares) edited the summary of this revision. (Show Details)Aug 26 2022, 1:32 AM
Mattias Fredriksson (Osares) added a reviewer: Hans Goudey (HooglyBoogly).
Mattias Fredriksson (Osares) added a comment.Aug 26 2022, 1:41 AM
This comment was removed by Mattias Fredriksson (Osares).
Mattias Fredriksson (Osares) mentioned this in D14481: Curves Trim.Aug 26 2022, 1:42 AM
Mattias Fredriksson (Osares) updated this revision to Diff 54986.Aug 26 2022, 1:48 AM
Mattias Fredriksson (Osares) edited the summary of this revision. (Show Details)

Removed unused gather()

Harbormaster completed remote builds in B23493: Diff 54986.Aug 26 2022, 1:48 AM
Hans Goudey (HooglyBoogly) added a comment.Aug 28 2022, 10:51 PM

Thanks for splitting this out.

source/blender/blenlib/BLI_array_utils.hh
39–49

This could be moved to a .cc file to avoid adding the implementation to every compile unit.
Since that's one of the benefits of a runtime type system, we might as well use it.

Hans Goudey (HooglyBoogly) requested changes to this revision.Aug 30 2022, 1:19 AM
This revision now requires changes to proceed.Aug 30 2022, 1:19 AM
Mattias Fredriksson (Osares) updated this revision to Diff 55333.Sep 5 2022, 9:46 PM
  • Moved gather() impl. to .cc
Harbormaster completed remote builds in B23624: Diff 55333.Sep 5 2022, 9:46 PM
Hans Goudey (HooglyBoogly) accepted this revision.Sep 13 2022, 1:12 AM
Hans Goudey (HooglyBoogly) added a reviewer: Jacques Lucke (JacquesLucke).
Aaron Carlisle (Blendify) added a subscriber: Aaron Carlisle (Blendify).Sep 13 2022, 1:43 AM
Aaron Carlisle (Blendify) added inline comments.
source/blender/blenlib/intern/BLI_array_utils.cc
1 ↗(On Diff #55333)
Mattias Fredriksson (Osares) updated this revision to Diff 55653.EditedSep 13 2022, 11:20 PM
  • Rebased and resolved conflicts, BLI_array_utils.cc -> array_utils.cc, added license statement
Harbormaster completed remote builds in B23740: Diff 55653.Sep 13 2022, 11:20 PM
Hans Goudey (HooglyBoogly) accepted this revision.Sep 13 2022, 11:54 PM
Mattias Fredriksson (Osares) updated this revision to Diff 55662.Sep 14 2022, 12:56 AM
  • unincluded BLI_utildefines.h, built with clang
Harbormaster completed remote builds in B23743: Diff 55662.Sep 14 2022, 12:56 AM
Jacques Lucke (JacquesLucke) accepted this revision.Sep 14 2022, 11:26 AM
This revision is now accepted and ready to land.Sep 14 2022, 11:26 AM
Hans Goudey (HooglyBoogly) added inline comments.Sep 15 2022, 1:37 AM
source/blender/blenlib/BLI_array_utils.hh
49–63

It would be nice to provide a version for Span too, so this can be useful in areas that don't use virtual arrays. That could be a separate step though.

Mattias Fredriksson (Osares) updated this revision to Diff 55756.Sep 15 2022, 11:27 PM
Mattias Fredriksson (Osares) marked an inline comment as done.
  • Rebased, added gather(span), src
  • Asserts
Harbormaster completed remote builds in B23790: Diff 55756.Sep 15 2022, 11:27 PM
Mattias Fredriksson (Osares) marked an inline comment as done.Sep 15 2022, 11:45 PM

Investigated using GSpan for gather() in mesh_to_curve_convert.cc. Due to attribute_math::convert_to_static_type() being part of BKE, with materialize_compressed_to_uninitialized() only implemented for IndexMask, and get_to_uninitialized() not being as performant. I decided to leave it as is since determining the appropriate approach was not apparent to me.

source/blender/blenlib/BLI_array_utils.hh
49–63

Added for implementations for gather(Span) from my local rejected Blender code depot. Been trying not to add unused functions as they seem to not been approved of 😁.

Hans Goudey (HooglyBoogly) added inline comments.Sep 15 2022, 11:48 PM
source/blender/blenlib/BLI_array_utils.hh
49–63

Yeah, generally we avoid it I guess, but I can already think of a few places where it would be helpful :)

Closed by commit rBce54f4855674: BLI: Add generic utlity for gathering values with indices (authored by Mattias Fredriksson (Osares), committed by Hans Goudey (HooglyBoogly)). · Explain WhySep 18 2022, 5:12 AM
This revision was automatically updated to reflect the committed changes.
Hans Goudey (HooglyBoogly) added a commit: rBce54f4855674: BLI: Add generic utlity for gathering values with indices.

Revision Contents
Changeset List

PathSize
source/
blender/
blenlib/
81 lines
intern/
13 lines
geometry/
intern/
16 lines
nodes/
geometry/
nodes/
19 lines

Diff 55756

View Options

source/blender/blenlib/BLI_array_utils.hh

Loading...
View Options

source/blender/blenlib/intern/array_utils.cc

Loading...
View Options

source/blender/geometry/intern/mesh_to_curve_convert.cc

Loading...
View Options

source/blender/nodes/geometry/nodes/node_geo_mesh_to_points.cc

Loading...