VSE: use c++ container for strip iterator #111909

Richard Antalik · 2023-09-04T03:33:28+02:00

Richard Antalik commented

2023-09-04 03:33:28 +02:00

Idea is to use blender::VectorSet instead of SeqCollection struct. This makes it possible to use native for loops, so SEQ_ITERATOR_FOREACH macro can be removed.
Another feature is, sets of strips no longer needs to be freed. However this poses a limitation, that query functions can not be used in case where these sets need to be available outside of scope where they are created.

Idea is to use blender::VectorSet instead of `SeqCollection` struct. This makes it possible to use native for loops, so `SEQ_ITERATOR_FOREACH` macro can be removed. Another feature is, sets of strips no longer needs to be freed. However this poses a limitation, that query functions can not be used in case where these sets need to be available outside of scope where they are created.

Richard Antalik added 4 commits 2023-09-04 03:33:38 +02:00

e4bdf33274 VSE: Use `blender::Vector` for iterator

f8a1acf657 Experiment - proper storage for object

c72785fadb Merge branch 'main' into iterator-cpp

a473cdb5c5 continue refactoring

Richard Antalik changed title from ~~[WIP} VSE use c++ conteiner for strip iterator~~ to [WIP] VSE use c++ conteiner for strip iterator

2023-09-04 03:33:47 +02:00

Richard Antalik added 1 commit 2023-09-04 04:18:26 +02:00

82dda78256 Refactor Vector -> VectorSet

Richard Antalik added 2 commits 2023-09-04 07:42:51 +02:00

b3ba1ff5e0 Rename occurences of seqCollection

8edd4b5c15 Fix RNA collection iterator

Hans Goudey changed title from ~~[WIP] VSE use c++ conteiner for strip iterator~~ to WIP: VSE: use c++ container for strip iterator

2023-09-04 20:45:55 +02:00

Hans Goudey commented

2023-09-04 20:50:01 +02:00

Just looking at this out of curiousity-- it's generally preferred to use references instead of pointers for arguments that aren't expected to be null. For example, VectorSet<Sequence *> *strips_dst should become VectorSet<Sequence *> &strips_dst. That helps simplify using the argument in the function.

Just looking at this out of curiousity-- it's generally preferred to use references instead of pointers for arguments that aren't expected to be null. For example, `VectorSet<Sequence *> *strips_dst` should become `VectorSet<Sequence *> &strips_dst`. That helps simplify using the argument in the function.

Richard Antalik commented

2023-09-05 01:15:32 +02:00

@HooglyBoogly Thanks for feedback, I was thinking actually about involving you in review. if you would like to do it, feel free to assign yourself.
Will have to read about reference vs. pointer. I am still very new to C++.

As I am reading about references, there are few places where I rely on returning nullptr instead of pointer to Vector or VectorSet. I remember struggling a bit to refine logic so this can be avoided, but I may keep using pointers in such case.

@HooglyBoogly Thanks for feedback, I was thinking actually about involving you in review. if you would like to do it, feel free to assign yourself. Will have to read about reference vs. pointer. I am still very new to C++. As I am reading about references, there are few places where I rely on returning `nullptr` instead of pointer to `Vector` or `VectorSet`. I remember struggling a bit to refine logic so this can be avoided, but I may keep using pointers in such case.

Hans Goudey commented

2023-09-05 01:31:46 +02:00

As I am reading about references, there are few places where I rely on returning nullptr instead of pointer to Vector or VectorSet. I remember struggling a bit to refine logic so this can be avoided, but I may keep using pointers in such case.

How about returning an empty container? That can be done with just return {}; Maybe it doesn't work, but maybe it does?

> As I am reading about references, there are few places where I rely on returning `nullptr` instead of pointer to `Vector` or `VectorSet`. I remember struggling a bit to refine logic so this can be avoided, but I may keep using pointers in such case. How about returning an empty container? That can be done with just `return {};` Maybe it doesn't work, but maybe it does?

Hans Goudey requested review from Hans Goudey 2023-09-05 01:31:55 +02:00

Richard Antalik commented

2023-09-05 01:52:36 +02:00

One place I think is in lookup cache, where I gather effects of a strip, so empty container could mean no effects, but nullptr means , that cache is not initialized.
Now this cache is built for all strips at once, so I can assume empty container means no effects, but it was an example of how this could be an issue.
In any case, will change pointers to references and report on any issues I found.

One place I think is in lookup cache, where I gather effects of a strip, so empty container could mean no effects, but `nullptr` means , that cache is not initialized. Now this cache is built for all strips at once, so I can assume empty container means no effects, but it was an example of how this could be an issue. In any case, will change pointers to references and report on any issues I found.

Richard Antalik commented

2023-09-05 05:22:25 +02:00

@HooglyBoogly I have started to replace pointers with references, and I have identified 2 obstacles:

First is keeping object available in some context struct:

typedef struct Ctx{
  Vector <T> *vec;
} Ctx;

Ctx *init_ctx(void){
  c->vec = new Vector <T>;
  return c;
}

I can't use reference there, since I need c to exist until I decide to free it.

Second obstacle is above mentioned lookup cache for effects:

This cache is blender::Map<const Sequence *, blender::VectorSet<Sequence *>> The VectorSet contains effect strips of Maps key which is input strip. This cache is designed, so it iterates over strips once.
But since strips can have many effects and ListBase is not ordered, I would have to iterate twice to create VectorSets first and then add effects to them in second iteration. Also this would result in unnecessary memory allocations.

So far I have been able to resolve this by using pointers internally and returning reference. So far to return empty set, I have declared global empty_set object. I guess there is some magic string to cast it inline, but I wasn't able to do it.

@HooglyBoogly I have started to replace pointers with references, and I have identified 2 obstacles: First is keeping object available in some context struct: ``` typedef struct Ctx{ Vector <T> *vec; } Ctx; Ctx *init_ctx(void){ c->vec = new Vector <T>; return c; } ``` I can't use reference there, since I need `c` to exist until I decide to free it. Second obstacle is above mentioned lookup cache for effects: This cache is `blender::Map<const Sequence *, blender::VectorSet<Sequence *>>` The `VectorSet` contains effect strips of `Map`s key which is input strip. This cache is designed, so it iterates over strips once. But since strips can have many effects and `ListBase` is not ordered, I would have to iterate twice to create VectorSets first and then add effects to them in second iteration. Also this would result in unnecessary memory allocations. So far I have been able to resolve this by using pointers internally and returning reference. So far to return empty set, I have declared global `empty_set` object. I guess there is some magic string to cast it inline, but I wasn't able to do it.

ok what referenced this pull request

2023-09-05 17:41:11 +02:00

Fix memory leak in the VSE "Frame Selected" operator #111987

Iliya Katushenock commented

2023-09-06 01:55:21 +02:00

Why don't you use smart pointers and constructors? What about optional return values?

Richard Antalik commented

2023-09-06 05:59:17 +02:00

Why don't you use smart pointers and constructors?

I am not sure how smart pointers would help here. I need object to be allocated on heap anyway. I can use it if that is preffered to plain pointers.

I looked at node code where similar case is handled, and I think, that the answer is MEM_new() function, which can initialize non-trivial objects.

What about optional return values?

Turns out, that I wasn't supposed to return reference to object, then return {} worked.

However there are few places where empty set is supplied to SEQ_transform_handle_overlap() as argument. So far I do

      blender::VectorSet<Sequence *> empty_set;
      SEQ_transform_handle_overlap(scene, ed->seqbasep, strip_col, empty_set, use_sync_markers);

But I can make variant of this function where it does not use this argument, since I know in advance, that the set will be empty. That is much nicer solution.

> Why don't you use smart pointers and constructors? I am not sure how smart pointers would help here. I need object to be allocated on heap anyway. I can use it if that is preffered to plain pointers. I looked at node code where similar case is handled, and I think, that the answer is `MEM_new()` function, which can initialize non-trivial objects. > What about optional return values? Turns out, that I wasn't supposed to return reference to object, then `return {}` worked. However there are few places where empty set is supplied to `SEQ_transform_handle_overlap()` as argument. So far I do ``` blender::VectorSet<Sequence *> empty_set; SEQ_transform_handle_overlap(scene, ed->seqbasep, strip_col, empty_set, use_sync_markers); ``` But I can make variant of this function where it does not use this argument, since I know in advance, that the set will be empty. That is much nicer solution.

Richard Antalik added 2 commits 2023-09-06 06:28:20 +02:00

736435ac91 Fix uninitialized object

4a5219b128 Merge branch 'main' into iterator-cpp

Richard Antalik added 1 commit 2023-09-06 06:33:40 +02:00

04962a714f Use references instead of pointers

Richard Antalik added 1 commit 2023-09-06 07:26:02 +02:00

ac85f39a33 Fix docstrings

Richard Antalik changed title from ~~WIP: VSE: use c++ container for strip iterator~~ to VSE: use c++ container for strip iterator

2023-09-06 07:26:36 +02:00

Richard Antalik added 1 commit 2023-09-08 13:20:55 +02:00

2f218daaf8 Merge branch 'main' into iterator-cpp

Hans Goudey requested changes 2023-09-08 14:38:13 +02:00

Hans Goudey left a comment

Nice to remove these single-purpose data structures that are unnecessary in C++ code! A few of my comments apply everywhere in the patch, particularly using Span where possible and avoiding auto.

Nice to remove these single-purpose data structures that are unnecessary in C++ code! A few of my comments apply everywhere in the patch, particularly using `Span` where possible and avoiding `auto`.

source/blender/editors/animation/anim_ops.cc Outdated

						
				@ -113,3 +113,2 @@

				  int best_distance = MAXFRAME;

				  Sequence *seq;

				  SEQ_ITERATOR_FOREACH (seq, strips) {

				  for (auto seq : strips) {

Hans Goudey commented

2023-09-08 14:05:00 +02:00

Usually we don't use auto for cases like this, since the type name is helpful to the reader and clarifies what you can do with the variable.

Usually we don't use `auto` for cases like this, since the type name is helpful to the reader and clarifies what you can do with the variable.

iss marked this conversation as resolved

source/blender/editors/space_sequencer/sequencer_add.cc Outdated

						
				@ -780,3 +778,3 @@

				}

				static void seq_build_proxy(bContext *C, SeqCollection *movie_strips)

				static void seq_build_proxy(bContext *C, blender::VectorSet<Sequence *> &movie_strips)

Hans Goudey commented

2023-09-08 14:05:42 +02:00

Functions like this should take a Span so that they aren't dependent on which container the data is actually stored in.

Same for any other function in this PR that doesn't modify or take ownership of the set.

Functions like this should take a `Span` so that they aren't dependent on which container the data is actually stored in. Same for any other function in this PR that doesn't modify or take ownership of the set.

iss marked this conversation as resolved

source/blender/editors/space_sequencer/sequencer_add.cc Outdated

						
				@ -885,3 +878,3 @@

				      const bool use_sync_markers = (((SpaceSeq *)area->spacedata.first)->flag &

				                                     SEQ_MARKER_TRANS) != 0;

				      SEQ_transform_handle_overlap(scene, ed->seqbasep, strip_col, nullptr, use_sync_markers);

				      {

Hans Goudey commented

2023-09-08 14:06:50 +02:00

It's not clear why the braces are added here.

iss marked this conversation as resolved

source/blender/editors/space_sequencer/sequencer_edit.cc Outdated

						
				@ -2249,3 +2247,3 @@

				        case SEQ_SIDE_RIGHT:

				          if (SEQ_time_left_handle_frame_get(scene, seq) >=

				              SEQ_time_right_handle_frame_get(scene, test)) {

				              SEQ_time_right_handle_frame_get(scene, test))

Hans Goudey commented

2023-09-08 14:07:48 +02:00

Looks like you're using the wrong clang format version?

iss marked this conversation as resolved

source/blender/editors/space_sequencer/sequencer_intern.h Outdated

						
				@ -164,3 +161,3 @@

				 * \return collection of strips (`Sequence`)

				 */

				struct SeqCollection *all_strips_from_context(struct bContext *C);

				blender::VectorSet<Sequence *> all_strips_from_context(struct bContext *C);

Hans Goudey commented

2023-09-08 14:28:36 +02:00

I moved this header to C++ in main, that way this PR doesn't need to remove the extern "C" itself.

I moved this header to C++ in main, that way this PR doesn't need to remove the `extern "C"` itself.

iss marked this conversation as resolved

source/blender/editors/space_sequencer/sequencer_retiming.cc Outdated

						
				@ -67,0 +63,4 @@

				  strips.add(seq);

				  blender::VectorSet<Sequence *> dependant;

				  SEQ_iterator_set_expand(scene, seqbase, strips, SEQ_query_strip_effect_chain);

				  dependant.remove(0);

Hans Goudey commented

2023-09-08 14:29:09 +02:00

remove(0) seems suspect, since 0 is not a pointer. What's the goal there?

`remove(0)` seems suspect, since `0` is not a pointer. What's the goal there?

Richard Antalik commented

2023-09-13 17:28:14 +02:00

Eeh, this seems to be incorrect even in original code.

The goal is to have 2 sets of strips - strips can be transformed freely, dependant are "children" of strips in strips set and can be transformed in Y direction only. SEQ_transform_handle_overlap() then does the magic according to these restrictions.

Eeh, this seems to be incorrect even in original code. The goal is to have 2 sets of strips - `strips` can be transformed freely, `dependant` are "children" of strips in `strips` set and can be transformed in Y direction only. `SEQ_transform_handle_overlap()` then does the magic according to these restrictions.

iss marked this conversation as resolved

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

						
				@ -328,1 +327,3 @@

				  SEQ_filter_selected_strips(strips);

				  blender::VectorSet strips = SEQ_query_rendered_strips(

				      scene, channels, seqbase, scene->r.cfra, 0);

				  strips.remove_if([&](auto seq) { return (seq->flag & SELECT) == 0; });

Hans Goudey commented

2023-09-08 14:31:15 +02:00

I realize this is just porting the existing code, it does seem a bit inefficient to build a container of all the strips and then just filter it to the selected ones. It might be just one strip selected with hundreds visible for example. Feel free to ignore this for later though!

Richard Antalik commented

2023-09-13 17:33:28 +02:00

SEQ_query_rendered_strips() does not return all strips. It returns strips that will be rendered at particular frame.
Without some form of caching, this is IMO most efficient way to get the result I need.

`SEQ_query_rendered_strips()` does not return all strips. It returns strips that will be rendered at particular frame. Without some form of caching, this is IMO most efficient way to get the result I need.

Hans Goudey commented

2023-09-13 17:44:35 +02:00

My point is, doing the filter selection inside of SEQ_query_rendered_strips would be more efficient, since we don't build a large VectorSet just to remove most of the items. As long as we're agreed on that, I think both options are valid-- there's an argument to be made about code clarity too.

My point is, doing the filter selection inside of `SEQ_query_rendered_strips` would be more efficient, since we don't build a large `VectorSet` just to remove most of the items. As long as we're agreed on that, I think both options are valid-- there's an argument to be made about code clarity too.

Richard Antalik commented

2023-09-13 18:26:35 +02:00

Not sure if I understand, you mean to move filtering of selected items into function SEQ_query_rendered_strips? I can't do that, because that function is used in bunch of places where it needs to provide unselected strips as well.

Not sure if I understand, you mean to move filtering of selected items into function `SEQ_query_rendered_strips`? I can't do that, because that function is used in bunch of places where it needs to provide unselected strips as well.

Hans Goudey commented

2023-09-14 17:15:04 +02:00

I can't do that, because that function is used in bunch of places

I mean as an option, not all the time. But anyway, just want us to be clear on the tradeoff.

>I can't do that, because that function is used in bunch of places I mean as an option, not all the time. But anyway, just want us to be clear on the tradeoff.

Richard Antalik commented

2023-09-14 17:35:21 +02:00

I understand what you mean and I started to think about having one query function, that you can specify a bunch of pre-defined conditions. But ultimately I think it would be harder to read the code and understand what the function is supposed to do. But at the same time perhaps closer to my original goal. But after some time I started to more appreciate just having a container I can fill as I want to, than having a library of calls to do one predefined thing.

👍 1

iss marked this conversation as resolved

source/blender/makesrna/intern/rna_sequencer.cc Outdated

						
				@ -216,0 +224,4 @@

				  }

				}

				typedef struct SequencesAllIterator {

Hans Goudey commented

2023-09-08 14:32:06 +02:00

typedef is unnecessary in this situation in C++

`typedef` is unnecessary in this situation in C++

iss marked this conversation as resolved

source/blender/sequencer/intern/iterator.cc Outdated

						
				@ -143,2 +65,3 @@

				void SEQ_collection_exclude(SeqCollection *collection, SeqCollection *exclude_elements)

				// This should be renamed if it is still needed.

				void SEQ_iterator_set_merge(VectorSet<Sequence *> &strips_dst, VectorSet<Sequence *> strips_src)

Hans Goudey commented

2023-09-08 14:33:18 +02:00

I'd suggest removing this and replacing its uses with strips_dst.add_multiple(strips_src);

I'd suggest removing this and replacing its uses with `strips_dst.add_multiple(strips_src);`

Richard Antalik commented

2023-09-13 17:35:51 +02:00

Ah nice, I wanted to suggest this feature, but it actually exists.

iss marked this conversation as resolved

source/blender/sequencer/intern/sequence_lookup.cc Outdated

						
				@ -30,3 +33,3 @@

				  GHash *seq_by_name;

				  GHash *meta_by_seq;

				  GHash *effects_by_seq;

				  blender::Map<const Sequence *, blender::VectorSet<Sequence *> *> *effects_by_seq;

Hans Goudey commented

2023-09-08 14:35:05 +02:00

The VectorSet here shouldn't need to be a pointer. If it does need to be a pointer, it should probably use std::unique_ptr. We try to avoid raw new and delete whenever possible (also MEM_new and MEM_delete too), since that sort of manual memory management is ideally unnecessary in C++.

The VectorSet here shouldn't need to be a pointer. If it does need to be a pointer, it should probably use `std::unique_ptr`. We try to avoid raw `new` and `delete` whenever possible (also `MEM_new` and `MEM_delete` too), since that sort of manual memory management is ideally unnecessary in C++.

Richard Antalik commented

2023-09-13 18:10:13 +02:00

I tried to use std::unique_ptr, but compiler had some issues around lookup_default(). In any case, does the smart pointer know, that the object is still used when it is added to Map?

I tried to use `std::unique_ptr`, but compiler had some issues around `lookup_default()`. In any case, does the smart pointer know, that the object is still used when it is added to `Map`?

Hans Goudey commented

2023-09-13 18:26:17 +02:00

I think you'll have to be more specific about what the issues were

Richard Antalik commented

2023-09-13 18:29:12 +02:00

Wanted to check on this tomorrow more in detail, but what I did was (on top of current patch):

diff --git a/source/blender/sequencer/intern/sequence_lookup.cc b/source/blender/sequencer/intern/sequence_lookup.cc
index 5b028ab40a0..4f1a802e1c0 100644
--- a/source/blender/sequencer/intern/sequence_lookup.cc
+++ b/source/blender/sequencer/intern/sequence_lookup.cc
@@ -32,7 +32,7 @@ static ThreadMutex lookup_lock = BLI_MUTEX_INITIALIZER;
 struct SequenceLookup {
   GHash *seq_by_name;
   GHash *meta_by_seq;
-  blender::Map<const Sequence *, blender::VectorSet<Sequence *> *> *effects_by_seq;
+  blender::Map<const Sequence *, std::unique_ptr<blender::VectorSet<Sequence *>>> *effects_by_seq;
   eSequenceLookupTag tag;
 };

@@ -40,7 +40,8 @@ static void seq_sequence_lookup_init(SequenceLookup *lookup)
 {
   lookup->seq_by_name = BLI_ghash_str_new(__func__);
   lookup->meta_by_seq = BLI_ghash_ptr_new(__func__);
-  lookup->effects_by_seq = new blender::Map<const Sequence *, blender::VectorSet<Sequence *> *>;
+  lookup->effects_by_seq =
+      new blender::Map<const Sequence *, std::unique_ptr<blender::VectorSet<Sequence *>>>;
   lookup->tag |= SEQ_LOOKUP_TAG_INVALID;
 }

@@ -52,10 +53,11 @@ static void seq_sequence_lookup_append_effect(const Sequence *input,
     return;
   }

-  blender::VectorSet<Sequence *> *effects = lookup->effects_by_seq->lookup_default(input, nullptr);
+  std::unique_ptr<blender::VectorSet<Sequence *>> effects =
+      lookup->effects_by_seq->lookup_default(input, nullptr);

   if (effects == nullptr) {
-    effects = new blender::VectorSet<Sequence *>;
+    effects = std::make_unique<blender::VectorSet<Sequence *>>();
     lookup->effects_by_seq->add(input, effects);
   }

@@ -179,7 +181,8 @@ blender::Span<Sequence *> seq_sequence_lookup_effects_by_seq(const Scene *scene,
   BLI_mutex_lock(&lookup_lock);
   seq_sequence_lookup_update_if_needed(scene, &scene->ed->runtime.sequence_lookup);
   SequenceLookup *lookup = scene->ed->runtime.sequence_lookup;
-  blender::VectorSet<Sequence *> *effects = lookup->effects_by_seq->lookup_default(key, nullptr);
+  std::unique_ptr<blender::VectorSet<Sequence *>> effects = lookup->effects_by_seq->lookup_default(
+      key, nullptr);

   if (effects == nullptr) {
     BLI_mutex_unlock(&lookup_lock);

Wanted to check on this tomorrow more in detail, but what I did was (on top of current patch): ```Diff diff --git a/source/blender/sequencer/intern/sequence_lookup.cc b/source/blender/sequencer/intern/sequence_lookup.cc index 5b028ab40a0..4f1a802e1c0 100644 --- a/source/blender/sequencer/intern/sequence_lookup.cc +++ b/source/blender/sequencer/intern/sequence_lookup.cc @@ -32,7 +32,7 @@ static ThreadMutex lookup_lock = BLI_MUTEX_INITIALIZER; struct SequenceLookup { GHash *seq_by_name; GHash *meta_by_seq; - blender::Map<const Sequence *, blender::VectorSet<Sequence *> *> *effects_by_seq; + blender::Map<const Sequence *, std::unique_ptr<blender::VectorSet<Sequence *>>> *effects_by_seq; eSequenceLookupTag tag; }; @@ -40,7 +40,8 @@ static void seq_sequence_lookup_init(SequenceLookup *lookup) { lookup->seq_by_name = BLI_ghash_str_new(__func__); lookup->meta_by_seq = BLI_ghash_ptr_new(__func__); - lookup->effects_by_seq = new blender::Map<const Sequence *, blender::VectorSet<Sequence *> *>; + lookup->effects_by_seq = + new blender::Map<const Sequence *, std::unique_ptr<blender::VectorSet<Sequence *>>>; lookup->tag |= SEQ_LOOKUP_TAG_INVALID; } @@ -52,10 +53,11 @@ static void seq_sequence_lookup_append_effect(const Sequence *input, return; } - blender::VectorSet<Sequence *> *effects = lookup->effects_by_seq->lookup_default(input, nullptr); + std::unique_ptr<blender::VectorSet<Sequence *>> effects = + lookup->effects_by_seq->lookup_default(input, nullptr); if (effects == nullptr) { - effects = new blender::VectorSet<Sequence *>; + effects = std::make_unique<blender::VectorSet<Sequence *>>(); lookup->effects_by_seq->add(input, effects); } @@ -179,7 +181,8 @@ blender::Span<Sequence *> seq_sequence_lookup_effects_by_seq(const Scene *scene, BLI_mutex_lock(&lookup_lock); seq_sequence_lookup_update_if_needed(scene, &scene->ed->runtime.sequence_lookup); SequenceLookup *lookup = scene->ed->runtime.sequence_lookup; - blender::VectorSet<Sequence *> *effects = lookup->effects_by_seq->lookup_default(key, nullptr); + std::unique_ptr<blender::VectorSet<Sequence *>> effects = lookup->effects_by_seq->lookup_default( + key, nullptr); if (effects == nullptr) { BLI_mutex_unlock(&lookup_lock); ```

Richard Antalik commented

2023-09-14 17:25:46 +02:00

I have looked bit more into this, and I am affraid I won't be able to resolve this issue. The compiler says something like "Attempted to reference removed function" probably when compiling lookup->effects_by_seq->lookup_default(input, nullptr)

I have looked bit more into this, and I am affraid I won't be able to resolve this issue. The compiler says something like "Attempted to reference removed function" probably when compiling `lookup->effects_by_seq->lookup_default(input, nullptr)`

source/blender/sequencer/intern/sequence_lookup.cc Outdated

						
				@ -112,3 +115,3 @@

				  (*lookup)->meta_by_seq = nullptr;

				  (*lookup)->effects_by_seq = nullptr;

				  //(*lookup)->effects_by_seq = nullptr;

				  MEM_freeN(*lookup);

Hans Goudey commented

2023-09-08 14:35:51 +02:00

SequenceLookup is now a non-trivial struct, and allocating it needs to be done with MEM_new, and freeing with MEM_delete (or smart pointers, etc).

`SequenceLookup` is now a non-trivial struct, and allocating it needs to be done with `MEM_new`, and freeing with `MEM_delete` (or smart pointers, etc).

iss marked this conversation as resolved

source/blender/sequencer/intern/sequence_lookup.cc Outdated

						
				@ -171,3 +174,3 @@

				}

				SeqCollection *seq_sequence_lookup_effects_by_seq(const Scene *scene, const Sequence *key)

				blender::VectorSet<Sequence *> seq_sequence_lookup_effects_by_seq(const Scene *scene,

Hans Goudey commented

2023-09-08 14:36:59 +02:00

This returns a copy of the container stored in the map. Maybe it can return a span instead and just reference the data in the map?

iss marked this conversation as resolved

Richard Antalik added 4 commits 2023-09-13 18:18:15 +02:00

bae2255bdf review

1d731bffa6 Merge branch 'main' into iterator-cpp

de0eccc972 review

30c1766f34 auto -> Sequence *

Richard Antalik added 1 commit 2023-09-14 17:09:25 +02:00

23b81ba500 Use span arguments where possible, minor fixes

Richard Antalik added 1 commit 2023-10-04 17:08:39 +02:00

4ea6799172 Merge branch 'iterator-cpp' of https://projects.blender.org/iss/blender into iterator-cpp

Richard Antalik added 1 commit 2023-10-04 18:30:50 +02:00

b8835d6baa Make `effects_by_seq` not pointer

Richard Antalik added 2 commits 2023-10-05 14:26:30 +02:00

f94672dcac Use lookup_or_add_default instead of explicitly creating VectorSet

375ad908ef format

Richard Antalik requested review from Hans Goudey 2023-10-17 00:56:54 +02:00

Hans Goudey requested changes 2023-10-17 23:08:30 +02:00

Hans Goudey left a comment

Just the change in data structures is really nice here. It all becomes much more obvious, nice job.

All headers affected by this cleanup that now have templates in them should be moved to .hh and have their extern "C" blocks removed in a separate commit that lands in main before this. That way there is no half-way situation where we're using C++ features in a C header.

Just the change in data structures is really nice here. It all becomes much more obvious, nice job. All headers affected by this cleanup that now have templates in them should be moved to `.hh` and have their `extern "C"` blocks removed in a separate commit that lands in main before this. That way there is no half-way situation where we're using C++ features in a C header.

source/blender/editors/animation/anim_ops.cc Outdated

						
				@ -108,3 +108,3 @@

				  ListBase *seqbase = SEQ_active_seqbase_get(SEQ_editing_get(scene));

				  SeqCollection *strips = SEQ_query_all_strips(seqbase);

				  blender::Span<Sequence *> strips = SEQ_query_all_strips(seqbase);

Hans Goudey commented

2023-10-17 22:55:24 +02:00

Did you test this? I wouldn't expect this to work.

The VectorSet returned by SEQ_query_all_strips needs to be stored in a variable so it's not immediately destructed.
The Span will fill itself with the memory in the VectorSet, but crucially it doesn't own the memory, which will be freed with the VectorSet.

Spans are non-owning references to memory from elsewhere. In C++ (and C too) it's the programmer's responsibility to think about the lifetime and ownership of data to avoid this problem.

Did you test this? I wouldn't expect this to work. The `VectorSet` returned by `SEQ_query_all_strips` needs to be stored in a variable so it's not immediately destructed. The `Span` will fill itself with the memory in the `VectorSet`, but crucially it doesn't _own_ the memory, which will be freed with the `VectorSet`. Spans are non-owning references to memory from elsewhere. In C++ (and C too) it's the programmer's responsibility to think about the lifetime and ownership of data to avoid this problem.

Richard Antalik commented

2023-10-19 02:30:00 +02:00

I guess this could be compiler dependent behavior. It works here, but will play it safe.

Hans Goudey commented

2023-10-19 09:34:04 +02:00

I'm fairly certain it's undefined behavior in every compiler. it's not "playing it safe" to change this, just fixing broken code.

iss marked this conversation as resolved

source/blender/editors/space_sequencer/sequencer_add.cc Outdated

						
				@ -834,3 +826,1 @@

				  if (overlap_shuffle_override) {

				    strip_col = SEQ_collection_create(__func__);

				  }

				  blender::VectorSet<Sequence *> added_strips;

Hans Goudey commented

2023-10-17 22:57:03 +02:00

It's clearer to only use the VectorSet if you actually need de-duplication Since this only gets two items added to it, I think it's not necessary here, so a plain Vector is probably a better choice.

It's clearer to only use the `VectorSet` if you actually need de-duplication Since this only gets two items added to it, I think it's not necessary here, so a plain `Vector` is probably a better choice.

iss marked this conversation as resolved

source/blender/editors/space_sequencer/sequencer_add.cc Outdated

						
				@ -971,3 +958,3 @@

				  }

				  if (SEQ_collection_len(movie_strips) == 0) {

				  if (movie_strips.size() == 0) {

Hans Goudey commented

2023-10-17 22:57:26 +02:00

.is_empty() should always be used instead of checking if the size is zero

`.is_empty()` should always be used instead of checking if the size is zero

iss marked this conversation as resolved

source/blender/editors/space_sequencer/sequencer_intern.hh Outdated

						
				@ -154,3 +154,3 @@

				 * \return collection of strips (`Sequence`)

				 */

				SeqCollection *all_strips_from_context(bContext *C);

				blender::VectorSet<Sequence *> all_strips_from_context(struct bContext *C);

Hans Goudey commented

2023-10-17 22:58:00 +02:00

No need to add struct back in the definition here

No need to add `struct` back in the definition here

Richard Antalik commented

2023-10-19 02:45:49 +02:00

I never understood the logic when I need to add struct to declarations... Sometime compiler complains, sometimes it doesn't, so I add it everywhere :/

I never understood the logic when I need to add `struct` to declarations... Sometime compiler complains, sometimes it doesn't, so I add it everywhere :/

iss marked this conversation as resolved

source/blender/editors/space_sequencer/sequencer_intern.hh Outdated

						
				@ -164,3 +164,3 @@

				 * \return collection of strips (`Sequence`)

				 */

				SeqCollection *selected_strips_from_context(bContext *C);

				blender::VectorSet<Sequence *> selected_strips_from_context(struct bContext *C);

Hans Goudey commented

2023-10-17 22:58:06 +02:00

Same here

iss marked this conversation as resolved

source/blender/editors/space_sequencer/sequencer_retiming.cc Outdated

						
				@ -223,3 +222,3 @@

				static int retiming_key_add_from_selection(bContext *C,

				                                           wmOperator *op,

				                                           SeqCollection *strips,

				                                           blender::VectorSet<Sequence *> &strips,

Hans Goudey commented

2023-10-17 22:59:09 +02:00

Pass this as a Span<Sequence *> if you don't have to add items.

Pass this as a `Span<Sequence *>` if you don't have to add items.

iss marked this conversation as resolved

source/blender/sequencer/intern/sequence_lookup.cc Outdated

						
				@ -108,3 +104,3 @@

				  BLI_ghash_free((*lookup)->seq_by_name, nullptr, nullptr);

				  BLI_ghash_free((*lookup)->meta_by_seq, nullptr, nullptr);

				  BLI_ghash_free((*lookup)->effects_by_seq, nullptr, SEQ_collection_free_void_p);

				  // BLI_ghash_free((*lookup)->effects_by_seq, nullptr, nullptr);

Hans Goudey commented

2023-10-17 23:04:29 +02:00

What's the status of this commented code? Looks as if the change is incomplete here?

Richard Antalik commented

2023-10-19 02:48:08 +02:00

Forgot to remove.

iss marked this conversation as resolved

Richard Antalik added 1 commit 2023-10-19 04:01:39 +02:00

25478bfdfb Address review

Richard Antalik added 1 commit 2023-10-19 04:02:48 +02:00

42cde2605e format

Richard Antalik requested review from Hans Goudey 2023-10-19 04:03:58 +02:00

Hans Goudey requested changes 2023-10-19 09:36:10 +02:00

Hans Goudey left a comment

Like I mentioned above, the C++ conversion of the headers (removing extern "C") and changing to .hh should be committed to main separately outside of this review. The change makes the PR too big and difficult to review properly, and combines one large trivial change with a more complex one.

Richard Antalik added 1 commit 2023-11-03 02:53:11 +01:00

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

307a0a7d30 Merge branch 'main' into iterator-cpp

Richard Antalik requested review from Hans Goudey 2023-11-03 02:59:56 +01:00

Hans Goudey commented

2023-11-03 08:41:02 +01:00

@blender-bot build

Hans Goudey approved these changes 2023-11-03 08:43:20 +01:00

Hans Goudey left a comment

Great, thanks!

Richard Antalik added 2 commits 2023-11-06 00:47:03 +01:00

d25df1ce24 Forgot to commit some changes

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

49655c5d66 Merge branch 'main' into iterator-cpp

Richard Antalik commented

2023-11-06 00:47:24 +01:00

@blender-bot build

Richard Antalik added 1 commit 2023-11-06 00:56:26 +01:00

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

aa8733c1a3 Missed a conflict

Richard Antalik commented

2023-11-06 00:57:05 +01:00

@blender-bot build

Richard Antalik added 1 commit 2023-11-06 01:30:40 +01:00

a59ce8b226 Remove unused function

Richard Antalik merged commit 3fccfe0bc6 into main

2023-11-06 01:36:55 +01:00

Richard Antalik referenced this issue from a commit

2023-11-06 01:36:56 +01:00

VSE: Use C++ containers for strip iteration

Richard Antalik deleted branch iterator-cpp

2023-11-06 01:36:56 +01:00

Bastien Montagne commented

2023-11-07 11:40:16 +01:00

For the record, in the future when making such huge refactor, it would be good to do some basic trivial testing in affected code...

This broke something as basic as duplicating four strips together, due to a small mistake (see 3fccfe0bc6 ).

Also, it would be better to split such changes in more, smaller commits too. Makes it easier to bisect and understand what is the source of the problem.

For the record, in the future when making such huge refactor, it would be good to do some basic trivial testing in affected code... This broke something as basic as duplicating four strips together, due to a small mistake (see 3fccfe0bc6 ). Also, it would be better to split such changes in more, smaller commits too. Makes it easier to bisect and understand what is the source of the problem.

Bastien Montagne commented

2023-11-07 12:53:43 +01:00

And 869372ffc3 fixes another... surprising mistake in that PR :(

And 869372ffc3 fixes another... surprising mistake in that PR :(

Richard Antalik commented

2023-11-07 13:22:29 +01:00

@mont29 Thanks for fixes, that static qualifier was really stupid, not sure what happened there.

For splitting the patch, I guess I could have done multiple commits for bunch of files, but this is single functional change so it would be a bit tricky.

@mont29 Thanks for fixes, that `static` qualifier was really stupid, not sure what happened there. For splitting the patch, I guess I could have done multiple commits for bunch of files, but this is single functional change so it would be a bit tricky.

Kazashi Yoshioka referenced this issue from a commit

2023-11-08 11:53:34 +01:00

VSE: Use C++ containers for strip iteration

Sign in to join this conversation.

No reviewers