Refactor: ANIM_setting_get_rna_values to return a Vector #113931

Christoph Lendenfeld · 2023-10-19T12:50:33+02:00

2023-10-19 12:50:33 +02:00

No functional changes.

Changing old C code to C++ by returning a Vector.
This reduces the argument count for that function and simplifies the code.

Questions for the reviewer:

when returning a Vector by value the memory will not be copied if I am not mistaken. That's the l_value/r_value feature in C++. Right?
I am using std::copy instead of memcpy. It compiles, the internet says that's the way. But I still feel unsure about the implications.

No functional changes. Changing old C code to C++ by returning a `Vector`. This reduces the argument count for that function and simplifies the code. ---- Questions for the reviewer: * when returning a Vector by value the memory will not be copied if I am not mistaken. That's the l_value/r_value feature in C++. Right? * I am using `std::copy` instead of `memcpy`. It compiles, the internet says that's the way. But I still feel unsure about the implications.

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-10-19 12:50:33 +02:00

Christoph Lendenfeld added 1 commit 2023-10-19 12:50:44 +02:00

336e78b1d1 array + size to vector

Christoph Lendenfeld referenced this pull request

2023-10-19 13:18:20 +02:00

Anim: Keyframing Rework #113278

Christoph Lendenfeld added 1 commit 2023-10-19 13:41:42 +02:00

5aea739575 change to mutable span

Jacques Lucke commented

2023-10-19 14:24:15 +02:00

when returning a Vector by value the memory will not be copied if I am not mistaken. That's the l_value/r_value feature in C++. Right?

That's right. In most cases where a function returns a vector that it just creates there is no extra copy or move. The new vector will be created directly in the stack space of the calling function. Also see https://en.cppreference.com/w/cpp/language/copy_elision

I am using std::copy instead of memcpy. It compiles, the internet says that's the way. But I still feel unsure about the implications.

For trivial types, like float, there is no functional difference between using memcpy or std::copy. For non-trivial types, std::copy will call assignment operators appropriately. Sometimes using std::copy_n may be easier.

> when returning a Vector by value the memory will not be copied if I am not mistaken. That's the l_value/r_value feature in C++. Right? That's right. In most cases where a function returns a vector that it just creates there is no extra copy or move. The new vector will be created directly in the stack space of the calling function. Also see https://en.cppreference.com/w/cpp/language/copy_elision > I am using std::copy instead of memcpy. It compiles, the internet says that's the way. But I still feel unsure about the implications. For [trivial types](https://en.cppreference.com/w/cpp/named_req/TrivialType), like `float`, there is no functional difference between using `memcpy` or `std::copy`. For non-trivial types, `std::copy` will call assignment operators appropriately. Sometimes using `std::copy_n` may be easier.

👍 1

Sybren A. Stüvel requested changes 2023-10-19 14:30:30 +02:00

Sybren A. Stüvel left a comment

Such an improvement! Thanks!

I've left some inline notes, should be fairly straight forward to address.

Such an improvement! Thanks! I've left some inline notes, should be fairly straight forward to address.

source/blender/animrig/intern/visualkey.cc Outdated

						
				@ -235,3 +233,1 @@

				      copy_v3_v3(buffer, ob->object_to_world[3]);

				      *r_count = 3;

				      return buffer;

				      for (int i = 0; i < 3; i++) {

Sybren A. Stüvel commented

2023-10-19 14:21:44 +02:00

I think it might be nice to keep the abstractions the way they were, at least here. That would mean having some copy_vector_v3(blender::Vector<float> dest, const float src[3]) function that basically performs this loop.

I think it might be nice to keep the abstractions the way they were, at least here. That would mean having some `copy_vector_v3(blender::Vector<float> dest, const float src[3])` function that basically performs this loop.

Hans Goudey commented

2023-10-19 14:36:35 +02:00

How about vector.extend({ob->object_to_world[3], 3});

How about `vector.extend({ob->object_to_world[3], 3});`

Christoph Lendenfeld commented

2023-10-19 15:08:09 +02:00

used vector.extend since I am not sure where I would put such a function

used `vector.extend` since I am not sure where I would put such a function

source/blender/animrig/intern/visualkey.cc

						
				@ -273,3 +276,1 @@

				    *r_count = 4;

				    return buffer;

				    for (int i = 0; i < 4; i++) {

Sybren A. Stüvel commented

2023-10-19 14:23:06 +02:00

Same as above, concerning abstraction level. IMO it would be perfectly fine to have a mat4_to_quat_vector() function that basically performs the same operation as you have here.

Same as above, concerning abstraction level. IMO it would be perfectly fine to have a `mat4_to_quat_vector()` function that basically performs the same operation as you have here.

dr.sybren marked this conversation as resolved

source/blender/blenkernel/BKE_animsys.h Outdated

						
				@ -263,3 +263,2 @@

				                                           struct PropertyRNA *prop,

				                                           float *values,

				                                           int count,

				                                           const blender::MutableSpan<float> &values,

Sybren A. Stüvel commented

2023-10-19 14:23:48 +02:00

AFAIK Span and MutableSpan are reference types anyway, and don't need to be passed as reference themselves. But @HooglyBoogly can say more about that.

AFAIK `Span` and `MutableSpan` are reference types anyway, and don't need to be passed as reference themselves. But @HooglyBoogly can say more about that.

Hans Goudey commented

2023-10-19 14:34:15 +02:00

Yes, it's better to pass them by value, otherwise inside the function it's like a pointer to a pointer.

They are 16 bytes, so they are small enough as well.

Yes, it's better to pass them by value, otherwise inside the function it's like a pointer to a pointer. They are 16 bytes, so they are small enough as well.

Christoph Lendenfeld commented

2023-10-19 15:08:29 +02:00

thanks for that, fixed

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

						
				@ -594,1 +584,3 @@

				        RNA_property_float_get_array(ptr, prop, values);

				      }

				      case PROP_FLOAT: {

				        float *tmp_float = static_cast<float *>(

Sybren A. Stüvel commented

2023-10-19 14:26:11 +02:00

Since the values don't need to be cast to float here, it might just work here to create a length-size Vector<float> and pass the address of the first element to RNA_property_float_get_array()?

Since the values don't need to be cast to `float` here, it might just work here to create a `length`-size `Vector<float>` and pass the address of the first element to `RNA_property_float_get_array()`?

👍 1

Christoph Lendenfeld commented

2023-10-19 15:08:49 +02:00

yep that seems to work

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

						
				@ -615,3 +611,3 @@

				        break;

				      default:

				        *values = 0.0f;

				        break;

Sybren A. Stüvel commented

2023-10-19 14:29:38 +02:00

This is a change in behaviour. Instead of returning one zero value, it returns no value. Not sure if this is an important distinction, but given that this PR is labeled "refactor", it's better to avoid functional changes.

Christoph Lendenfeld commented

2023-10-19 15:09:32 +02:00

good point. Also changed it for the property array case

Christoph Lendenfeld added 1 commit 2023-10-19 15:08:52 +02:00

c504b47919 implement feedback

Christoph Lendenfeld requested review from Sybren A. Stüvel 2023-10-19 15:15:06 +02:00

Christoph Lendenfeld commented

2023-10-19 15:16:42 +02:00

thanks all for the feedback
@dr.sybren as mentioned in the comment, I went for vector.extend since I don't know where to put functions like copy_vector_v3
can still do them if you point me to where they would live

thanks all for the feedback @dr.sybren as mentioned in the comment, I went for `vector.extend` since I don't know where to put functions like `copy_vector_v3` can still do them if you point me to where they would live

Iliya Katushenock reviewed 2023-10-19 17:13:02 +02:00

source/blender/animrig/ANIM_visualkey.hh

						
				@ -16,3 +18,2 @@

				bool visualkey_can_use(PointerRNA *ptr, PropertyRNA *prop);

				float *visualkey_get_values(

				    PointerRNA *ptr, PropertyRNA *prop, float *buffer, int buffer_size, int *r_count);

				Vector<float> visualkey_get_values(PointerRNA *ptr, PropertyRNA *prop);

Iliya Katushenock commented

2023-10-19 17:04:18 +02:00

You can add forward declaration for vector in header to do not include big header.

Christoph Lendenfeld commented

2023-10-19 17:59:23 +02:00

is there an example for that? All the header files I could find just include BLI_vector.hh
I tried the forward declaration but couldn't get it to work

is there an example for that? All the header files I could find just include `BLI_vector.hh` I tried the forward declaration but couldn't get it to work

Iliya Katushenock commented

2023-10-19 18:20:53 +02:00

namespace blender {
template<typename T, int64_t InlineBufferCapacity, typename Allocator> class Vector;
}   // namespace blender

Something like ^ should works. If that is not so trivial, then it's not that important.

```Cpp namespace blender { template<typename T, int64_t InlineBufferCapacity, typename Allocator> class Vector; } // namespace blender ``` Something like ^ should works. If that is not so trivial, then it's not that important.

Christoph Lendenfeld commented

2023-10-20 11:36:16 +02:00

still doesn't work. Complains about
cannot overload functions distinguished by return type alone

still doesn't work. Complains about `cannot overload functions distinguished by return type alone`

mod_moder marked this conversation as resolved

source/blender/blenkernel/intern/anim_sys.cc

						
				@ -3783,3 +3783,3 @@

				  NlaEvalChannelSnapshot *blended_necs = nlaeval_snapshot_ensure_channel(&blended_snapshot, nec);

				  memcpy(blended_necs->values, values, sizeof(float) * count);

				  std::copy(values.begin(), values.end(), blended_necs->values);

Iliya Katushenock commented

2023-10-19 17:08:13 +02:00

You can also use BLI_array_utils.hh to use parallel version of this function (in case is large spans).

You can also use `BLI_array_utils.hh` to use parallel version of this function (in case is large spans).

Christoph Lendenfeld commented

2023-10-19 17:51:58 +02:00

the element count is at most 4. I don't think parallel makes a difference here
but i'll keep it in mind. There will be things in animation that need to be run in parallel

the element count is at most 4. I don't think parallel makes a difference here but i'll keep it in mind. There will be things in animation that need to be run in parallel

dr.sybren marked this conversation as resolved

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

						
				@ -594,1 +584,3 @@

				        RNA_property_float_get_array(ptr, prop, values);

				      }

				      case PROP_FLOAT: {

				        values = blender::Vector<float>(length);

Iliya Katushenock commented

2023-10-19 17:12:05 +02:00

values.reinitialize(length);

`values.reinitialize(length);`

Christoph Lendenfeld commented

2023-10-19 17:59:38 +02:00

thanks, done that

Christoph Lendenfeld added 1 commit 2023-10-19 17:52:50 +02:00

16aa43c777 use vector.reinitialize()

Sybren A. Stüvel commented

2023-10-20 12:33:10 +02:00

@dr.sybren as mentioned in the comment, I went for vector.extend since I don't know where to put functions like copy_vector_v3
can still do them if you point me to where they would live

I think those could go into the same files as their array-based counterparts. That would require a little discussion with the code module, though. What could work also is something like this:

values.resize(3);
mat4_to_size(values.data(), tmat);

I think this is both more readable and more efficient. And if this works, it prevents us from having to create a copy of every single array-based function to a vector-based alternative.

> @dr.sybren as mentioned in the comment, I went for `vector.extend` since I don't know where to put functions like `copy_vector_v3` > can still do them if you point me to where they would live I think those could go into the same files as their array-based counterparts. That would require a little discussion with the code module, though. What could work also is something like this: ```cpp values.resize(3); mat4_to_size(values.data(), tmat); ``` I think this is both more readable and more efficient. And if this works, it prevents us from having to create a copy of every single array-based function to a vector-based alternative.

Christoph Lendenfeld added 1 commit 2023-10-20 14:12:10 +02:00

f946f256e8 implement sybrens suggestions

Christoph Lendenfeld commented

2023-10-20 14:16:02 +02:00

@dr.sybren that's even better, changed to that
except for this line
mat4_to_axis_angle(&values[1], &values[0], tmat);
but that also works

@dr.sybren that's even better, changed to that except for this line `mat4_to_axis_angle(&values[1], &values[0], tmat);` but that also works

Iliya Katushenock commented

2023-10-20 14:26:30 +02:00

Please don't chenge size of vector manually. Or use values.resize(values.size() + 3); In other case this will be a stateful variable and everyone going to make sure is any append / reinitialize / clear will not break your manual copying.
If implicit span initializing ({data, size}) not clear for you, is there already extend(data, size) method overload.

Please don't chenge size of vector manually. Or use `values.resize(values.size() + 3);` In other case this will be a stateful variable and everyone going to make sure is any `append` / `reinitialize` / `clear` will not break your manual copying. If implicit span initializing (`{data, size}`) not clear for you, is there already `extend(data, size)` method overload.

Christoph Lendenfeld commented

2023-10-20 16:24:21 +02:00

@mod_moder does it matter in this case? the function doing the resizing is creating the vector. so no one else could have used it yet

👍 1

Sybren A. Stüvel approved these changes 2023-10-23 10:32:16 +02:00

Sybren A. Stüvel left a comment

👍

:+1:

Christoph Lendenfeld merged commit 265858525e into main

2023-10-24 09:01:48 +02:00

Christoph Lendenfeld deleted branch refactor_get_rna_values

2023-10-24 09:01:50 +02:00

Christoph Lendenfeld referenced this issue from a commit

2023-10-24 09:01:50 +02:00

Refactor: ANIM_setting_get_rna_values to return a Vector

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

Refactor: ANIM_setting_get_rna_values to return a Vector #113931