blender/blender

Fix #101263: Vector pass wrongly saved in File Output #124522

Merged

Omar Emara merged 3 commits from OmarEmaraDev/blender:fix-101263-gpu into main

2024-07-11 16:08:36 +02:00

Omar Emara commented

2024-07-11 14:04:53 +02:00

Member

The vector pass and potentially other vectors that store 4 values are
stored wrongly, in particular, the last channel is ignored. To fix this
we identify if a vector pass is 4D and store the information in the
result meta data, then use this information to either save a 3D or a 4D
pass in the File Output node.

This is a partial fix for the GPU compositor only. The complete fix for
the CPU compositor will be submitted separately as it is not
straightforward and will likely require a refactor.

The vector pass and potentially other vectors that store 4 values are stored wrongly, in particular, the last channel is ignored. To fix this we identify if a vector pass is 4D and store the information in the result meta data, then use this information to either save a 3D or a 4D pass in the File Output node. This is a partial fix for the GPU compositor only. The complete fix for the CPU compositor will be submitted separately as it is not straightforward and will likely require a refactor.

Omar Emara added the

labels 2024-07-11 14:04:54 +02:00

Omar Emara added 1 commit 2024-07-11 14:05:03 +02:00

Fix #101263 : Vector pass wrongly saved in File Output 7e51568b76

The vector pass and potentially other vectors that store 4 values are
stored wrongly, in particular, the last channel is ignored. To fix this
we identify if a vector pass is 4D and store the information in the
result meta data, then use this information to either save a 3D or a 4D
pass in the File Output node.

This is a partial fix for the GPU compositor only. The complete fix for
the CPU compositor will be submitted separately as it is not
straightforward and will likely require a refactor.

Omar Emara requested review from Sergey Sharybin 2024-07-11 14:05:18 +02:00

Sergey Sharybin approved these changes 2024-07-11 15:17:56 +02:00

Sergey Sharybin reviewed 2024-07-11 15:19:02 +02:00

source/blender/render/intern/compositor.cc Outdated

						
				@ -432,0 +442,4 @@

				      return;

				    }

				    if (StringRef(render_pass->chan_id, 4) == "XYZW") {

Sergey Sharybin commented

2024-07-11 15:18:59 +02:00

Owner

Better to avoid creating StringRef with potentially more characters than there is in the actual chan_id, as discussed in the chat.
But don't need to do extra review iteraiton.

Better to avoid creating StringRef with potentially more characters than there is in the actual chan_id, as discussed in the chat. But don't need to do extra review iteraiton.

OmarEmaraDev marked this conversation as resolved

Omar Emara added 2 commits 2024-07-11 16:07:37 +02:00

Merge branch 'main' into fix-101263-gpu 72d47a9b58

Use null terminated constructor 4a9b096128

Omar Emara merged commit 57a6832b17 into main

2024-07-11 16:08:36 +02:00

Omar Emara deleted branch fix-101263-gpu

2024-07-11 16:08:39 +02:00

Omar Emara referenced this issue from a commit

2024-07-11 16:08:41 +02:00

Fix #101263: Vector pass wrongly saved in File Output

Bill-Spitzak reviewed 2024-07-11 22:15:51 +02:00

source/blender/render/intern/compositor.cc

						
				@ -429,6 +429,23 @@ class Context : public realtime_compositor::Context {

				        },

				        false);

				    RenderLayer *render_layer = RE_GetRenderLayer(render_result, view_layer->name);

Bill-Spitzak commented

2024-07-11 22:15:51 +02:00

Contributor

I think it would be better to write this like this:

if (RenderLayer *render_layer = RE_GetRenderLayer(...)) {
  if (RenderPass *render_pass = RE_pass_find_by_name(...)) {
    if (StringRef(render_pass->chan_id) == "XYZW") {
      meta_data.is_4d_vector = true;`
    }
  }
}
RE_Release(render);

I think it would be better to write this like this: ```Cpp if (RenderLayer *render_layer = RE_GetRenderLayer(...)) { if (RenderPass *render_pass = RE_pass_find_by_name(...)) { if (StringRef(render_pass->chan_id) == "XYZW") { meta_data.is_4d_vector = true;` } } } RE_Release(render); ```

Iliya Katushenock commented

2024-07-11 22:26:12 +02:00

Member

This is anti patter in blender, see: https://devtalk.blender.org/t/some-anti-patterns/26490

This is anti patter in blender, see: https://devtalk.blender.org/t/some-anti-patterns/26490

Bill-Spitzak commented

2024-07-12 00:02:01 +02:00

Contributor

If in fact those tests are never supposed to return null then the original code and mine both conflict with that document. It should just be written to assume they are not null.

In any case I think the version that contains only a single RE_Release call is greatly clearer and safer.

If in fact those tests are never supposed to return null then the original code and mine both conflict with that document. It should just be written to assume they are not null. In any case I think the version that contains only a single `RE_Release` call is greatly clearer and safer.

Iliya Katushenock commented

2024-07-12 12:44:59 +02:00

Member

This is not a referenced, so this always have to be checked. Idea to split if-statements is to make this more readable (avoid a lot of nestings) and make it more scalable because it's very common to add another one such check.

This is not a referenced, so this always have to be checked. Idea to split if-statements is to make this more readable (avoid a lot of nestings) and make it more scalable because it's very common to add another one such check.

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Automated Testing

Blender Asset Bundle

Code Documentation

Dependency Graph

Development Management

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Viewport & EEVEE

Virtual Reality

Asset Browser Project

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Viewport & EEVEE

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#124522

No description provided.