Cleanup: Update ntree_shader_disconnect_inactive_mix_branches for Rotation socket #109411

Closed

Iliya Katushenock wants to merge 4 commits from mod_moder:tmp_cleanup_mix_preprocessing into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Iliya Katushenock commented

2023-06-27 16:45:07 +02:00

Member

Rotation sockets was added into Mix node in 3957a1ad03.
A ntree_shader_disconnect_inactive_mix_branches optimization
function should be updated to take into account new sockets.

Rotation sockets was added into Mix node in 3957a1ad0349fc41b5f84f6a553cf18f5c598dcd. A `ntree_shader_disconnect_inactive_mix_branches` optimization function should be updated to take into account new sockets.

Iliya Katushenock added the

EEVEE & Viewport

labels 2023-06-27 16:45:07 +02:00

Iliya Katushenock added 1 commit 2023-06-27 16:45:15 +02:00

b185d8df65 init

Iliya Katushenock added this to the Nodes & Physics project 2023-06-27 16:48:15 +02:00

Iliya Katushenock requested review from Miguel Pozo 2023-06-27 16:48:25 +02:00

Iliya Katushenock requested review from Hans Goudey 2023-06-27 16:48:25 +02:00

Iliya Katushenock commented

2023-06-27 16:50:55 +02:00

Author

Member

I wonder if it would be better to change this to use socket names instead of indexes?

I wonder if it would be better to change this to use socket names instead of indexes?

Miguel Pozo requested changes 2023-06-27 17:24:55 +02:00

Miguel Pozo left a comment

Member

Looks good to me, although I personally wouldn't consider changing the if/elses for continues and switches an improvement.

Looks good to me, although I personally wouldn't consider changing the if/elses for continues and switches an improvement.

source/blender/nodes/shader/node_shader_tree.cc Outdated

						
				@ -1067,0 +1074,4 @@

				    const NodeShaderMix *storage = static_cast<NodeShaderMix *>(node->storage);

				    /*

				     * 0: factor,

				     * 1: fector-factor,

Miguel Pozo commented

2023-06-27 17:21:39 +02:00

Member

Typo

Typo

mod_moder marked this conversation as resolved

Miguel Pozo commented

2023-06-27 17:36:10 +02:00

Member

I wonder if it would be better to change this to use socket names instead of indexes?

That would be quite expensive performance-wise.
Maybe add an inline enum to the function, so the checks can become something like this:

shader_node_disconnect_inactive_mix_branch(ntree, node, factor, float_a, float_b, storage->clamp_factor);
/* Disconnect links from data_type-specific sockets that are not currently in use */
for (int i : IndexRange(socket_len)) {
  if(!ELEM(i, factor, float_a, float_b))
  {
    shader_node_disconnect_input(ntree, node, i);
  }
}

> I wonder if it would be better to change this to use socket names instead of indexes? That would be quite expensive performance-wise. Maybe add an inline enum to the function, so the checks can become something like this: ```Cpp shader_node_disconnect_inactive_mix_branch(ntree, node, factor, float_a, float_b, storage->clamp_factor); /* Disconnect links from data_type-specific sockets that are not currently in use */ for (int i : IndexRange(socket_len)) { if(!ELEM(i, factor, float_a, float_b)) { shader_node_disconnect_input(ntree, node, i); } } ```

Iliya Katushenock commented

2023-06-27 18:04:58 +02:00

Author

Member

@pragma37 Actually it should be cheap. At least in comparison with the shader. On the other hand, while I was writing comments, so as not to get confused, I was no longer happy with what I was doing.

@pragma37 Actually it should be cheap. At least in comparison with the shader. On the other hand, while I was writing comments, so as not to get confused, I was no longer happy with what I was doing.

Iliya Katushenock commented

2023-06-28 15:32:40 +02:00

Author

Member

although I personally wouldn't consider changing the if/elses for continues and switches an improvement.

To me, seems better

  default:
    BLI_assert_unreachable();

Instead of

} else {
  BLI_assert_unreachable();
}

> although I personally wouldn't consider changing the if/elses for continues and switches an improvement. To me, seems better ```Cpp default: BLI_assert_unreachable(); ``` Instead of ```Cpp } else { BLI_assert_unreachable(); } ```

Iliya Katushenock added 3 commits 2023-06-28 15:51:25 +02:00

d7ae227dd9 Merge branch 'main' into tmp_cleanup_mix_preprocessing

af5b58cd5a Merge branch 'main' into tmp_cleanup_mix_preprocessing

0f6fa9b1a0 progress

Iliya Katushenock commented

2023-06-28 15:51:53 +02:00

Author

Member

I have removed all indirectly related changes, I will send them as separate cleanups.

I have removed all indirectly related changes, I will send them as separate cleanups.

Iliya Katushenock requested review from Miguel Pozo 2023-06-28 15:52:01 +02:00

Iliya Katushenock commented

2023-06-28 16:35:20 +02:00

Author

Member

@pragma37 I split using a names of socket into separate PR: Refactoring: Explicit using of socket names to disconnect inactive mix branch

@pragma37 I split using a names of socket into separate PR: [Refactoring: Explicit using of socket names to disconnect inactive mix branch](https://projects.blender.org/blender/blender/pulls/109447)

Miguel Pozo commented

2023-06-30 21:19:29 +02:00

Member

I was going to test this before approving the changes, but it looks like the rotation mode is only available for geometry nodes, so there's no point in implementing this at the moment.

I was going to test this before approving the changes, but it looks like the rotation mode is only available for geometry nodes, so there's no point in implementing this at the moment.

Iliya Katushenock commented

2023-06-30 21:21:52 +02:00

Author

Member

Sometimes it may be necessary, but if you think so, probably now it was really hasty

Sometimes it may be necessary, but if you think so, probably now it was really hasty

Iliya Katushenock commented

2023-10-12 21:24:22 +02:00

Author

Member

https://projects.blender.org/blender/blender/pulls/113553

Iliya Katushenock closed this pull request

2023-10-12 21:24:24 +02:00

Iliya Katushenock deleted branch tmp_cleanup_mix_preprocessing

2023-10-12 21:24:37 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

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

2 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#109411

No description provided.