Bezier curves on paths

[Semphriss]

Closes #1455
(Asked for spline curves, but Bezier turned out much, much easier to implement)

video.mp4

Fun fact: Implementing the bezier curve logic and pathtracing took 30 minutes, all bugfixes included. Adding those handles took me 5 hours of non-stop debugging.

[Semphriss]

Bug fixed:

In the video, you can see that placing nodes doesn't happen in order. This is fixed, and any new node will be added at the end of the line 👍

[HybridDog]

Thanks. I've tested it and it works. I think it is important to be able to easily create continuous transitions between the curve segments.
As far as I know, a transition is C1 continuous if both bezier markers have the same distance to the node and are collinear, i.e. pos_node - pos_marker_1 = pos_marker_2 - pos_node.
Maybe you can change the code so that if the user holds a certain button and moves a BezierMarker 1, the opposite BezierMarker 2 is moved to the position 2 * pos_node - pos_marker_1.

[Semphriss]

I thought the same but I postponed the feature, but I'll add it immediately if it is requested.

[Semphriss]

2021-01-10.15-13-30.mp4

[HybridDog]

I noticed that pressing Ctrl to spawn Bezier handles does not work reliable:
When I move the cursor to the node, press Ctrl and then click on the node, it moves the path node as though I wouldn't hold Ctrl.
Otoh, when I first press Ctrl and then move the cursor to the node and click the node, it spawns the handles (and for some reason, does not show the path node's text description).

[Semphriss]

I can reproduce that, but I'm not sure how to proceed about how to fix it. I'll investigate

[HybridDog]

It works when hover_object() is called in the key press methods.

+++ b/src/editor/overlay_widget.cpp
@@ -943,6 +943,9 @@ EditorOverlayWidget::on_key_up(const SDL_KeyboardEvent& key)
   if (sym == SDLK_LALT || sym == SDLK_RALT) {
     alt_pressed = false;
   }
+  // Selected objects during hovering depend on the currently pressed keys,
+  // so call hover_object when keys are pressed or released
+  hover_object();
   return true;
 }
 
@@ -963,6 +966,7 @@ EditorOverlayWidget::on_key_down(const SDL_KeyboardEvent& key)
   if (sym == SDLK_LALT || sym == SDLK_RALT) {
     alt_pressed = true;
   }
+  hover_object();
   return true;
 }

[Semphriss]

It's unfortunate that there isn't a simple "Accept changes" button, lol :)

[Semphriss]

I found a semi-regression: the default placement of bezier handles adds some sort of easing effect to the regular movement. This can be corrected by placing all handles 1/3rd way towards their target node, but it's difficult to implement that by default.

I think I'll add an option to toggle beziers, and leave it off by default for better compatibility (and to not break easing).

[Semphriss]

All fixed, ready for review once again.

Instead of adding an option, which would make the code much more complex, I made the no-beziers situation a special case.

[HybridDog]

Thanks. I like these curved paths, but I found a new problem:
When the path mode is Ping-pong, the curve is different in forward and backward direction.

hinundher.mp4

When the platform moves forward to the last node, it follows the curve visualized in red, but when it flies back, it moves differently.

[Semphriss]

Oops, I think I know what causes that.

"Next node" doesn't always mean the same thing in ping-pong mode :D

Omw to fix that

[Semphriss]

Changes:

• Bezier paths now go at constant speed, removing the need for my previous hack
• Both Bezier curves and easing patterns are now reversed properly when going backwards in ping-pong mode

[HybridDog]

I think the speed within a curve segment should not be constant. The easing effect of the curves is intentional (in my opinion) and makes it possible to have C1 continuity.

[Semphriss]

The problem with that is that the default positioning of the bezier curves add an unintended easing effect on linear nodes, making it a pain to maintain a constant speed.

I'm hesitant as to make it an option, as it's quite advanced and might be unnecessary for most purposes. I'll ask around for feedback about that.

[Semphriss]

Little votes were cast, but the tendency was towards adding an option.

This is, I believe, the best compromise between people who want to rely on the "Speed" property to truly go at constant speed, and those who want the speed to adapt properly to the pinching of the bezier curves.

[HybridDog]

The option works fine.

For some reason I could see Attempt to get point on Bezier curve further than the end. warnings.

I have changed the behaviour when holding alt:

• If the time values of the node markers are not equal, the offset vectors of the two bezier markers need to have a different length.
• If adapt_speed is set, C1 continuity is in general not possible, so it now preserves the length of the opposite offset vector

Here are the changes: HybridDog@21cb213
You can apply them with git am:
wget https://github.com/HybridDog/supertux/commit/21cb213cf34c174a2a6b3d8901ac9be4e53f9653.patch && git am -3 21cb213cf34c174a2a6b3d8901ac9be4e53f9653.patch

[Semphriss]

I'm not exactly sure I understand - what behaviour needs to be changed?

Just to confirm, "adapt speed" actually means "make the PathWalker go fast where the Bezier curve is stretched and slow where it is compressed", so it goes at a fixed constant speed when it is set to false.

How does adapt_speed impact C1 continuity? I believe setting the same speed value on all nodes should ensure that the pathwalking object traverses nodes smoothly, without sudden change in speed, regardless of adapt_speed.

[HybridDog]

Here's a video about my changes:
https://user-images.githubusercontent.com/3192173/107888616-cf69af80-6f0d-11eb-859d-ab1d481a0714.mp4

I've changed the position adjustment of the opposite marker when holding alt. If the node marker and both bezier markers are collinear, there's G1 continuity.
With constant speed (no adapt_speed), it can only be C1 continuous between two curve segments if both segments have the same speed, so changing the length of the bezier marker offset vectors does not help and holding alt should only make the markers collinear (to at least achieve G1 continuity).
With variable speed (adapt_speed), the length of the bezier marker offset vectors can be changed to achieve C1 continuity. If the durations (time values) of both curve segments is equal, these vectors need to have the same length. However, if the durations are different, it affects the speed, so the two vectors need to have a different length for C1 continuity.

[Semphriss]

I'm really hesitant about that, because too many options and too much inconsistency might bring in a lot of confusion for the users.

I honestly don't think there is a big need for so many options, because although practical, they don't seem (to me) important enough to justify such complexity. Remember that SuperTux is primarily targeted towards a young audience 😛

I suggest we keep it simple for the time being, and we decide what to do based on further user feedback. Beziers were done from the request of #1455, so if there is any need for more features, I trust we'll get an issue from a level designer soon enough.

In the meantime, feel free to maintain a branch with any features that could be interesting; it'll come in handy if and when those issues will come :)

[HybridDog]

My changes for the behaviour when holding alt don't introduce a new option.

I personally don't need the constant speed (disabled adapt_speed) option.

• Conceptually, with variable speed and C1 continuity, a platform has an inertia and gets accelerated at each point in time, which makes it move smoothly like in reality.
• Otoh without Bezier curves, the platform moves with only C0 continuity, so when the path is complicated, the player never knows when the platform will suddenly change its direction and speed.
• Bezier curves and constant speed (disabled adapt_speed) (and optionally equal speed on all segments) looks wrong to me because it looks like the platform doesn't have an acceleration and inertia, and it is more difficult for the player to predict future positions of the platform, especially when there is a strong curvature (quick direction changes).

[Semphriss]

Your changes when holding alt don't introduce new options, but they make the existing one more complex to learn.

Forcing adapt_speed to be enabled would render linear movement near-impossible, which is a huge minus in levels which need to have a platform move at constant speed (Some levels have platforms that move constantly in one direction for long periods of time, and have gameplay based on that).

RE inertia and smooth movement: that's exactly what easing is for. There are 30+ functions that can reproduce most of the effects you need to achieve intuitive/interesting movement, may it be acceleration, deceleration, bouncing, or other. Adding beziers' default impact on acceleration into the mix would make easings look ugly and unintuitive.

As for platform predictability, good level designing is the key. Making the patterns simple, short and repeating, or placing coins along the path, or shaping the surrounding decoration in a way that naturally directs the eye towards the path the platform will take are all possible ways to mitigate the problem.

[HybridDog]

Your changes when holding alt don't introduce new options, but they make the existing one more complex to learn.

Holding alt just moves the opposite bezier marker to obtain C1 or G1 continuity. I think it's easy to learn for the user because he/she can simply try it and observe the results.

Forcing adapt_speed to be enabled would render linear movement near-impossible […]

In my opinion, it should be possible to use both linear and Bezier curve segments on one path. However, Bezier curve segments with constant speed (disabled adapt_speed) are less important because they can be approximated by linear segments.
Levels where a platform moves a long distance with constant speed can use a linear segment for this part of the path.

RE inertia and smooth movement: that's exactly what easing is for.

Easing and Bezier curve segments shouldn't be combined in my opinion. With Bezier curves, adapt_speed and the alt key, it is possible to create a C1 continuous curve with intuitive smooth movement. With linear segments there would be no G1 continuity between segments, and it is difficult to use constant speed Bezier curves together with fixed easing functions to create a intuitive movement.
The easing functions are nonetheless useful for small linear paths (e.g. with two nodes).

As for platform predictability, good level designing is the key.

You're right; the level designer can set the path in a way which solves the problem. Once I played a level where the path of a platform was long and not easily predictable.

[Semphriss]

I'm not sure if I mentioned this yet - there is no such thing as linear movement with adapt_speed enabled. You can try putting simple nodes without touching the bezier handles, and put adapt_speed to on; the platform will accelerate and decelerate, as if you were using easing. The problem is that this easing is extremely difficult and tedious to undo with beziers.

I think it's easy to learn for the user because he/she can simply try it and observe the results.

Remember SuperTux is played mostly by children. Most players have no idea what beziers curves are, let alone C0/C1/G1 continuity; most people probably won't bother pressing alt at all, and those who will try will only notice that sometimes the other handle only rotates, and sometimes it goes to space and beyond, and they'll have no clue why.

The option of "adapt_speed" is already quite advanced, and most people will go without understanding what it does. The reason why I added that option and made it default to false, is to allow those who, like you, notice the movement feels wrong and are going to search for if they can change something somewhere, they'll likely try that option and notice it works as they please. As for the generic user, having the default option to false will avoid them wondering why there is some sort of "default easing" on their path, and why they can't undo it, and why the platform doesn't go at constant speed as the "Speed" property implies.

Fortunately, what we're talking about now are not game features, but rather editor features, that is, tools; we can easily change our mind later if it turned out that the option we chose was the wrong one. Plus, it is easy for one of us to simply keep a patch and apply it in our own version whenever we want, if we want it to work the way we want, and that won't break level compatibility. For the moment, I'd advise that we start with the simplest option (remember, people currently still don't have beziers at all 😛), and that we wait for users' feedback about it. If you are right, and those tools would indeed profit multiple users, then I'm all open to apply that patch at any moment. But given the complexity it would add, I'd prefer that we start simple and then work our way up according to what the users tell us.

[HybridDog]

I'm not sure if I mentioned this yet - there is no such thing as linear movement with adapt_speed enabled.

I haven't tested this before, sorry. When there are no bezier handles, it should move linearly to not break backwards compatibility and to avoid unintended easing. Perhaps it could be useful to have a case distinction between simple linear segments and segments with bezier handles. The "Speed" property could be hidden from the node configuration of a bezier segment.

[…] most people probably won't bother pressing alt at all […]

The current behaviour when pressing alt works incorrectly when the two segments have different duration, which is worse than not changing the length of the vector to the opposite handle in my opinion. The opposite handle goes to space only in extreme cases where the duration difference is very large; maybe the opposite handle could not be moved at all in these cases.

I'd prefer that we start simple and then work our way up according to what the users tell us.

I agree. I personally would prefer a case distinction between simple linear segments and segments with bezier handles, and hide the Speed and Easing options for the bezier segments, but constant speed bezier curves should be fine, too.

[Semphriss]

About treating linearity as a special case, it was what I originally did as of 3380c47, but I thought it would be annoying for the users who use beziers lightly to have a big difference between one short handle and no handles at all. At second thought, I have no idea how often this would happen in practice, and how likely users are to think that way.

I propose we merge what we agree on and resume discussion in a while, when we will have gotten some feedback from users.

Is it fine if we merge it as it is at the moment? Would you prefer that I disable the alt feature while we sort it out? Or any other temporary disable?

[HybridDog]

I think merging should be fine if you change the message Press CTRL to move Bezier handles to Press CTRL to move Bezier handles (experimental) and Press ALT to make Bezier handles continuous to Press ALT to move the opposite Bezier handle so that users know that Bezier segments may behave differently in the future and because holding ALT sometimes causes C1 and sometimes only G1 continuity.

If having no handles is treated as though the handles would have the same position as their parent node marker, the segment is a straight line with easing, which can probably also be achieved with a linear segment with Easing set. Therefore I think that in practice users should be fine with the case distinction.

[Semphriss]

@HybridDog Sorry for the delay - I completely disabled the alt feature for simplicity and I think it's ready for merge now. Do you want to review the code another time before merging?

[HybridDog]

I think the code is almost fine. I noticed two problems:

• I often see [WARNING] ../src/math/bezier.cpp:84 Attempt to get point on Bezier curve further than the end. messages when I play this level:
  level8.stl.zip
• When I erase a platform which has a path with Bezier markers (and the path is currently not visible in the editor) the Bezier markers suddenly appear:
  

[Semphriss]

The second issue was already existent before beziers, I intended to fix that with my editor revamp PR.

As for the first issue, I'll look into it, I'll push a patch in a bit.

[Semphriss]

It's fixed, it was due to the limited precision of floating points being challenged when the requested length is extremely close or equal to the maximum length. I added a check to make sure the overflow is substantial before printing a warning; it shouldn't print anything under normal circumstances anymore.

[HybridDog]

Thanks. It can be merged now.

[weluvgoatz]

I still have an issue with it. When I open up a level and place a platform, I can't seem to grab the path node (the tooltips for it don't even appear...) in some cases. Pressing CTRL or moving the bezier handles seem to fix it, but I shouldn't have to do this when I just want to grab on to the path node...

[weluvgoatz]

The game also freezes/hangs up/crashes when placing willowisps now.

[Semphriss]

Long overdue, but now it's been confirmed that everything has been addressed so far.