Fix #106929: adjust normalization of zero vector in nodelink shader #107636

Merged

Leon Schittek merged 2 commits from merotosc/blender:bugfix/missing_nodelink_segment into main

2023-05-12 20:35:30 +02:00

Marino Toscano commented

2023-05-05 01:03:58 +02:00

Contributor

The nodelink shader did not check for zero vectors when normalizing the tangent.
This lead to an undefined vector and the first segment of the bezier curve couldn't be drawn.
Added a check for zero vectors and used the vector between the two endpoints for the tangent.

Issues: #106929, #89282

EDIT:
Added images for reference with before and after.
Long vertical lines with small x difference are still a bit buggy because the tangent is always horizontal like explained in Issue #106929.

The nodelink shader did not check for zero vectors when normalizing the tangent. This lead to an undefined vector and the first segment of the bezier curve couldn't be drawn. Added a check for zero vectors and used the vector between the two endpoints for the tangent. Issues: #106929, #89282 EDIT: Added images for reference with before and after. Long vertical lines with small x difference are still a bit buggy because the tangent is always horizontal like explained in Issue #106929.

nodelink_before_after.png

60 KiB

nodelink_vertical_issue.png

23 KiB

Marino Toscano added 1 commit 2023-05-05 01:04:05 +02:00

3e9e68cb59 Fix #106929 : adjust normalization of zero vector in nodelink shader

Marino Toscano referenced this pull request

2023-05-05 01:11:17 +02:00

Nodes: Broken noodle display with reroutes #106929

Leon Schittek added this to the Nodes & Physics project 2023-05-05 07:46:34 +02:00

Leon Schittek added the

label 2023-05-05 07:48:18 +02:00

Iliya Katushenock commented

2023-05-05 10:10:48 +02:00

Member

Image examples are very helpful.

Image examples are very helpful.

Leon Schittek reviewed 2023-05-05 21:38:52 +02:00

Leon Schittek left a comment

Member

Thanks for the fix! Works well in my tests.

You're right that the narrowing segments in some situations aren't exactly pretty. But that's a more general issue with how the node links are drawn since it doesn't ensure an even line thickness. You get similar artifacts when the link has strong curvature:

Thanks for the fix! Works well in my tests. --- You're right that the narrowing segments in some situations aren't exactly pretty. But that's a more general issue with how the node links are drawn since it doesn't ensure an even line thickness. You get similar artifacts when the link has strong curvature: ![not-pretty-node-link.png](/attachments/a7fe740f-33d7-457d-872d-86bb59c62318)

not-pretty-node-link.png

18 KiB

source/blender/gpu/shaders/gpu_shader_2D_nodelink_vert.glsl Outdated

						
				@ -86,3 +86,2 @@

				  /* tangent space at t */

				  tangent = normalize(tangent);

				  /* Tangent space at t. If the handle overlaps with the endpoint the vector between the two nodes

Leon Schittek commented

2023-05-05 21:12:12 +02:00

Member

I think "nodes" should be "sockets".

I know this is super nitpicky but I'm not sure "endpoint" and "handle" are good word choices in this part of the code. What about this:

  /* Tangent space at t. If the inner and outer control points overlap, the tangent is invalid.
   * Use the vector between the sockets instead. */

I think "nodes" should be "sockets". I know this is super nitpicky but I'm not sure "endpoint" and "handle" are good word choices in this part of the code. What about this: ``` /* Tangent space at t. If the inner and outer control points overlap, the tangent is invalid. * Use the vector between the sockets instead. */ ```

source/blender/gpu/shaders/gpu_shader_2D_nodelink_vert.glsl Outdated

						
				@ -88,1 +87,3 @@

				  tangent = normalize(tangent);

				  /* Tangent space at t. If the handle overlaps with the endpoint the vector between the two nodes

				   * is used as tangent. */

				  if (is_zero(tangent)) {

Leon Schittek commented

2023-05-05 21:14:28 +02:00

Member

I think this is a good place to use the ternary operator:

 tangent = is_zero(tangent) ? normalize(P3 - P0) : normalize(tangent);

I think this is a good place to use the ternary operator: ``` tangent = is_zero(tangent) ? normalize(P3 - P0) : normalize(tangent); ```

Marino Toscano commented

2023-05-05 22:37:32 +02:00

Author

Contributor

Good idea, much more cleaner. I changed it.

Good idea, much more cleaner. I changed it.

Marino Toscano added 1 commit 2023-05-05 22:33:24 +02:00

3cbfb22716 Fix #106929 : update comment with better wording and use ternary operator

Leon Schittek commented

2023-05-12 07:20:57 +02:00

Member

Thanks for updating the PR, @merotosc.

@HooglyBoogly, this works well and looks good to me. Do you agree, or do you think this should be fixed differently?
It would be nice to get this into 3.6 before it has to be rebased. I can take care of commiting / styling the commit message etc. if that helps.

Thanks for updating the PR, @merotosc. @HooglyBoogly, this works well and looks good to me. Do you agree, or do you think this should be fixed differently? It would be nice to get this into 3.6 before it has to be rebased. I can take care of commiting / styling the commit message etc. if that helps.

Leon Schittek requested review from Hans Goudey 2023-05-12 07:21:48 +02:00

Hans Goudey approved these changes 2023-05-12 14:48:33 +02:00

Hans Goudey commented

2023-05-12 14:49:06 +02:00

Member

Thanks for the PR! Yeah, this looks like a fine change to me.

I can take care of commiting / styling the commit message etc. if that helps.
That would be great, thanks!

Thanks for the PR! Yeah, this looks like a fine change to me. >I can take care of commiting / styling the commit message etc. if that helps. That would be great, thanks!

Leon Schittek merged commit a05a7988dc into main

2023-05-12 20:35:30 +02:00

Leon Schittek referenced this issue from a commit

2023-05-12 20:35:40 +02:00

Fix #106929: Gap in node links when sockets are aligned

Howard Trickey referenced this issue from a commit

2023-05-29 02:51:40 +02:00

Merge in orign/main:

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

4 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#107636

No description provided.