Anim: Graph Editor - use Map to update TransInfo pointers instead of searching #120816

Christoph Lendenfeld · 2024-04-19T11:04:23+02:00

2024-04-19 11:04:23 +02:00

This is a performance improvement when moving a bunch of keys on the same FCurve in heavy scenes.

When moving keys in such a way that the BezTriple array of the FCurve has to be sorted,
the pointers of TransInfo also have to be updated (deeper explanation below).
This used to happen by doing a nested loop over all BeztMap and all TransData2D.
There was a bit of optimization with the blender::Vector<bool> adjusted which stored if a TransData2D has
been fixed yet. But in general the complexity was still BeztMap.size() * TransData.size().

There are two optimizations that can be done here.

Skip any BeztMap if old_index == new_index. If the Key is not going to move any pointers to it will still be valid.
Use a Map<float *, int> built beforehand to quickly get the TransData2D that needs updating instead of searching. The int in this case is the index to the TransData2D array.

Doing this reduces the complexity to BeztMap.size() + TransData.size(). (if I am not mistaken)

Measurements of beztmap_to_data

-	Before	After
Moving 1 key of 1 FCurve	~24000 ns	~5800ns
Moving ~1000 keys of 1 FCurve	17ms	0.02ms

Measurements of remake_graph_transdata

-	Before	After
Moving 1 key of 279 FCurves	290ms	22ms
Moving ~300 keys of 279 FCurves	82 SECONDS	80ms

Test file used
https://download.blender.org/ftp/sybren/animation-rigging/heavy_mocap_test.blend

The deeper technical explanation.
TransInfo has an array of TransData.
TransData has pointers to the float arrays of a BezTriple.
The BezTriple array is sorted by swapping data, meaning the TransData will now point to the wrong data in the array.
This has to be updated and we can do that by using the BeztMap. This is all under the assumption that BeztMap is sorted in the exact same way as BezTriple otherwise this method will fail.
But by doing it the same way, we can know at which index the BezTriple is before and after sorting.
Now we just need to find the corresponding TransData. That can be done by comparing pointers. The BeztMap stores the BezTriple it represents and from it we can get the pointers to its vec 0, 1 and 2. (key and handles)

This is a performance improvement when moving a bunch of keys on the same `FCurve` in heavy scenes. When moving keys in such a way that the `BezTriple` array of the FCurve has to be sorted, the pointers of `TransInfo` also have to be updated (deeper explanation below). This used to happen by doing a nested loop over all `BeztMap` and all `TransData2D`. There was a bit of optimization with the `blender::Vector<bool> adjusted` which stored if a `TransData2D` has been fixed yet. But in general the complexity was still `BeztMap.size() * TransData.size()`. There are two optimizations that can be done here. * Skip any BeztMap if `old_index == new_index`. If the Key is not going to move any pointers to it will still be valid. * Use a `Map<float *, int>` built beforehand to quickly get the `TransData2D` that needs updating instead of searching. The `int` in this case is the index to the `TransData2D` array. Doing this reduces the complexity to `BeztMap.size() + TransData.size()`. (if I am not mistaken) Measurements of `beztmap_to_data` | - | Before | After | | - | - | - | | Moving 1 key of 1 FCurve | ~24000 ns | ~5800ns | | Moving ~1000 keys of 1 FCurve | 17ms | 0.02ms | Measurements of `remake_graph_transdata` | - | Before | After | | - | - | - | | Moving 1 key of 279 FCurves | 290ms | 22ms | | Moving ~300 keys of 279 FCurves | 82 **SECONDS** | 80ms | Test file used https://download.blender.org/ftp/sybren/animation-rigging/heavy_mocap_test.blend ------ The deeper technical explanation. `TransInfo` has an array of `TransData`. `TransData` has pointers to the float arrays of a `BezTriple`. The `BezTriple` array is sorted by swapping data, meaning the `TransData` will now point to the wrong data in the array. This has to be updated and we can do that by using the `BeztMap`. This is all under the assumption that `BeztMap` is sorted in the exact same way as `BezTriple` otherwise this method will fail. But by doing it the same way, we can know at which index the `BezTriple` is before and after sorting. Now we just need to find the corresponding `TransData`. That can be done by comparing pointers. The `BeztMap` stores the `BezTriple` it represents and from it we can get the pointers to its `vec` 0, 1 and 2. (key and handles)

❤️ 6 🎉 2 🚀 1

Christoph Lendenfeld added this to the 4.2 LTS milestone 2024-04-19 11:04:23 +02:00

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2024-04-19 11:04:23 +02:00

Christoph Lendenfeld added 1 commit 2024-04-19 11:04:32 +02:00

6bc780127f use map

Nathan Vegdahl requested review from Nathan Vegdahl 2024-04-19 11:43:46 +02:00

Iliya Katushenock reviewed 2024-04-19 16:03:10 +02:00

source/blender/editors/transform/transform_convert_graph.cc Outdated

						
				@ -809,3 +823,1 @@

				  /* Used to mark whether an TransData's pointers have been fixed already, so that we don't

				   * override ones that are already done. */

				  blender::Vector<bool> adjusted(tc->data_len, false);

				  blender::Map<float *, int> trans_info_map;

Iliya Katushenock commented

2024-04-19 16:03:10 +02:00

Not sure if this is correct to use float * key for array of floats.

Not sure if this is correct to use `float *` key for array of floats.

Christoph Lendenfeld commented

2024-04-19 16:45:42 +02:00

tc->data_2d[i].loc2d is a float pointer and the i is what i need to find again later.

`tc->data_2d[i].loc2d` is a float pointer and the `i` is what i need to find again later.

Iliya Katushenock commented

2024-04-19 16:56:37 +02:00

Yeah, sorry, i just get deep in to that...

👍 1

mod_moder marked this conversation as resolved

Nathan Vegdahl commented

2024-04-22 11:42:39 +02:00

While testing, I immediately ran into an assert when trying to move keys in the linked heavy mocap test file:

BLI_assert failed: /home/guest/Projects/blender/blender/source/blender/blenlib/BLI_map.hh:1140, add__impl(), at 'hash_(*slot.key()) == hash'
../install_debug_clang/blender(___interceptor_backtrace+0x79) [0x62b9019]fish: Job 1, '../install_debug_clang/blender' terminated by signal SIGABRT (Abort)

I reliably run into this every time.

(Although if I comment out the assert, everything seems to work fine. Nevertheless, this suggests there's a bug somewhere, whether it be in this PR or in the BLI_map code.)

While testing, I immediately ran into an assert when trying to move keys in the linked heavy mocap test file: ``` BLI_assert failed: /home/guest/Projects/blender/blender/source/blender/blenlib/BLI_map.hh:1140, add__impl(), at 'hash_(*slot.key()) == hash' ../install_debug_clang/blender(___interceptor_backtrace+0x79) [0x62b9019]fish: Job 1, '../install_debug_clang/blender' terminated by signal SIGABRT (Abort) ``` I reliably run into this every time. (Although if I comment out the assert, everything *seems* to work fine. Nevertheless, this suggests there's a bug somewhere, whether it be in this PR or in the BLI_map code.)

Iliya Katushenock commented

2024-04-22 13:51:17 +02:00

Probably this is the thing that i wrote about, but not sure\

Christoph Lendenfeld commented

2024-04-23 09:55:03 +02:00

thanks for catching that, not quite sure why that happens though. It doesn't happen right away for me though which is even weirder

Christoph Lendenfeld added 3 commits 2024-04-23 11:26:46 +02:00

a8d9975b79 Merge branch 'main' into optimize_beztmap_to_data

925cc07b36 move map out of threaded loop

e268aee4e4 remove timer

Christoph Lendenfeld added 1 commit 2024-04-23 14:04:06 +02:00

21e983377f Merge branch 'main' into optimize_beztmap_to_data

Sybren A. Stüvel requested review from Sybren A. Stüvel 2024-04-25 11:03:52 +02:00

Sybren A. Stüvel requested changes 2024-04-25 11:27:21 +02:00

Dismissed

Sybren A. Stüvel left a comment

This is a huge speedup, congrats!

This is a _huge_ speedup, congrats!

source/blender/editors/transform/transform_convert_graph.cc Outdated

						
				@ -803,0 +801,4 @@

				static inline void update_trans_data(TransData *td,

				                                     const FCurve *fcu,

				                                     const int new_index,

				                                     const int swap_handles)

Sybren A. Stüvel commented

2024-04-25 11:00:28 +02:00

Since swap_handles comes from BeztMap::swap_handles, it should be a short. Then again, since this function only deals with the 1 and "not 1" values, maybe this should be const bool do_swap_handles and then the call site can put swap_handles == 1 in there? Or have a BLI_assert(swap_handles >= 0) or something along those lines. To me it's too unclear, at this function, what the possible values for swap_handles would be.

But also see my note about this further down.

For another PR: it would be good to get rid of that short type altogether, and just use a 3-valued enum for instead.

Since `swap_handles` comes from `BeztMap::swap_handles`, it should be a `short`. Then again, since this function only deals with the `1` and "not `1`" values, maybe this should be `const bool do_swap_handles` and then the call site can put `swap_handles == 1` in there? Or have a `BLI_assert(swap_handles >= 0)` or something along those lines. To me it's too unclear, at this function, what the possible values for `swap_handles` would be. But also see my note about this further down. For another PR: it would be good to get rid of that `short` type altogether, and just use a 3-valued `enum` for instead.

ChrisLend marked this conversation as resolved

source/blender/editors/transform/transform_convert_graph.cc Outdated

						
				@ -803,3 +805,2 @@

				{

				  TransData2D *td2d;

				  TransData *td;

				  if (td->flag & TD_BEZTRIPLE && td->hdata) {

Sybren A. Stüvel commented

2024-04-25 11:05:32 +02:00

In which case would td->flag & TD_BEZTRIPLE be true and td->hdata false? Is that a valid, expected case? If not, then I think something like this would be better:

if ((td->flag & TD_BEZTRIPLE) == 0) {
  return;
}
BLI_assert_msg(td->hdata, "TD_BEZTRIPLE cannot exist without hdata");

In which case would `td->flag & TD_BEZTRIPLE` be true and `td->hdata` false? Is that a valid, expected case? If not, then I think something like this would be better: ```cpp if ((td->flag & TD_BEZTRIPLE) == 0) { return; } BLI_assert_msg(td->hdata, "TD_BEZTRIPLE cannot exist without hdata"); ```

Christoph Lendenfeld commented

2024-04-25 12:05:22 +02:00

I don't know honestly. I copied this code as is from beztmap_to_data. Ideally I'd like to avoid changing it with this PR for fear of introducing bugs and keeping the PR scope narrow.

I don't know honestly. I copied this code as is from `beztmap_to_data`. Ideally I'd like to avoid changing it with this PR for fear of introducing bugs and keeping the PR scope narrow.

👍 1

dr.sybren marked this conversation as resolved

source/blender/editors/transform/transform_convert_graph.cc Outdated

						
				@ -806,2 +816,3 @@

				}

				  TransDataContainer *tc = TRANS_DATA_CONTAINER_FIRST_SINGLE(t);

				/* This function firstly adjusts the pointers that the transdata has to each BezTriple. */

Sybren A. Stüvel commented

2024-04-25 11:12:21 +02:00

"firstly" implies a "secondly", but that seems to be missing. Also the "this function ..." can be removed, and shortened to something like "Adjust the pointers that ..."

👍 1

ChrisLend marked this conversation as resolved

source/blender/editors/transform/transform_convert_graph.cc Outdated

						
				@ -808,0 +824,4 @@

				  /* At this point, beztmaps are already sorted, so their current index is assumed to be what the

				   * BezTriple index will be after sorting. */

				  for (const int new_index : bezms.index_range()) {

				    const BeztMap *bezm = &bezms[new_index];

Sybren A. Stüvel commented

2024-04-25 11:13:21 +02:00

Does this have to be a pointer? Or could it be a reference?

👍 1

ChrisLend marked this conversation as resolved

source/blender/editors/transform/transform_convert_graph.cc Outdated

						
				@ -808,0 +827,4 @@

				    const BeztMap *bezm = &bezms[new_index];

				    if (new_index == bezm->oldIndex) {

				      /* If the index is the same,

				       * any pointers to BezTriple will still point to the correct data. */

Sybren A. Stüvel commented

2024-04-25 11:14:01 +02:00

This comment can be rewrapped.

👍 1

ChrisLend marked this conversation as resolved

source/blender/editors/transform/transform_convert_graph.cc Outdated

						
				@ -809,3 +834,1 @@

				  /* Used to mark whether an TransData's pointers have been fixed already, so that we don't

				   * override ones that are already done. */

				  blender::Vector<bool> adjusted(tc->data_len, false);

				    /* For the handles (vec[0]/vec[2]), we must also check if they need to be swapped. */

Sybren A. Stüvel commented

2024-04-25 11:14:41 +02:00

Isn't this also necessary when the bezm isn't moved index-wise?

Christoph Lendenfeld commented

2024-04-25 16:05:25 +02:00

hmmmm maybe.
Since I am not sure I will let any BeztMap pass where swap_handles is set.
Given that just means 3 more Map lookups and swap_handles is rarely set this shouldn't be a performance issue

hmmmm maybe. Since I am not sure I will let any BeztMap pass where `swap_handles` is set. Given that just means 3 more Map lookups and `swap_handles` is rarely set this shouldn't be a performance issue

👍 1

dr.sybren marked this conversation as resolved

source/blender/editors/transform/transform_convert_graph.cc Outdated

						
				@ -825,0 +838,4 @@

				    if (trans_data_map.contains(bezm->bezt->vec[0])) {

				      const int trans_data_index = trans_data_map.lookup(bezm->bezt->vec[0]);

				      td2d = &tc->data_2d[trans_data_index];

				      if (bezm->swap_handles == 1) {

Sybren A. Stüvel commented

2024-04-25 11:22:36 +02:00

Since this is effectively going over the values of an enum, I think a switch with three cases might be a nice approach here.

switch (bezm->swap_handles) {
  case 1:
    // .. do stuff
    break;
  case 0:
  case -1:
    // .. do stuff
    break;
}

It then becomes much more explicit that the 0 and -1 cases are handled equally. Or maybe they do get a separate case with a BLI_assert_unreachable() in the -1 one?

Since this is effectively going over the values of an enum, I think a `switch` with three `case`s might be a nice approach here. ```cpp switch (bezm->swap_handles) { case 1: // .. do stuff break; case 0: case -1: // .. do stuff break; } ``` It then becomes much more explicit that the `0` and `-1` cases are handled equally. Or maybe they do get a separate `case` with a `BLI_assert_unreachable()` in the `-1` one?

Nathan Vegdahl commented

2024-04-25 13:07:21 +02:00

Seconded.

Christoph Lendenfeld commented

2024-04-25 17:08:47 +02:00

given that swap handles is a bool after #121076: Refactor: swap handle logic in Graph Editor transform code I think this will stay an if

given that swap handles is a bool after [#121076: Refactor: swap handle logic in Graph Editor transform code](https://projects.blender.org/blender/blender/pulls/121076) I think this will stay an `if`

👍 1

dr.sybren marked this conversation as resolved

source/blender/editors/transform/transform_convert_graph.cc Outdated

						
				@ -846,0 +847,4 @@

				      td = &tc->data[trans_data_index];

				      update_trans_data(td, fcu, new_index, bezm->swap_handles);

				    }

				    if (trans_data_map.contains(bezm->bezt->vec[2])) {

Sybren A. Stüvel commented

2024-04-25 11:25:05 +02:00

This seems to be a copy of the above code. Probably better to extract that into a small function that takes the index into vec, and when swapping is necessary, uses 2 - that_vec_index.

This seems to be a copy of the above code. Probably better to extract that into a small function that takes the index into `vec`, and when swapping is necessary, uses `2 - that_vec_index`.

Nathan Vegdahl commented

2024-04-25 12:39:07 +02:00

I think I agree, but with the following caveats:

The resulting function would only ever be called from this function. At least, I can't think of other situations where it would be useful. So it seems very narrow and specific.
Relatedly, I have no idea what a useful non-obtuse name for it would be.

So I'd suggest making it a lambda defined just above its usage inside this function. That would make it clear that it's only called here, and would alleviate the importance of the naming issue (since you immediately see the context where it's used). And as a bonus, you can see all the relevant code for what this function does in one place, without having to jump around the file.

I think I agree, but with the following caveats: - The resulting function would only ever be called from this function. At least, I can't think of other situations where it would be useful. So it seems very narrow and specific. - Relatedly, I have no idea what a useful non-obtuse name for it would be. So I'd suggest making it a lambda defined just above its usage inside this function. That would make it clear that it's only called here, and would alleviate the importance of the naming issue (since you immediately see the context where it's used). And as a bonus, you can see all the relevant code for what this function does in one place, without having to jump around the file.

Christoph Lendenfeld commented

2024-04-25 17:19:53 +02:00

that's how that would look as a lambda. IMO this is less readable than having a bit of code duplication.

  inline auto update_handle = [](TransDataContainer *tc,
                                 const blender::Map<float *, int> &trans_data_map,
                                 const FCurve *fcu,
                                 const BeztMap &bezm,
                                 const int bezt_index,
                                 const short handle_index /* Either 0 or 2. */) {
    if (!trans_data_map.contains(bezm.bezt->vec[handle_index])) {
      return;
    }
    const int trans_data_index = trans_data_map.lookup(bezm.bezt->vec[handle_index]);
    TransData2D *td2d = &tc->data_2d[trans_data_index];
    if (bezm.swap_handles) {
      td2d->loc2d = fcu->bezt[bezt_index].vec[2 - handle_index];
    }
    else {
      td2d->loc2d = fcu->bezt[bezt_index].vec[handle_index];
    }
    TransData *td = &tc->data[trans_data_index];
    update_trans_data(td, fcu, bezt_index, bezm.swap_handles);
  };

that's how that would look as a lambda. IMO this is less readable than having a bit of code duplication. ```Cpp inline auto update_handle = [](TransDataContainer *tc, const blender::Map<float *, int> &trans_data_map, const FCurve *fcu, const BeztMap &bezm, const int bezt_index, const short handle_index /* Either 0 or 2. */) { if (!trans_data_map.contains(bezm.bezt->vec[handle_index])) { return; } const int trans_data_index = trans_data_map.lookup(bezm.bezt->vec[handle_index]); TransData2D *td2d = &tc->data_2d[trans_data_index]; if (bezm.swap_handles) { td2d->loc2d = fcu->bezt[bezt_index].vec[2 - handle_index]; } else { td2d->loc2d = fcu->bezt[bezt_index].vec[handle_index]; } TransData *td = &tc->data[trans_data_index]; update_trans_data(td, fcu, bezt_index, bezm.swap_handles); }; ```

Sybren A. Stüvel commented

2024-04-29 11:28:38 +02:00

I agree. Thanks @ChrisLend for checking this out, but it indeed isn't as nice as it was in my head ;-)

nathanvegdahl marked this conversation as resolved

source/blender/editors/transform/transform_convert_graph.cc Outdated

						
				@ -885,0 +891,4 @@

				  /* Build a map from the data that is being modified to its index. This is used to quickly update

				   * the pointers to where the data ends up after sorting. */

				  blender::Map<float *, int> trans_data_map;

				  for (int i = 0; i < tc->data_len; i++) {

Sybren A. Stüvel commented

2024-04-25 11:26:54 +02:00

It might be nice to extract this into a data_to_beztmap() function, and have that defined close to its counterpart beztmap_to_data().

It might be nice to extract this into a `data_to_beztmap()` function, and have that defined close to its counterpart `beztmap_to_data()`.

Nathan Vegdahl commented

2024-04-25 13:22:14 +02:00

I'm not sure I agree. beztmap_to_data() being split out makes sense to me because of the size of its code, so splitting it out makes the over-all flow of the code where it's called easier to see. But conceptually it's actually part of this function: it's only called from one place and only in this function, and it's highly specific to its call site. If it were significantly smaller, I would actually prefer it to be inlined.

This code has the same single-call-site specificity of beztmap_to_data(), but without the size to justify splitting it out. IMO splitting this out into a separate function would just force me to jump to another part of the file to see what's going on, without much benefit.

I'm not sure I agree. `beztmap_to_data()` being split out makes sense to me because of the size of its code, so splitting it out makes the over-all flow of the code where it's called easier to see. But conceptually it's actually part of this function: it's only called from one place and only in this function, and it's highly specific to its call site. If it were significantly smaller, I would actually prefer it to be inlined. This code has the same single-call-site specificity of `beztmap_to_data()`, but without the size to justify splitting it out. IMO splitting this out into a separate function would just force me to jump to another part of the file to see what's going on, without much benefit.

Christoph Lendenfeld commented

2024-04-25 16:14:21 +02:00

also not sure about that.
beztmap_to_data just uses the map and BeztMap to update the pointers
while this loop creates the map in the first place (without using a BeztMap).

imo that isn't symmetrical like data_to_beztmap suggests

also not sure about that. `beztmap_to_data` just uses the map and `BeztMap` to update the pointers while this loop creates the map in the first place (without using a `BeztMap`). imo that isn't symmetrical like `data_to_beztmap` suggests

Sybren A. Stüvel commented

2024-04-26 15:52:13 +02:00

Fair enough, never mind.

dr.sybren marked this conversation as resolved

Nathan Vegdahl reviewed 2024-04-25 14:26:34 +02:00

Nathan Vegdahl left a comment

I agree with most of Sybren's nits, but otherwise your changes look good to me!

As an aside, the over-all structure of the code in this area (nothing to do with your PR) feels unfortunate to me:

Creating a separate beztmap that gets sorted.
Then updating the trans data based on that sort.
Then doing another identical sort (with a separate function that has to preserve identical sorting behavior!) of the actual bezt data.

It feels both fragile and inefficient.

In any case, I don't think it's worth doing a larger refactor to change that. But I wanted to vent about it a bit anyway, ha ha.

I agree with most of Sybren's nits, but otherwise your changes look good to me! As an aside, the over-all structure of the code in this area (nothing to do with your PR) feels unfortunate to me: - Creating a separate beztmap that gets sorted. - Then updating the trans data based on that sort. - Then doing another identical sort (with a separate function that has to preserve identical sorting behavior!) of the actual bezt data. It feels both fragile and inefficient. In any case, I don't think it's worth doing a larger refactor to change that. But I wanted to vent about it a bit anyway, ha ha.

source/blender/editors/transform/transform_convert_graph.cc Outdated

						
				@ -806,2 +816,3 @@

				}

				  TransDataContainer *tc = TRANS_DATA_CONTAINER_FIRST_SINGLE(t);

				/* This function firstly adjusts the pointers that the transdata has to each BezTriple. */

Nathan Vegdahl commented

2024-04-25 13:23:38 +02:00

I had the same comment here as @dr.sybren, but he got to it before I finished my review. ;-) "Firstly" feels like the description is incomplete.

nathanvegdahl marked this conversation as resolved

Christoph Lendenfeld added 3 commits 2024-04-25 16:10:38 +02:00

69750b040c Merge branch 'main' into optimize_beztmap_to_data

cda38c24ee sybrens comments

3d6d0db50b let BeztMap pass if handles need to be swapped

Christoph Lendenfeld added 2 commits 2024-04-25 17:00:01 +02:00

769d7afd90 Merge branch 'main' into optimize_beztmap_to_data

a0cb7301a6 resolve merge issues

Christoph Lendenfeld added 1 commit 2024-04-25 17:23:55 +02:00

7936be3da4 remove confusing comment

Nathan Vegdahl commented

2024-04-26 10:32:55 +02:00

Something else that occurred to me: it might be worth taking this opportunity to rename beztmap_to_data(). The current name IMO doesn't reflect what it does at all. It sounds like it's creating completely new data from the beztmap, when in fact it's just updating the bezt pointers in the existing transform data. And the beztmap itself is almost incidental (it's just the means by which the actual bezt pointers get updated), rather than the purpose of the function.

Maybe something along the lines of update_transdata_bezt_pointers() would be better.

Something else that occurred to me: it might be worth taking this opportunity to rename `beztmap_to_data()`. The current name IMO doesn't reflect what it does at all. It sounds like it's creating completely new data from the beztmap, when in fact it's just updating the bezt pointers in the existing transform data. And the beztmap itself is almost incidental (it's just the means by which the actual bezt pointers get updated), rather than the purpose of the function. Maybe something along the lines of `update_transdata_bezt_pointers()` would be better.

👍 1

Christoph Lendenfeld commented

2024-04-26 10:37:05 +02:00

@nathanvegdahl I 100% agree. Should that be a refactor PR or is it ok to do that in this one?

Nathan Vegdahl commented

2024-04-26 13:35:16 +02:00

I guess technically it should be a separate refactor PR. But I don't feel strongly about it.

Christoph Lendenfeld added 2 commits 2024-04-26 14:15:18 +02:00

acc5a5ee0d Merge branch 'main' into optimize_beztmap_to_data

a63e39d80f resolve merg isseus

Christoph Lendenfeld commented

2024-04-26 14:15:36 +02:00

landed in a separate PR and merged

Sybren A. Stüvel approved these changes 2024-04-29 11:30:17 +02:00

Sybren A. Stüvel left a comment

🚀

Iliya Katushenock reviewed 2024-04-29 11:33:49 +02:00

source/blender/editors/transform/transform_convert_graph.cc Outdated

						
				@ -809,3 +822,1 @@

				      /* Skip item if already marked. */

				      if (adjusted[j]) {

				        continue;

				    if (trans_data_map.contains(bezm.bezt->vec[0])) {

Iliya Katushenock commented

2024-04-29 11:33:49 +02:00

To avoid contains and lookup for the same key:

if (const int *trans_data_index = trans_data_map.lookup_ptr(bezm.bezt->vec[0])) {
  ...
)

To avoid `contains` and `lookup` for the same key: ```Cpp if (const int *trans_data_index = trans_data_map.lookup_ptr(bezm.bezt->vec[0])) { ... ) ```

👍 1

Nathan Vegdahl approved these changes 2024-04-30 10:10:53 +02:00

Nathan Vegdahl left a comment

Aside from @mod_moder 's comment (good catch!) about avoiding redundant contains()+lookup(), looks good to me!

Aside from @mod_moder 's comment (good catch!) about avoiding redundant `contains()`+`lookup()`, looks good to me!

Christoph Lendenfeld added 2 commits 2024-04-30 10:23:11 +02:00

3a6c3619c6 Merge branch 'main' into optimize_beztmap_to_data

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

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

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

a9d01f6909 avoid double map lookup

Christoph Lendenfeld commented

2024-04-30 10:24:06 +02:00

@blender-bot build

Christoph Lendenfeld referenced this issue from a commit

2024-04-30 10:47:00 +02:00

Anim: Graph Editor - use Map to update TransInfo pointers instead of searching

Christoph Lendenfeld merged commit c6c7d3d8c4 into main

2024-04-30 10:47:00 +02:00

Christoph Lendenfeld deleted branch optimize_beztmap_to_data

2024-04-30 10:47:03 +02:00

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

Anim: Graph Editor - use Map to update TransInfo pointers instead of searching #120816