Fix #116418: Stroke direction wrong on curved surfaces #116539

Marcelo Mutzbauer · 2023-12-25T20:02:32+01:00

Marcelo Mutzbauer commented

2023-12-25 20:02:32 +01:00

Even though the brush rotation is computed as a 2D angle (based on the mouse movement), it currently gets applied by rotating the projected X direction around the the normal in 3D.
This patch ensures that rotation gets applied first, and only then does the motion direction get projected into the tangent plane.
A potential issue with the current approach is that the random perturbations will also be applied in 2D, but this seems to be fine from discussions with @JulienKaspar and @Sergey.
Also, there was an error where the location should probably be converted to world coordinates...

All these changes seem to fix the issue described in #116418.

I also noticed some minor "inconsistencies" with how the rotation is applied: For curve strokes, the direction of the curve corresponded to the upward direction of the brush. For view plane, area plane mapping and anchored strokes, the mouse motion indicated the downward direction of the brush. For compatibility, I tried my best to enforce the latter conventions throughout, but I'm not so confident about oversights.

Also, it seems like rake is being applied indiscriminately for textured brushes, regardless of whether the option is checked in the tool panel. This might have been introduced with commit 35071af (see the changes to paint_rake_rotation_active), but since unwinding the commit history that far breaks dependencies, I could not verify if this is the actual culprit. I did not make any changes to address this (separate) issue.

Even though the brush rotation is computed as a 2D angle (based on the mouse movement), it currently gets applied by rotating the projected X direction around the the normal in 3D. This patch ensures that rotation gets applied first, and only then does the motion direction get projected into the tangent plane. A potential issue with the current approach is that the random perturbations will also be applied in 2D, but this seems to be fine from discussions with @JulienKaspar and @Sergey. Also, there was an error where the location should probably be converted *to* world coordinates... All these changes seem to fix the issue described in #116418. I also noticed some minor "inconsistencies" with how the rotation is applied: For curve strokes, the direction of the curve corresponded to the upward direction of the brush. For view plane, area plane mapping and anchored strokes, the mouse motion indicated the downward direction of the brush. For compatibility, I tried my best to enforce the latter conventions throughout, but I'm not so confident about oversights. -------- Also, it seems like rake is being applied indiscriminately for textured brushes, regardless of whether the option is checked in the tool panel. This might have been introduced with commit 35071af (see the changes to `paint_rake_rotation_active`), but since unwinding the commit history that far breaks dependencies, I could not verify if this is the actual culprit. I did not make any changes to address this (separate) issue.

Marcelo Mutzbauer added 3 commits 2023-12-25 20:02:41 +01:00

0b1221c813 Fix #116418

59aed06648 Merge branch 'main' into fix_curved_rake

58287ec640 Minor rearrangements

Iliya Katushenock added this to the Sculpt, Paint & Texture project 2023-12-25 20:23:22 +01:00

Iliya Katushenock changed title from ~~Fix #116418~~ to Fix #116418: Stroke direction wrong on curved surfaces

2023-12-25 20:23:47 +01:00

Marcelo Mutzbauer added 1 commit 2023-12-25 23:11:03 +01:00

86c6ee9624 More intuitive(?) measurement of the rotation angle

Marcelo Mutzbauer reviewed 2023-12-25 23:13:08 +01:00

source/blender/blenkernel/intern/paint.cc Outdated

						
				@ -1357,3 +1357,3 @@

				    /* Limit how often we update the angle to prevent jitter. */

				    if (len_squared_v2(dpos) >= r * r) {

				      rotation = atan2f(dpos[0], dpos[1]);

				      rotation = atan2f(dpos[1], dpos[0]);

Marcelo Mutzbauer commented

2023-12-25 23:13:08 +01:00

Confusingly, atan2f takes y(rise) as its first argument, https://en.cppreference.com/w/c/numeric/math/atan2

I changed this calculation because it looks unintentional to me, but I can update the pull request to use the original way of measuring that angle.

On the other hand, changing conventions now is probably dangerous ...

Confusingly, `atan2f` takes y(rise) as its first argument, https://en.cppreference.com/w/c/numeric/math/atan2 I changed this calculation because it looks unintentional to me, but I can update the pull request to use the original way of measuring that angle. On the other hand, changing conventions now is probably dangerous ...

Marcelo Mutzbauer added 1 commit 2023-12-25 23:20:10 +01:00

04906729be Better naming

Julien Kaspar requested review from Julien Kaspar 2024-01-02 11:00:39 +01:00

Julien Kaspar added the

Module

Sculpt, Paint & Texture

label 2024-01-02 11:00:51 +01:00

Julien Kaspar requested review from Sergey Sharybin 2024-01-02 11:01:03 +01:00

Julien Kaspar approved these changes 2024-01-02 11:02:50 +01:00

Julien Kaspar left a comment

It fixes the issue as far as I tested. So on the user side this looks good to me :1

Waiting for another dev to take a look at this.

It fixes the issue as far as I tested. So on the user side this looks good to me :1 Waiting for another dev to take a look at this.

Sergey Sharybin commented

2024-01-02 14:25:45 +01:00

@JulienKaspar How about the part about the random perturbations applied in 2D? Does this behave good for you?

On a code side it seems fine, so I think it is more of agreeing on the functional level, updating the commit message to it (instead of leaving note that feedback is still needed) and committing the fix.

@JulienKaspar How about the part about the random perturbations applied in 2D? Does this behave good for you? On a code side it seems fine, so I think it is more of agreeing on the functional level, updating the commit message to it (instead of leaving note that feedback is still needed) and committing the fix.

Julien Kaspar commented

2024-01-02 14:39:21 +01:00

How about the part about the random perturbations applied in 2D? Does this behave good for you?

@Marcelo-Mutzbauer I think for this I need a test case. I can't tell if it behaves wrong or correct since it's random.
Do you mean the Random Mapping or Random option for the texture rotation?

Also, it seems like rake is being applied indiscriminately for textured brushes, regardless of whether the option is checked in the tool panel.

I can confirm this for Paint brushes. This seems to be because the tip shape can be set to be square, and in that case is using Rake for the stroke direction.
Unfortunately the texture is then automatically also using Rake ... that shouldn't be the case.
Maybe we can fix that separately.

> How about the part about the random perturbations applied in 2D? Does this behave good for you? @Marcelo-Mutzbauer I think for this I need a test case. I can't tell if it behaves wrong or correct since it's random. Do you mean the Random Mapping or Random option for the texture rotation? > Also, it seems like rake is being applied indiscriminately for textured brushes, regardless of whether the option is checked in the tool panel. I can confirm this for Paint brushes. This seems to be because the tip shape can be set to be square, and in that case is using Rake for the stroke direction. Unfortunately the texture is then automatically also using Rake ... that shouldn't be the case. Maybe we can fix that separately.

Marcelo Mutzbauer added 1 commit 2024-01-02 21:40:29 +01:00

1ae70f82ee Fix some rotation problems

Marcelo Mutzbauer added 1 commit 2024-01-02 21:41:01 +01:00

f3d0dacc32 Merge branch 'main' into fix_curved_rake

Marcelo Mutzbauer commented

2024-01-02 21:53:32 +01:00

Thank you all for taking a look!

The last version broke view plane mapping and some other things, hopefully that is now working again.
(
It seems like in main, the ups->rotation was supposed to represent the negation of the angle of 2D rotation, which might have been motivated by the fact that when sampling the brush texture, we need the inverse transform. The latest update makes it so that it just represents the 2D angle of the brush transform (even though I am second-guessing this maneuver now). It does not represent the rotation of the mouse movement anymore as it did in my first attempt.

I noticed some minor "inconsistencies" with how the rotation is applied: For curve strokes, the direction of the curve corresponded to the upward direction of the brush. For view plane, area plane mapping and anchored strokes, the mouse motion indicated the downward direction of the brush. For compatibility, I tried my best to enforce the latter conventions throughout, but I'm not so confident about oversights.
)

@JulienKaspar I was thinking of the Random option for texture rotation... For area plane mapping, there are two possible approaches here: (1) Either we randomly rotate the brush around the axis that points out of the monitor, or (2) we rotate it around the normal under the cursor. The patch currently implements approach (1). However, this only concerns the general direction of the brush texture, since we would still apply the brush using area plane mapping.
I am having a hard time coming up with a test case, but the difference mainly comes to bear when sculpting on parts of the mesh that are being looked at from a slanted direction (say the outer rim of the sphere).
I guess we could also expose it as an option, but we've got enough of those as things stand ...

Thank you all for taking a look! The last version broke view plane mapping and some other things, hopefully that is now working again. ( It seems like in main, the `ups->rotation` was supposed to represent the negation of the angle of 2D rotation, which might have been motivated by the fact that when sampling the brush texture, we need the inverse transform. The latest update makes it so that it just represents the 2D angle of the brush transform (even though I am second-guessing this maneuver now). It does not represent the rotation of the mouse movement anymore as it did in my first attempt. I noticed some minor "inconsistencies" with how the rotation is applied: For curve strokes, the direction of the curve corresponded to the upward direction of the brush. For view plane, area plane mapping and anchored strokes, the mouse motion indicated the downward direction of the brush. For compatibility, I tried my best to enforce the latter conventions throughout, but I'm not so confident about oversights. ) @JulienKaspar I was thinking of the Random option for texture rotation... For area plane mapping, there are two possible approaches here: (1) Either we randomly rotate the brush around the axis that points out of the monitor, or (2) we rotate it around the normal under the cursor. The patch currently implements approach (1). However, this only concerns the general direction of the brush texture, since we would still apply the brush using area plane mapping. I am having a hard time coming up with a test case, but the difference mainly comes to bear when sculpting on parts of the mesh that are being looked at from a slanted direction (say the outer rim of the sphere). I guess we could also expose it as an option, but we've got enough of those as things stand ...

Julien Kaspar commented

2024-01-03 10:57:08 +01:00

I was thinking of the Random option for texture rotation

@Marcelo-Mutzbauer In that case I think it's fine. SInce it's a random angle for each step of the stroke and it doesn't cause any issues with the texture mapping itself, it works well 👍

I never noticed the inverted texture rotation for Curve Strokes. Good catch!

I'd say on my end this PR is ready to go into main.

> I was thinking of the Random option for texture rotation @Marcelo-Mutzbauer In that case I think it's fine. SInce it's a random angle for each step of the stroke and it doesn't cause any issues with the texture mapping itself, it works well 👍 I never noticed the inverted texture rotation for Curve Strokes. Good catch! I'd say on my end this PR is ready to go into main.

Marcelo Mutzbauer added 1 commit 2024-01-03 15:34:39 +01:00

caf44e3c62 Forgot to add a file in my previous commit

Marcelo Mutzbauer commented

2024-01-03 15:40:13 +01:00

As suggested by @Sergey, I tried to update the PR description to remove the part about feedback.

ThinkingPolygons commented

2024-01-03 19:22:13 +01:00

First-time contributor

@Marcelo-Mutzbauer hi, no performance regressions with those changes?

Marcelo Mutzbauer commented

2024-01-03 20:18:41 +01:00

@ThinkingPolygons hi, I don't think there should be, but admittedly I did not do any meticulous benchmarking ... Did you notice something?
Basically all the major changes only affect areas that don't seem like hotpaths (in that they don't run per-vertex or anything like that), and the new computations shouldn't be significantly slower.

@ThinkingPolygons hi, I don't _think_ there should be, but admittedly I did not do any meticulous benchmarking ... Did you notice something? Basically all the major changes only affect areas that don't seem like hotpaths (in that they don't run per-vertex or anything like that), and the new computations shouldn't be significantly slower.

Julien Kaspar approved these changes 2024-01-04 15:01:56 +01:00

Julien Kaspar left a comment

I didn't notice any performance regressions. I'll update my review as approved.

Sergey Sharybin approved these changes 2024-01-09 11:42:05 +01:00

Sergey Sharybin left a comment

On the code side seems fine, the correctness of behavior I'd rely on Julien's testing.
Good work in catching inconsistencies and documenting the semantic of the brush rotation.

On the code side seems fine, the correctness of behavior I'd rely on Julien's testing. Good work in catching inconsistencies and documenting the semantic of the brush rotation.

Philipp Oeser referenced this pull request

2024-01-09 11:48:40 +01:00

Brush rotation doesn't update as frequent as Spacing #116736

Sergey Sharybin merged commit d16543a155 into main

2024-01-09 11:58:54 +01:00

Sergey Sharybin deleted branch fix_curved_rake

2024-01-09 11:58:55 +01:00

Sergey Sharybin referenced this issue from a commit

2024-01-09 11:58:57 +01:00

Fix #116418: Stroke direction wrong on curved surfaces

Kazashi Yoshioka referenced this issue from a commit

2024-01-12 19:10:53 +01:00