Move rotation interpolation to Curve3d and refactor baking #64212

xiongyaohua · 2022-08-10T11:24:03Z

Related with PR #64043, PR #63956 there are two purpose of this PR:

Continuing refactor rotation interpolation logic for PathFollower3D #64043 's effort, move all remaining rotation interpolation modes from PathFollower3D to Curve3D
Reimplement baking for Curve3D with geometry methods of Basis class

The new implementation should be more stable. Although I test it for some cases, further testers are needed, as I mainly use Curve2D for current project.

Untitled.mov

Tested against examples in the following tickets:
48864
27137
60151

YuriSizov · 2022-08-10T11:29:00Z

Do you have a proposal or an issue that this closes? We ask that our contributors first document the problem or limitation that they experience, so that other contributors, users, and maintainers can find the best solution. A PR that adds new API needs a proposal, and a PR that fixes a bug needs a bug report.

You would also need to squash all your commits into one before this can be merged (but this can be done after the code review).

xiongyaohua · 2022-08-10T14:35:29Z

Do you have a proposal or an issue that this closes? We ask that our contributors first document the problem or limitation that they experience, so that other contributors, users, and maintainers can find the best solution. A PR that adds new API needs a proposal, and a PR that fixes a bug needs a bug report.
You would also need to squash all your commits into one before this can be merged (but this can be done after the code review).

Thanks for the advice, this is my first effort to submit code on Github, and is still trying to figure out the proper procedure. I add a proposal.

YuriSizov · 2022-08-10T14:37:28Z

Thanks for the advice, this is my first effort to submit code on Github, and is still trying to figure out the proper procedure.

It often depends from project to project. We have our procedures explained in the documentation for contributors:
https://docs.godotengine.org/en/stable/community/contributing/index.html

fire · 2022-08-10T22:33:31Z

@TokageItLab I will look at this too.

xiongyaohua · 2022-08-14T10:22:38Z

I tested my implementation with examples in the following tickets:
48864
27137
60151

it seems all worked well, and not only ORIENTED but also all other modes are working reasonably now. Confirmation needed.

fire · 2022-08-14T12:34:20Z

Something is odd. There are 6 ways to convert euler to quaternion. You only have 3.

xiongyaohua · 2022-08-14T13:15:34Z

Something is odd. There are 6 ways to convert euler to quaternion. You only have 3.

Because only those 3 are exposed in the original PathFollower3D API, and I am following that convention. I can add the rest 3 if it is necessary, but I think maybe it is better to do that in another PR since it changes API?

Zireael07 · 2022-11-02T18:01:00Z

Is there anything blocking this now that the 2D equivalent has been merged?

doc/classes/Curve3D.xml

scene/resources/curve.cpp

scene/resources/curve.h

xiongyaohua · 2022-11-04T05:35:06Z

A major refactor of the sampling methods on baked points. The main changes are

Factor out the binary search code, repeated in several sampling methods, into a new method _find_interval().
rearrange the error checking calls, in order to avoid redundant checks.
Add tilt_enable property to PathFollow3D, to control whether it honers the tilt of the Curve3D

Now this PR should be ready for review. I plan to start coding this proposal once this PR merged, as sample_baked_with_rotation() method from this PR is essential for an clean implementation.

scene/resources/curve.cpp

xiongyaohua · 2022-11-07T11:25:09Z

There is no consensus on this yet.

Ok, then it is my misunderstanding.

It is impossible to determine, as even this PR description does not compare the two enough. It is recommended that the original behavior be retained so that this can be properly discussed in another PR.

I propose the comparison can be done by user between new build and original build, if the old behavior is indeed better, it can be implemented properly in following PR(move both data and behavior to PathFollow3D).

Honestly this PR discussion is too long (arguably the longest per file? 130+ post for just 4 code files), it seems this part of system is too intricate for me. I propose we wrap it up here, or leave it to someone more capable.

Zireael07 · 2022-11-07T11:34:08Z

Let's keep the old behavior for now but keep the new TNB code somewhere so that it can be reused in a follow-up PR imho

Also > artistic control

isn't an issue here imho

TokageItLab · 2022-11-07T13:53:00Z

The actual discussion is not that much since most of the comments are code comment and variable name corrections and your responses to them.

The length of the discussion is not the point, in refactoring it is important to be careful and to implement it correctly. Also, the stance of change the old behavior as a trial and then revert after seeing the response is basically not approved. We need to test and discuss whether it makes sense or not before merging. If you want to avoid discussion for tilt, it is best to keep the old behavior.

Indeed, I don't like the old tilt's behavior, but I can't decide that the new tilt's behavior is fine or not yet. At least, if you remove the old tilt behavior here, we lose the ability to cover it. So it is best to keep the original behavior here, then in a later PR, implement the artistic control and change the old tilt behavior at the same time.

xiongyaohua · 2022-11-11T02:27:51Z

the old behavior should be retained to reduce discussion in this PR

done

TokageItLab

The comment should be if it is a long process where the function name alone does not describe the behavior or "the reason for the constant" or "the why not another implementation for here". Do not include your simple question.

Most of the problems seem to have been taken care of. After fixed comment, please put all your commits in one by squashing.

scene/3d/path_3d.cpp

scene/resources/curve.cpp

scene/resources/curve.h

scene/resources/curve.cpp

This commit makes the following major changes 1. Add "sample_baked_with_rotation()" to Curve3D, making it usable independently. A similar change was made to Curve2D previously. 2. Refactor the _bake() method on Curve3D, using Parallel Transport Frame instead of Frenet Frame. 3. Refactor the sample_* methods, including: i. Factor out common binary search code, following the DRY principe ii. sample_up_vector() interpolated up vector as part of rotation frame(posture) for consistancy and accuracy.

TokageItLab

It made me confused since the variables and functions were not matching the actual behavior, but I think that most of problem has been removed and it becomes readable in some extent now. The behavior is not perfect, but it is better than before. Thank you for hard working :)

I approved this for now because we need to follow the Curve2D changes and merge this. Some improvements should be made in a follow-up PR later.

I leave some comments below on points that I could not determine on my own.

TokageItLab · 2022-11-12T08:12:08Z

scene/resources/curve.cpp

+		bool is_loop = true;
+		// Loop smoothing only applies when the curve is a loop, which means two ends meet, and share forward directions.
+		{
+			if (!points_ptr[0].is_equal_approx(points_ptr[point_count - 1])) {
+				is_loop = false;
+			}
+
+			real_t dot = forward_write[0].dot(forward_write[point_count - 1]);
+			if (dot < 1.0 - 0.01) { // Alignment should not be too tight, or it dosen't work for coarse bake interval
+				is_loop = false;
+			}
 		}

-		up_write[idx] = up;
+		// Twist up vectors, so that they align at two ends of the curve.
+		if (is_loop) {
+			const Vector3 up_start = up_write[0];
+			const Vector3 up_end = up_write[point_count - 1];


It seems that the rotations still don't perfectly match between progress 0 and 1.

Possibly, I guess the main cause is that the interval at the end of the bake is different from the other intervals. Yeah, now behavior is better than before, so I think it is fine for now, but there is room for improvement.

Yes, I agree. From the picture I saw the tangents(blue arrows) is not aligned perfectly. Which should be problem in baking. I will investigate later.

Should be fixed by #69043

TokageItLab · 2022-11-12T08:43:17Z

scene/resources/curve.cpp

+Vector3 Curve3D::sample_baked_up_vector(real_t p_offset, bool p_apply_tilt) const {
+	if (baked_cache_dirty) {
+		_bake();
+	}
+
+	// Validate: Curve may not have baked up vectors.
+	ERR_FAIL_COND_V_MSG(!up_vector_enabled, Vector3(0, 1, 0), "No up vectors in Curve3D.");
+
+	int count = baked_up_vector_cache.size();
+	if (count == 1) {
+		return baked_up_vector_cache.get(0);
 	}

-	return up.rotated(axis, up.angle_to(up1) * frac);
+	p_offset = CLAMP(p_offset, 0.0, get_baked_length()); // PathFollower implement wrapping logic.
+
+	Curve3D::Interval interval = _find_interval(p_offset);
+	return _sample_posture(interval, p_apply_tilt).get_column(1);


I don't know whether sample_baked_up_vector() always needs forward_vector. _sample_posture() does spherical interpolation of the basis created from the baked up_vector and unbaked forward_vector inside it, but it seems to me that this is beyond the role of the function of the name sample_baked_up_vector().

Rather, since the only use of sample_baked_up_vector() is for paths in CSGShape, perhaps we can remove this and keep only sample_baked_with_rotation(). But that include unnecessary processing, so I still think it is better to rename sample_baked_up_vector() to sample_up_vector() as a wrapper funtion of _sample_posture().

How do you think? @reduz

created from the baked up_vector and unbaked forward_vector inside it

to not bake the forward_vector is just an optimization, because it is cheap to calculate on the fly. If performance is indeed an issue, baking cache for forward_vector can be add back easily.

since the only use of sample_baked_up_vector() is for paths in CSGShape

Another possibility is to remove sample_baked_up_vector() altogether, and let CSGShape use the rotation frame from sample_baked_with_rotation() directly.

Currently the CSGShape path mode calculate its own rotation frame

godot/modules/csg/csg_shape.cpp

Line 1963 in 324106b

Transform3D facing = Transform3D().looking_at(direction, current_up);

It use the same method as sample_baked_with_rotation() (looking_at(tangent, up)). So using rotation frame directly from sample_baked_with_rotation() is equivalent, but have advantages for future: Let CSGShape path mode benefit from improvements on Curve3D for free, such as better tilt, bug fixes, performance optimization

So I suggested in my comment above that CSGShape use sample_baked_with_rotation() and remove sample_baked_up_vector(). But it include a process sample_baked() which is not needed, so I more recommended renaming it to sample_up_vector(). Or make sample_posture() public and remove sample_baked_up_vector() is another way.

In any case, the name sample_baked_up_vector() seems strange for me because it is sampling something unbaked by _sample_posture() internally.

The final decision on whether to rename it or not is left to @reduz or another experienced contributor.

xiongyaohua · 2022-11-12T09:33:44Z

, but I think that most of problem has been removed and it becomes readable in some extent now. The behavior is not perfect, but it is better than before. Thank you for hard working :)

Also thank you for all your time putting into this PR. Hope we can get to consensus faster in future PRs :)

xiongyaohua · 2022-11-18T07:08:26Z

@Calinou This looks like a good change, but make sure to apply it in 3D as well. (This can be done in a future pull request, but it should be done before 4.0 is released if this PR is merged for 4.0.)

@YuriSizov is it possible to move this PR to 4.0 milestone? Considering the Curve2D counterpart has already been merged.

akien-mga · 2022-11-18T15:23:05Z

Thanks!

Ademan · 2022-11-19T05:57:00Z

This change seems to have changed the behavior for Curve3D::sample_baked_up_vector(). I noticed this while rebasing my changes for #68873 which adds a visual indicator and editable handle for the tilt of a Curve3D (The handle is simply Curve3D::sample_baked_up_vector(t, true))

Here is a screenshot of a curve with tilt 0 for all points before and after this was merged:

My change rebased on top of 8ab3e73^ (immediately before the merge uploaded here ):

My change rebased on top of 8ab3e73 :

You can see the up vectors are 90 degrees from their previous location.

EDIT: a better screenshot

TokageItLab · 2022-11-19T07:16:47Z

@xiongyaohua Might it be possible that normal is being baked instead up vector by mistake? I'm not sure since I haven't looked into rotating algorithm itself enough. In any case, the Path3D area is refactoring now, so if anyone find anything odd, we should open an issue and discuss it each time @Ademan.

xiongyaohua requested review from a team as code owners August 10, 2022 11:24

YuriSizov added enhancement feature proposal labels Aug 10, 2022

YuriSizov added this to the 4.x milestone Aug 10, 2022

YuriSizov added the topic:core label Aug 10, 2022

YuriSizov added the topic:3d label Aug 10, 2022

xiongyaohua mentioned this pull request Aug 10, 2022

Move rotation interpolation logic from PathFollower2D/3D to Curve2D/3D godotengine/godot-proposals#5136

Closed

xiongyaohua force-pushed the curve3d_baking_refactor branch from 43c27ab to e55b2a8 Compare August 10, 2022 14:47

xiongyaohua force-pushed the curve3d_baking_refactor branch from e55b2a8 to 15c5147 Compare August 11, 2022 04:39

xiongyaohua force-pushed the curve3d_baking_refactor branch 2 times, most recently from 55aede0 to 37a811e Compare August 31, 2022 08:52

xiongyaohua mentioned this pull request Sep 2, 2022

refactor rotation interpolation logic for PathFollower3D #64043

Closed

xiongyaohua force-pushed the curve3d_baking_refactor branch from 37a811e to fe23fb0 Compare September 2, 2022 00:27

xiongyaohua changed the title ~~Curve3d baking refactor~~ Move rotation interpolation to Curve3d and refactor baking Oct 31, 2022

xiongyaohua force-pushed the curve3d_baking_refactor branch from fe23fb0 to 96ae391 Compare November 3, 2022 02:01

Iniquitatis reviewed Nov 3, 2022

View reviewed changes

doc/classes/Curve3D.xml Outdated Show resolved Hide resolved

Iniquitatis reviewed Nov 3, 2022

View reviewed changes

scene/resources/curve.cpp Outdated Show resolved Hide resolved

xiongyaohua force-pushed the curve3d_baking_refactor branch 2 times, most recently from dd3d14f to c53faee Compare November 4, 2022 05:31

TokageItLab reviewed Nov 4, 2022

View reviewed changes

scene/resources/curve.h Outdated Show resolved Hide resolved

TokageItLab reviewed Nov 7, 2022

View reviewed changes

scene/resources/curve.cpp Outdated Show resolved Hide resolved

TokageItLab removed the for pr meeting label Nov 8, 2022

xiongyaohua force-pushed the curve3d_baking_refactor branch from 389db3e to 2ea259b Compare November 11, 2022 02:22

xiongyaohua requested a review from TokageItLab November 11, 2022 02:32

TokageItLab requested changes Nov 11, 2022

View reviewed changes

scene/resources/curve.cpp Outdated Show resolved Hide resolved

scene/resources/curve.cpp Outdated Show resolved Hide resolved

xiongyaohua force-pushed the curve3d_baking_refactor branch from 34f5364 to 77898bf Compare November 12, 2022 03:55

xiongyaohua force-pushed the curve3d_baking_refactor branch from 77898bf to 5241464 Compare November 12, 2022 04:16

TokageItLab approved these changes Nov 12, 2022

View reviewed changes

TokageItLab reviewed Nov 12, 2022

View reviewed changes

YuriSizov modified the milestones: 4.x, 4.0 Nov 18, 2022

akien-mga merged commit 8ab3e73 into godotengine:master Nov 18, 2022

xiongyaohua deleted the curve3d_baking_refactor branch November 18, 2022 15:59

Ademan mentioned this pull request Nov 19, 2022

Curve3D::sample_baked_up_vector() returns value perpendicular to its previous value after merge #68893

Closed

xiongyaohua mentioned this pull request Nov 23, 2022

Curve3D bake refactor continue #69043

Merged

Calinou mentioned this pull request Dec 2, 2022

CSGPolygon3D using Path does not follow medium sized path well #69507

Closed

xiongyaohua mentioned this pull request Dec 3, 2022

Curve3D get_closest_point broken #69220

Closed

raulsntos mentioned this pull request Jun 15, 2023

Clamp PathFollow3D progress when not looping #78280

Merged

xiongyaohua mentioned this pull request May 29, 2024

Integrate PGA (Projective Geometric Algebra) in Godot godotengine/godot-proposals#9841

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move rotation interpolation to Curve3d and refactor baking #64212

Move rotation interpolation to Curve3d and refactor baking #64212

xiongyaohua commented Aug 10, 2022 •

edited

Loading

YuriSizov commented Aug 10, 2022

xiongyaohua commented Aug 10, 2022

YuriSizov commented Aug 10, 2022

fire commented Aug 10, 2022

xiongyaohua commented Aug 14, 2022 •

edited

Loading

fire commented Aug 14, 2022

xiongyaohua commented Aug 14, 2022 •

edited

Loading

Zireael07 commented Nov 2, 2022

xiongyaohua commented Nov 4, 2022 •

edited

Loading

xiongyaohua commented Nov 7, 2022 •

edited

Loading

Zireael07 commented Nov 7, 2022 •

edited

Loading

TokageItLab commented Nov 7, 2022 •

edited

Loading

xiongyaohua commented Nov 11, 2022 •

edited

Loading

TokageItLab left a comment

TokageItLab left a comment

TokageItLab Nov 12, 2022 •

edited

Loading

xiongyaohua Nov 12, 2022 •

edited

Loading

xiongyaohua Nov 23, 2022

TokageItLab Nov 12, 2022 •

edited

Loading

xiongyaohua Nov 12, 2022 •

edited

Loading

TokageItLab Nov 12, 2022 •

edited

Loading

xiongyaohua commented Nov 12, 2022 •

edited

Loading

xiongyaohua commented Nov 18, 2022 •

edited

Loading

akien-mga commented Nov 18, 2022

Ademan commented Nov 19, 2022 •

edited

Loading

TokageItLab commented Nov 19, 2022 •

edited

Loading

Move rotation interpolation to Curve3d and refactor baking #64212

Move rotation interpolation to Curve3d and refactor baking #64212

Conversation

xiongyaohua commented Aug 10, 2022 • edited Loading

YuriSizov commented Aug 10, 2022

xiongyaohua commented Aug 10, 2022

YuriSizov commented Aug 10, 2022

fire commented Aug 10, 2022

xiongyaohua commented Aug 14, 2022 • edited Loading

fire commented Aug 14, 2022

xiongyaohua commented Aug 14, 2022 • edited Loading

Zireael07 commented Nov 2, 2022

xiongyaohua commented Nov 4, 2022 • edited Loading

xiongyaohua commented Nov 7, 2022 • edited Loading

Zireael07 commented Nov 7, 2022 • edited Loading

TokageItLab commented Nov 7, 2022 • edited Loading

xiongyaohua commented Nov 11, 2022 • edited Loading

TokageItLab left a comment

Choose a reason for hiding this comment

TokageItLab left a comment

Choose a reason for hiding this comment

TokageItLab Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

xiongyaohua Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

xiongyaohua Nov 23, 2022

Choose a reason for hiding this comment

TokageItLab Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

xiongyaohua Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

TokageItLab Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

xiongyaohua commented Nov 12, 2022 • edited Loading

xiongyaohua commented Nov 18, 2022 • edited Loading

akien-mga commented Nov 18, 2022

Ademan commented Nov 19, 2022 • edited Loading

TokageItLab commented Nov 19, 2022 • edited Loading

xiongyaohua commented Aug 10, 2022 •

edited

Loading

xiongyaohua commented Aug 14, 2022 •

edited

Loading

xiongyaohua commented Aug 14, 2022 •

edited

Loading

xiongyaohua commented Nov 4, 2022 •

edited

Loading

xiongyaohua commented Nov 7, 2022 •

edited

Loading

Zireael07 commented Nov 7, 2022 •

edited

Loading

TokageItLab commented Nov 7, 2022 •

edited

Loading

xiongyaohua commented Nov 11, 2022 •

edited

Loading

TokageItLab Nov 12, 2022 •

edited

Loading

xiongyaohua Nov 12, 2022 •

edited

Loading

TokageItLab Nov 12, 2022 •

edited

Loading

xiongyaohua Nov 12, 2022 •

edited

Loading

TokageItLab Nov 12, 2022 •

edited

Loading

xiongyaohua commented Nov 12, 2022 •

edited

Loading

xiongyaohua commented Nov 18, 2022 •

edited

Loading

Ademan commented Nov 19, 2022 •

edited

Loading

TokageItLab commented Nov 19, 2022 •

edited

Loading