Fix #119178: Geometry Nodes: Its possible to write non-normalized quaternion attribute from python API #119628

Lleu Yang · 2024-03-18T15:31:46+01:00

Lleu Yang commented

2024-03-18 15:31:46 +01:00

A setter is added for the QuaternionAttribute.value RNA property, which
provides additional check and normalization for non-normalized inputs.

Edit: added benchmark.

A setter is added for the QuaternionAttribute.value RNA property, which provides additional check and normalization for non-normalized inputs. Edit: added benchmark.

GS_quaternion_normalize_benchmark.blend

1.9 MiB

Lleu Yang force-pushed fix-119178 from b7736d56c1 to a816dd50b9

2024-03-18 15:35:26 +01:00

Compare

Iliya Katushenock added this to the Nodes & Physics project 2024-03-18 16:33:50 +01:00

Iliya Katushenock added the

labels 2024-03-18 16:33:58 +01:00

Iliya Katushenock requested review from Jacques Lucke 2024-03-18 16:34:04 +01:00

Iliya Katushenock requested review from Hans Goudey 2024-03-18 16:34:05 +01:00

Iliya Katushenock reviewed 2024-03-18 16:35:34 +01:00

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

						
				@ -309,0 +313,4 @@

				  using blender::math::Quaternion;

				  float *fs = static_cast<float *>(ptr->data);

				  auto q = Quaternion(VecBase<float, 4>(values));

Iliya Katushenock commented

2024-03-18 16:34:44 +01:00

const Quaternion src = math::normalize(Quaternion(VecBase<float, 4>(values)));
?

`const Quaternion src = math::normalize(Quaternion(VecBase<float, 4>(values)));` ?

❤️ 1

Lleu Yang commented

2024-03-18 16:52:19 +01:00

This is indeed better since math::normalize provides both unit scale validation and normalization. Will change. Thanks!

Edit: Sorry for the frustration. I have looked into the wrong function. In fact math::normalize doesn't check before actual calculation. I wonder if there's any practical difference in performance between the check-before-calc method and calc-immediately method?

This is indeed better since `math::normalize` provides both unit scale validation and normalization. Will change. Thanks! Edit: Sorry for the frustration. I have looked into the wrong function. In fact `math::normalize` doesn't check before actual calculation. I wonder if there's any practical difference in performance between the check-before-calc method and calc-immediately method?

Iliya Katushenock commented

2024-03-18 19:41:08 +01:00

Not sure if branch with non-zero cost check actually will make it faster.

Lleu Yang commented

2024-03-19 16:55:53 +01:00

I have done a quick benchmark, and it shows that there's little difference between the two methods. I will change my code since your line reduces unnecessary complexity. Again, thanks!

I have done a quick [benchmark](https://projects.blender.org/attachments/e3421f8c-f30e-4afd-b455-27928ff518c8), and it shows that there's little difference between the two methods. I will change my code since your line reduces unnecessary complexity. Again, thanks!

megakite marked this conversation as resolved

Lleu Yang added 1 commit 2024-03-19 16:59:00 +01:00

f22da6f59d Remove unnecessary unit check

Iliya Katushenock commented

2024-04-16 14:11:27 +02:00

On the last geometry nodes meeting we decide that this also might make sense to ask @brecht about this/
Mainly this still looks like additional overhead and not so explicit for API user.

On the last geometry nodes meeting we decide that this also might make sense to ask @brecht about this/ Mainly this still looks like additional overhead and not so explicit for API user.

Brecht Van Lommel commented

2024-04-16 14:18:31 +02:00

Quaternion rotations on objects are not normalized, I'm not sure why they should be here. That kind of normalization does not work with manual keyframing. I would rather expect any place using this quaternion to normalize it if needed.

👍 2

Iliya Katushenock commented

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

This means we have to change a lot of geometry nodes / bli code to have math functions to deal with non-normalized quaternions.

👀 1

Lleu Yang commented

2024-04-16 17:14:47 +02:00

I see. Would it be better if I mark this PR as WIP for further operations?