Cycles: Add Portal BSDF #114386

david494 · 2023-11-02T03:34:47+01:00

david494 commented

2023-11-02 03:34:47 +01:00

Initial implementation of a portal closure.

This will allow various non physical effects to be rendered directly in cycles without the need for compositing. In this implementation the portals also allow passes and AOVs through, which has more possibilities such as reflected and refracted passes and AOVs.

This is just a first stab at an implementation, it is at least missing:

documentation
examples of usage

The actual code is mostly a duplication of the transparent closure, with changes at some key points.

I am unsure about a couple of things:

using sd->P and sd->wi to set the new ray origin and direction, maybe this should be stored somewhere else?
changing state->isect.object in shade_surface.h to give the next ray the possibility to hit the same polygon again
the value of SH_NODE_BSDF_PORTAL
whether the new ray direction should be set as "incoming" or "outgoing". Incoming has the advantage of being immediately accessible on the geometry node, but somehow seems backwards for a portal node.
the copyright info on new files: I copied what was at the top of the files for the transparent closure.
sd->closure_transparent_extinction: This is currently shared with transparent closures, and I think that is the best solution, but mixing transparency and portals might lead to weird results.

edit: added example of camera filming its own screen
edit2: added obvious test image :)
edit3: added some of the scenes that I used for testing

Initial implementation of a portal closure. This will allow various non physical effects to be rendered directly in cycles without the need for compositing. In this implementation the portals also allow passes and AOVs through, which has more possibilities such as reflected and refracted passes and AOVs. --- This is just a first stab at an implementation, it is at least missing: - documentation - examples of usage The actual code is mostly a duplication of the transparent closure, with changes at some key points. I am unsure about a couple of things: - using `sd->P` and `sd->wi` to set the new ray origin and direction, maybe this should be stored somewhere else? - changing `state->isect.object` in `shade_surface.h` to give the next ray the possibility to hit the same polygon again - the value of `SH_NODE_BSDF_PORTAL` - whether the new ray direction should be set as "incoming" or "outgoing". Incoming has the advantage of being immediately accessible on the geometry node, but somehow seems backwards for a portal node. - the copyright info on new files: I copied what was at the top of the files for the transparent closure. - `sd->closure_transparent_extinction`: This is currently shared with transparent closures, and I think that is the best solution, but mixing transparency and portals might lead to weird results. edit: added example of camera filming its own screen edit2: added obvious test image :) edit3: added some of the scenes that I used for testing

portal_example.png

1.2 MiB

portal-portal.jpg

139 KiB

tv_scene_packed.blend

28 MiB

portal_scene_v003.blend

171 KiB

monitor-portal.jpg

398 KiB

👀 7 ❤️ 22 🎉 5 🚀 2

david494 added 1 commit 2023-11-02 03:35:02 +01:00

a52acf2cb2 Cycles: Add Portal BSDF

Initial implementation of a portal closure.

david494 added 1 commit 2023-11-02 04:39:05 +01:00

40efb0e0fc small cleanup

david494 added 2 commits 2023-11-02 21:50:04 +01:00

3776292a47 Merge branch 'main' into cycles-portal-bsdf

920b11554c Small formatting changes

Brecht Van Lommel commented

2023-11-03 18:12:01 +01:00

I am unsure about a couple of things:

using sd->P and sd->wi to set the new ray origin and direction, maybe this should be stored somewhere else?

changing state->isect.object in shade_surface.h to give the next ray the possibility to hit the same polygon again

Modifying these in the BSDF sampling code is problematic. We should handle it as a separate case like we do for subsurface scatting, since it's not a great fit to handle it as a regular BSDF.

  if (CLOSURE_IS_BSSRDF(sc->type)) {
    return subsurface_bounce(kg, state, sd, sc);
  }
  else if (CLOSURE_IS_PORTAL(sc->type) {
    return ...;
  }

the value of SH_NODE_BSDF_PORTAL

Better add this add the end of the list, after SH_NODE_MIX. Otherwise it reuses the value from an old node.

whether the new ray direction should be set as "incoming" or "outgoing". Incoming has the advantage of being immediately accessible on the geometry node, but somehow seems backwards for a portal node.

Outgoing seems more natural. I would name it "Position" and "Direction".

the copyright info on new files: I copied what was at the top of the files for the transparent closure.

Use:

/* SPDX-FileCopyrightText: 2023 Blender Authors
 *
 * SPDX-License-Identifier: Apache-2.0 */

sd->closure_transparent_extinction: This is currently shared with transparent closures, and I think that is the best solution, but mixing transparency and portals might lead to weird results.

That seems fine.

> I am unsure about a couple of things: > - using `sd->P` and `sd->wi` to set the new ray origin and direction, maybe this should be stored somewhere else? > - changing `state->isect.object` in `shade_surface.h` to give the next ray the possibility to hit the same polygon again Modifying these in the BSDF sampling code is problematic. We should handle it as a separate case like we do for subsurface scatting, since it's not a great fit to handle it as a regular BSDF. ``` if (CLOSURE_IS_BSSRDF(sc->type)) { return subsurface_bounce(kg, state, sd, sc); } else if (CLOSURE_IS_PORTAL(sc->type) { return ...; } ``` > - the value of `SH_NODE_BSDF_PORTAL` Better add this add the end of the list, after `SH_NODE_MIX`. Otherwise it reuses the value from an old node. > - whether the new ray direction should be set as "incoming" or "outgoing". Incoming has the advantage of being immediately accessible on the geometry node, but somehow seems backwards for a portal node. Outgoing seems more natural. I would name it "Position" and "Direction". > - the copyright info on new files: I copied what was at the top of the files for the transparent closure. Use: ``` /* SPDX-FileCopyrightText: 2023 Blender Authors * * SPDX-License-Identifier: Apache-2.0 */ ``` > - `sd->closure_transparent_extinction`: This is currently shared with transparent closures, and I think that is the best solution, but mixing transparency and portals might lead to weird results. That seems fine.

david494 added 1 commit 2023-11-03 22:35:38 +01:00

87b8b38f23 Switch "incoming" to "direction"; change portal node ID; update copyright info

david494 added 1 commit 2023-11-04 00:42:57 +01:00

3cef5438f0 Handle portal as a special case, and not as part of the normal bsdf sampling

david494 added 1 commit 2023-11-04 01:17:10 +01:00

aa8dec51f8 Updated tooltip for portal node

david494 added 1 commit 2023-11-04 01:20:46 +01:00

a9cb4ff3ae Merge branch 'main' into cycles-portal-bsdf

david494 commented

2023-11-04 01:31:35 +01:00

Thanks so much for taking the time to look at this! <3

I tried integrating all the comments. I still have some questions:

I left a part of the copied transparent closure code in bsdf_portal.h that I am not sure about. If we don't increment sd->num_closure_left if the path would terminate, I guess we just terminate one bounce earlier? I am not sure about the intention of this in the case of the transparent closure.
I still overwrite sd->P, sd->wi and sd->object, but now this is handled as a special case in shade_surface.h instead of the bsdf code, so it might be fine?
Since it is no longer handled as a bsdf, I removed the CLOSURE_BSDF_PORTAL_ID case from bsdf_sample(), but I am not sure if that function may be called in some other path, where handling a portal closure might be useful?

Also just wanted to say, I really enjoy reading the code, I am impressed and surprised how readable it is for a production renderer!

Thanks so much for taking the time to look at this! <3 I tried integrating all the comments. I still have some questions: - I left a part of the copied transparent closure code in `bsdf_portal.h` that I am not sure about. If we don't increment `sd->num_closure_left` if the path would terminate, I guess we just terminate one bounce earlier? I am not sure about the intention of this in the case of the transparent closure. - I still overwrite `sd->P`, `sd->wi` and `sd->object`, but now this is handled as a special case in `shade_surface.h` instead of the bsdf code, so it might be fine? - Since it is no longer handled as a bsdf, I removed the `CLOSURE_BSDF_PORTAL_ID` case from `bsdf_sample()`, but I am not sure if that function may be called in some other path, where handling a portal closure might be useful? Also just wanted to say, I really enjoy reading the code, I am impressed and surprised how readable it is for a production renderer!

david494 changed title from ~~WIP: Cycles: Add Portal BSDF~~ to Cycles: Add Portal BSDF

2023-11-04 02:40:52 +01:00

david494 commented

2023-11-04 12:39:29 +01:00

Regarding this:

I left a part of the copied transparent closure code in bsdf_portal.h that I am not sure about. If we don't increment sd->num_closure_left if the path would terminate, I guess we just terminate one bounce earlier? I am not sure about the intention of this in the case of the transparent closure.

I found this comment that seems relevant:

  /* Ray is to be terminated, but continue with transparent bounces and
   * emission as long as we encounter them. This is required to make the
   * MIS between direct and indirect light rays match, as shadow rays go
   * through transparent surfaces to reach emission too. */
  PATH_RAY_TERMINATE_AFTER_TRANSPARENT = (1U << 18U),

I think that explains the intention, and I believe we then want to have the same code for the portal closure, as it is also transparent for shadow rays, because it reuses the transparency and extinction logic. I updated the patch to do that.

And regarding this:

I still overwrite sd->P, sd->wi and sd->object, but now this is handled as a special case in shade_surface.h instead of the bsdf code, so it might be fine?

After testing some more, this is obviously not fine, so I updated the code to use a custom closure to hold the needed parameters.

Regarding this: > - I left a part of the copied transparent closure code in `bsdf_portal.h` that I am not sure about. If we don't increment `sd->num_closure_left` if the path would terminate, I guess we just terminate one bounce earlier? I am not sure about the intention of this in the case of the transparent closure. I found this comment that seems relevant: ``` /* Ray is to be terminated, but continue with transparent bounces and * emission as long as we encounter them. This is required to make the * MIS between direct and indirect light rays match, as shadow rays go * through transparent surfaces to reach emission too. */ PATH_RAY_TERMINATE_AFTER_TRANSPARENT = (1U << 18U), ``` I think that explains the intention, and I believe we then want to have the same code for the portal closure, as it is also transparent for shadow rays, because it reuses the transparency and extinction logic. I updated the patch to do that. And regarding this: > - I still overwrite sd->P, sd->wi and sd->object, but now this is handled as a special case in shade_surface.h instead of the bsdf code, so it might be fine? After testing some more, this is obviously not fine, so I updated the code to use a custom closure to hold the needed parameters.

david494 added 1 commit 2023-11-04 12:39:39 +01:00

2432e2e1db Use the same path termination logic in the portal closure as in the transparent closure

david494 added 1 commit 2023-11-04 19:26:15 +01:00

f9fcfaebfc Don't overwrite ShaderData fields in portal code

david494 added 1 commit 2023-11-05 23:52:56 +01:00

d71f95bf83 Add extinction if a portal closure exists already

david494 added 3 commits 2023-11-06 04:55:17 +01:00

b06c547806 Merge branch 'main' into cycles-portal-bsdf

86bc811e1c Fix throughput, was wrong in case of multiple closures

3c64fd10a3 conform to 'make format'

Brecht Van Lommel commented

2023-11-08 19:36:52 +01:00

This is looking good, but I need some time to look into the finer details for transparency, AOVs and path termination. Hopefully I will have time for it next week.

david494 commented

2023-11-08 23:14:22 +01:00

Just some of the things about AOVs that I thought about while writing this:

some passes are straightforward and obviously still correct through portals, such as light passes, UV, or cryptomattes. We also probably don't want to touch custom AOVs
position and normal pass are still in world space and not offset or rotated by the portal, which I think is the best solution?
Z pass is the problematic one, and does not take portals into account. It just calculates world position to camera position. To change that I think we would have to track path length, not just current ray length in the integrator state. (which might be useful even without portals, maybe even exposed on the lightpath node?)
similarly the mist pass is weird at the moment

All in all, as a user I think I would not be surprised that not all passes make sense through a portal, but of course it would be nice if we could do better.

Just some of the things about AOVs that I thought about while writing this: - some passes are straightforward and obviously still correct through portals, such as light passes, UV, or cryptomattes. We also probably don't want to touch custom AOVs - position and normal pass are still in world space and not offset or rotated by the portal, which I think is the best solution? - Z pass is the problematic one, and does not take portals into account. It just calculates world position to camera position. To change that I think we would have to track path length, not just current ray length in the integrator state. (which might be useful even without portals, maybe even exposed on the lightpath node?) - similarly the mist pass is weird at the moment All in all, as a user I think I would not be surprised that not all passes make sense through a portal, but of course it would be nice if we could do better.

david494 added 1 commit 2023-11-17 23:27:23 +01:00

ccecf25c91 added missing portal gpu shader, currently just rendered as transparency

david494 added 1 commit 2023-11-18 01:49:37 +01:00

aca098995d skip MIS for rays through portals

david494 added 1 commit 2023-11-19 21:44:59 +01:00

947b127cdb reworked shadow logic again

MIS has to be off for correct light through a portal, but that means
we can't have transparent shadows for portal closures, so we turn them
off for any shader that contains a portal.

Brecht Van Lommel commented

2024-01-29 18:19:17 +01:00

@LukasStockner, is this something you can review?

Brecht Van Lommel approved these changes 2024-01-29 18:24:26 +01:00

Dismissed

Brecht Van Lommel left a comment

Not a full review, just some quick comments from reading the code.

intern/cycles/kernel/closure/bsdf_portal.h Outdated

						
				@ -0,0 +27,4 @@

				    return;

				  }

				  if (sd->flag & SD_TRANSPARENT || sd->flag & SD_PORTAL) {

Brecht Van Lommel commented

2024-01-25 21:35:52 +01:00

More efficient: (sd->flag & (SD_TRANSPARENT|SD_PORTAL))

More efficient: `(sd->flag & (SD_TRANSPARENT|SD_PORTAL)) `

david494 commented

2024-01-29 22:37:46 +01:00

Yep, thanks. Also reads better :)

david494 marked this conversation as resolved

intern/cycles/kernel/closure/bsdf_portal.h Outdated

						
				@ -0,0 +28,4 @@

				  }

				  if (sd->flag & SD_TRANSPARENT || sd->flag & SD_PORTAL) {

				    sd->closure_transparent_extinction += weight;

Brecht Van Lommel commented

2024-01-29 18:22:41 +01:00

The initialization of this is getting complicated.

We better set this to zero in surface_shader_eval next to sd->num_closure, and then always use +=.

The initialization of this is getting complicated. We better set this to zero in `surface_shader_eval` next to `sd->num_closure`, and then always use `+=`.

david494 commented

2024-01-29 22:38:36 +01:00

That simplifies things nicely, I agree.

brecht marked this conversation as resolved

intern/cycles/kernel/film/data_passes.h Outdated

						
				@ -69,3 +69,3 @@

				    }

				    if (!(sd->flag & SD_TRANSPARENT) || kernel_data.film.pass_alpha_threshold == 0.0f ||

				    if (!(sd->flag & SD_TRANSPARENT || sd->flag & SD_PORTAL) ||

Brecht Van Lommel commented

2024-01-25 21:30:22 +01:00

(sd->flag & (SD_TRANSPARENT|SD_PORTAL))

`(sd->flag & (SD_TRANSPARENT|SD_PORTAL)) `

👍 1

david494 marked this conversation as resolved

intern/cycles/kernel/integrator/path_state.h Outdated

						
				@ -108,3 +108,3 @@

				  /* ray through transparent keeps same flags from previous ray and is

				   * not counted as a regular bounce, transparent has separate max */

				  if (label & LABEL_TRANSPARENT) {

				  if (label & LABEL_TRANSPARENT || label & LABEL_PORTAL) {

Brecht Van Lommel commented

2024-01-25 21:30:27 +01:00

(label & (LABEL_TRANSPARENT|LABEL_PORTAL))

`(label & (LABEL_TRANSPARENT|LABEL_PORTAL)) `

👍 1

david494 marked this conversation as resolved

intern/cycles/kernel/integrator/shade_shadow.h Outdated

						
				@ -52,2 +52,4 @@

				#  endif

				  /* Disable transparent shadows for portals */

				  if (shadow_sd->flag & SD_PORTAL) {

Brecht Van Lommel commented

2024-01-25 21:34:21 +01:00

This makes it so the presence of a single portal BSDF affects the whole material, regardless of its weight, which might for example be near zero. That doesn't seem right?

david494 commented

2024-01-29 23:04:43 +01:00

Yes, that one is a bit strange. I did not find a better place to disable transparent shadows for rays intersecting with portals, since I am reusing transparent_extinction, which is used in surface_shader_transparency(). I think we would either need to track portal transparency separately, or just treat portals as opaque (similar to refractions). The first option would add quite a bit of complexity I think, and the second one would get rid of one of my primary use cases (that passes and AOVs are visible through portals).

In my tests disabling transparent shadows for the whole material works out quite well, because with a weight=0 the shader is optimized out and we still get transparent shadows. If the transition is still problematic, one can still use "lightpath->is shadow ray" tricks to get transparent shadows (but this wasn't really necessary in my test scenes).

Yes, that one is a bit strange. I did not find a better place to disable transparent shadows for rays intersecting with portals, since I am reusing transparent_extinction, which is used in surface_shader_transparency(). I think we would either need to track portal transparency separately, or just treat portals as opaque (similar to refractions). The first option would add quite a bit of complexity I think, and the second one would get rid of one of my primary use cases (that passes and AOVs are visible through portals). In my tests disabling transparent shadows for the whole material works out quite well, because with a weight=0 the shader is optimized out and we still get transparent shadows. If the transition is still problematic, one can still use "lightpath->is shadow ray" tricks to get transparent shadows (but this wasn't really necessary in my test scenes).

david494 commented

2024-01-29 23:21:56 +01:00

I think I found an unrelated bug with transparent shadows during my tests: If there are any materials that contain transparent closures and have the "Transparent Shadows" checkbox set, all other materials in the scene that are transparent, but don't have "Transparent Shadows", are now completely opaque for shadows. I will report this as a separate bug.

edit: reported here #117652

I think I found an unrelated bug with transparent shadows during my tests: If there are any materials that contain transparent closures and have the "Transparent Shadows" checkbox set, all _other_ materials in the scene that are transparent, but don't have "Transparent Shadows", are now completely opaque for shadows. I will report this as a separate bug. edit: reported here https://projects.blender.org/blender/blender/issues/117652

intern/cycles/kernel/integrator/surface_shader.h Outdated

						
				@ -944,3 +944,3 @@

				    return one_spectrum();

				  }

				  else if (sd->flag & SD_TRANSPARENT) {

				  else if (sd->flag & SD_TRANSPARENT || sd->flag & SD_PORTAL) {

Brecht Van Lommel commented

2024-01-29 18:23:50 +01:00

(sd->flag & (SD_TRANSPARENT|SD_PORTAL))

`(sd->flag & (SD_TRANSPARENT|SD_PORTAL)) `

👍 1

david494 marked this conversation as resolved

Brecht Van Lommel dismissed brecht’s review 2024-01-29 18:24:53 +01:00

Reason:

Accidentally approved, did not mean to do it yet

david494 added 4 commits 2024-01-29 22:35:04 +01:00

adaa9e9f06 Merge branch 'main' into cycles-portal-bsdf

bf381f0e8e added new file to CMakeLists, so kernels can be build from source again

44e810b46c add missing ccl_private declaration for metal compatibility

2f378d807c simplify closure_transparent_extinction logic, plus small fixes based on review

david494 added 1 commit 2024-02-05 23:57:18 +01:00

4e6a92ce3f Fix merge conflicts in MIS logic, still skipping MIS for portals

david494 added 1 commit 2024-02-21 13:06:13 +01:00

536a5d42b0 Merge branch 'main', fixing conflict with BKE_node.hh

Brecht Van Lommel commented

2024-02-28 18:59:11 +01:00

@LukasStockner @weizhen is this something either of you could look at in the coming weeks, so we can get this into 4.2?

Lukas Stockner requested changes 2024-03-02 01:43:47 +01:00

Dismissed

Lukas Stockner left a comment

Code-wise, this seems mostly fine to me, just a few minor comments.

Also, OSL support is currently missing. I don't see any issues with adding it as a custom closure there - if you need any help with it, just let me know and I can take care of it.

There's also always the larger-picture view of whether the code complexity is warranted for a fairly niche feature, but I think in this case it's still reasonable.

Finally, one concern that I have is that we already have "light portals", so there's a danger of mixing things up (especially labels such as SD_PORTAL). I can't really think of a good alternative name for this BSDF though, so maybe we rename the existing Light Portals to e.g. Light Guides?

Code-wise, this seems mostly fine to me, just a few minor comments. Also, OSL support is currently missing. I don't see any issues with adding it as a custom closure there - if you need any help with it, just let me know and I can take care of it. There's also always the larger-picture view of whether the code complexity is warranted for a fairly niche feature, but I think in this case it's still reasonable. Finally, one concern that I have is that we already have "light portals", so there's a danger of mixing things up (especially labels such as `SD_PORTAL`). I can't really think of a good alternative name for this BSDF though, so maybe we rename the existing Light Portals to e.g. Light Guides?

intern/cycles/kernel/closure/bsdf.h Outdated

						
				@ -284,6 +285,7 @@ ccl_device_inline void bsdf_roughness_eta(const KernelGlobals kg,

				      *eta = 1.0f;

Lukas Stockner commented

2024-03-02 01:34:45 +01:00

(not specific to this line, but to the file):
Currently, CLOSURE_BSDF_PORTAL_ID isn't handled in the bsdf_sample switch statement. This works since it is handled by integrate_surface_portal instead, but we might want to make that explicit with a kernel_assert(false) case for it.

(not specific to this line, but to the file): Currently, `CLOSURE_BSDF_PORTAL_ID` isn't handled in the `bsdf_sample` switch statement. This works since it is handled by `integrate_surface_portal` instead, but we might want to make that explicit with a `kernel_assert(false)` case for it.

david494 commented

2024-04-08 02:19:11 +02:00

I agree. Added, with a small comment.

brecht marked this conversation as resolved

intern/cycles/kernel/closure/bsdf.h Outdated

						
				@ -482,3 +487,4 @@

				      break;

				    case CLOSURE_BSDF_TRANSPARENT_ID:

				    case CLOSURE_BSDF_PORTAL_ID:

				      eval = bsdf_transparent_eval(sc, sd->wi, wo, pdf);

Lukas Stockner commented

2024-03-02 01:32:47 +01:00

I'd prefer a separate bsdf_portal_eval just for consistency, even if it's the same stub as bsdf_transparent_eval.

I'd prefer a separate `bsdf_portal_eval` just for consistency, even if it's the same stub as `bsdf_transparent_eval`.

david494 commented

2024-04-08 02:20:04 +02:00

Yes, that makes it slightly more obvious if either has to change in the future.

brecht marked this conversation as resolved

intern/cycles/kernel/closure/bsdf_portal.h Outdated

						
				@ -0,0 +15,4 @@

				static_assert(sizeof(ShaderClosure) >= sizeof(PortalClosure), "PortalClosure is too large!");

				ccl_device void bsdf_portal_setup(ccl_private ShaderData *sd,

Lukas Stockner commented

2024-03-02 01:31:22 +01:00

I'm assuming that this is based on bsdf_transparent_setup.
That one is a bit of a special case, since all transparent closures are merged together.

In general, the usual pattern is that closure allocation and the sample weight check is handled in closure.h, and the setup function only handles parameter clamping etc.

That being said, the closures.h switch statement is massive, and I don't really see a problem with doing it this way...

I'm assuming that this is based on `bsdf_transparent_setup`. That one is a bit of a special case, since all transparent closures are merged together. In general, the usual pattern is that closure allocation and the sample weight check is handled in `closure.h`, and the setup function only handles parameter clamping etc. That being said, the `closures.h` switch statement is massive, and I don't really see a problem with doing it this way...

david494 commented

2024-04-08 02:25:03 +02:00

This is indeed based on the transparent BSDF code. While adding OSL support I also kept it close to the transparent code, which made the OSL changes very simple: osl_closure_portal_bsdf_setup just calls bsdf_portal_setup, so having this separated seems useful here.

This is indeed based on the transparent BSDF code. While adding OSL support I also kept it close to the transparent code, which made the OSL changes very simple: `osl_closure_portal_bsdf_setup` just calls `bsdf_portal_setup`, so having this separated seems useful here.

LukasStockner marked this conversation as resolved

intern/cycles/kernel/integrator/path_state.h Outdated

						
				@ -115,6 +115,10 @@ ccl_device_inline void path_state_next(KernelGlobals kg,

				      flag |= PATH_RAY_TERMINATE_ON_NEXT_SURFACE;

				    }

				    if (shader_flag & SD_PORTAL) {

Lukas Stockner commented

2024-03-02 01:37:55 +01:00

Shouldn't this check for label & LABEL_PORTAL instead? If an object is 50% diffuse and 50% portal, the diffuse bounces should still perform MIS.

Shouldn't this check for `label & LABEL_PORTAL` instead? If an object is 50% diffuse and 50% portal, the diffuse bounces should still perform MIS.

david494 commented

2024-04-08 02:30:53 +02:00

This is inside a check that already checks for either LABEL_PORTAL or LABEL_TRANSPARENT (so should not disable MIS on diffuse?), and based on my tests I believe we want to disable MIS for transparent as well if there is a portal closure in the mix. Otherwise the shadow behind a mix between portal and transparent does not converge to the correct result.

This is inside a check that already checks for either `LABEL_PORTAL` or `LABEL_TRANSPARENT` (so should not disable MIS on diffuse?), and based on my tests I believe we want to disable MIS for transparent as well if there is a portal closure in the mix. Otherwise the shadow behind a mix between portal and transparent does not converge to the correct result.

Lukas Stockner commented

2024-04-20 23:16:54 +02:00

Ah, right, I missed the previous check.

LukasStockner marked this conversation as resolved

intern/cycles/kernel/integrator/shade_surface.h Outdated

						
				@ -160,0 +166,4 @@

				  float sum_sample_weight = 0.0f;

				  for (int i = 0; i < sd->num_closure; i++) {

				    ccl_private const ShaderClosure *other_sc = &sd->closure[i];

Lukas Stockner commented

2024-03-02 01:26:30 +01:00

other_sc is misleading here, since unlike the MIS code the loop isn't skipping sc.

`other_sc` is misleading here, since unlike the MIS code the loop isn't skipping `sc`.

david494 commented

2024-04-08 02:33:25 +02:00

Yes, that is a little bit of unfortunate naming. I did not want to shadow the function parameter sc, but maybe that is fine as its immediately cast to pc and we can refer to pc everywhere after that.

Yes, that is a little bit of unfortunate naming. I did not want to shadow the function parameter `sc`, but maybe that is fine as its immediately cast to `pc` and we can refer to `pc` everywhere after that.

LukasStockner marked this conversation as resolved

intern/cycles/kernel/integrator/shade_surface.h Outdated

						
				@ -160,0 +192,4 @@

				  INTEGRATOR_STATE_WRITE(state, ray, dP) = differential_make_compact(sd->dP);

				#endif

				  INTEGRATOR_STATE_WRITE(state, path, throughput) *= pc->weight / sc->sample_weight *

Lukas Stockner commented

2024-03-02 01:26:06 +01:00

Might be a bit clearer to explicitly write

const float pick_pdf = sc->sample_weight / sum_sample_weight;
INTEGRATOR_STATE_WRITE(state, path, throughput) *= pc->weight / pick_pdf;

even though it means an extra division.

Might be a bit clearer to explicitly write ``` const float pick_pdf = sc->sample_weight / sum_sample_weight; INTEGRATOR_STATE_WRITE(state, path, throughput) *= pc->weight / pick_pdf; ``` even though it means an extra division.

david494 commented

2024-04-08 02:34:12 +02:00

I agree, looks more readable.

LukasStockner marked this conversation as resolved

david494 added 3 commits 2024-04-08 02:07:57 +02:00

dca52a8f24 Merge branch 'main' into cycles-portal-bsdf

5a10b83714 add OSL support

a9ea876595 small refactors based on review

david494 added 1 commit 2024-04-08 02:19:01 +02:00

c47609c0b6 added bsdf_portal_eval, separate from bsdf_transparent_eval now

david494 commented

2024-04-08 02:54:48 +02:00

Sorry, it took me a while to get back to this. Thanks for the review! All the code comments should be addressed.

Also, OSL support is currently missing. I don't see any issues with adding it as a custom closure there - if you need any help with it, just let me know and I can take care of it.

OSL should be working now as well. That was suprisingly straightforward! (If I indeed did it correctly)

There's also always the larger-picture view of whether the code complexity is warranted for a fairly niche feature, but I think in this case it's still reasonable.

I agree that it is niche, but seems to be fairly well isolated code-wise. At least there have been very few updates needed to keep the code up to date with main.

Finally, one concern that I have is that we already have "light portals", so there's a danger of mixing things up (especially labels such as SD_PORTAL). I can't really think of a good alternative name for this BSDF though, so maybe we rename the existing Light Portals to e.g. Light Guides?

Light portals are already named light portals in other renderers, so I think we should keep that name. I don't have a strong opinion on the name of this feature. This really just does a new raytrace anywhere, so maybe "Raytrace BSDF", or "Trace BSDF"? (Still ignoring that it is not really a BSDF). Having this as the user facing name might be okay, but in the code this seems very generic and might be even more confusing. I am open to other suggestions... maybe "Ray Portal"?

Sorry, it took me a while to get back to this. Thanks for the review! All the code comments should be addressed. > Also, OSL support is currently missing. I don't see any issues with adding it as a custom closure there - if you need any help with it, just let me know and I can take care of it. OSL should be working now as well. That was suprisingly straightforward! (If I indeed did it correctly) > There's also always the larger-picture view of whether the code complexity is warranted for a fairly niche feature, but I think in this case it's still reasonable. I agree that it is niche, but seems to be fairly well isolated code-wise. At least there have been very few updates needed to keep the code up to date with main. > Finally, one concern that I have is that we already have "light portals", so there's a danger of mixing things up (especially labels such as SD_PORTAL). I can't really think of a good alternative name for this BSDF though, so maybe we rename the existing Light Portals to e.g. Light Guides? Light portals are already named light portals in other renderers, so I think we should keep that name. I don't have a strong opinion on the name of this feature. This really just does a new raytrace anywhere, so maybe "Raytrace BSDF", or "Trace BSDF"? (Still ignoring that it is not really a BSDF). Having this as the user facing name might be okay, but in the code this seems very generic and might be even more confusing. I am open to other suggestions... maybe "Ray Portal"?

david494 requested review from Lukas Stockner 2024-04-09 10:40:34 +02:00

Lukas Stockner commented

2024-04-20 23:38:00 +02:00

Code looks good to me now.

In terms of name, I like "Ray Portal", that seems distinct enough from "Light Portal" and describes what is happening quite nicely.

Code looks good to me now. In terms of name, I like "Ray Portal", that seems distinct enough from "Light Portal" and describes what is happening quite nicely.

david494 added 1 commit 2024-04-21 00:34:28 +02:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

31bde20690 rename portal to ray portal

david494 commented

2024-04-21 00:42:28 +02:00

Thanks again, I now replaced all references to 'portal' with 'ray portal'.

(and updated the test files in the top post)

Thanks again, I now replaced all references to 'portal' with 'ray portal'. (and updated the test files in the top post)

Lukas Stockner reviewed 2024-04-21 18:32:02 +02:00

Lukas Stockner left a comment

Mostly LGTM now, just two nitpicks and one detail that I'm not certain about.

intern/cycles/kernel/integrator/shade_surface.h Outdated

						
				@ -160,0 +176,4 @@

				    return LABEL_NONE;

				  }

				  if (len_squared(sd->P - pc->P) > 1e-9f) {

Lukas Stockner commented

2024-04-21 18:28:21 +02:00

Nitpick: 1e-9f is really small, we usually use 1e-5f as an epsilon value.

Nitpick: `1e-9f` is really small, we usually use `1e-5f` as an epsilon value.

david494 commented

2024-04-21 20:18:05 +02:00

1e-9 is somewhat arbitrary, but my reasoning was that this is a squared distance. 1e-5 would still consider an offset of 0.001 the same point. I think a reasonable value here is dependent on scene scale.

Lukas Stockner commented

2024-04-22 01:57:07 +02:00

Hm, yeah, for squared distance it's fine I guess.

LukasStockner marked this conversation as resolved

intern/cycles/kernel/integrator/shade_surface.h

						
				@ -160,0 +189,4 @@

				  INTEGRATOR_STATE_WRITE(state, ray, tmin) = 0.0f;

				  INTEGRATOR_STATE_WRITE(state, ray, tmax) = FLT_MAX;

				#ifdef __RAY_DIFFERENTIALS__

				  INTEGRATOR_STATE_WRITE(state, ray, dP) = differential_make_compact(sd->dP);

Lukas Stockner commented

2024-04-21 18:27:38 +02:00

Not sure if this is the best way to handle it. This approach assumes that there's a 1:1 relationship between the shading position and the origin of the new ray, but that can't really be assumed.

The only realistic alternative I can think of is to set the differential to zero. Evaluating the real differential of the node input would be massive overkill.

@brecht, any opinions? I'm not 100% sure how important ray->dP currently is. Using sd->dP as an approximation is probably fine?

Not sure if this is the best way to handle it. This approach assumes that there's a 1:1 relationship between the shading position and the origin of the new ray, but that can't really be assumed. The only realistic alternative I can think of is to set the differential to zero. Evaluating the real differential of the node input would be massive overkill. @brecht, any opinions? I'm not 100% sure how important `ray->dP` currently is. Using `sd->dP` as an approximation is probably fine?

Brecht Van Lommel commented

2024-04-22 12:38:35 +02:00

I think just copying like this is ok. It's correct if the portal is a flat window and there is no rotation, which I guess is close enough as an approximation.

brecht marked this conversation as resolved

intern/cycles/kernel/osl/shaders/node_ray_portal_bsdf.osl

						
				@ -0,0 +5,4 @@

				#include "stdcycles.h"

				shader node_ray_portal_bsdf(color Color = 0.8,

				                            normal Normal = N,

Lukas Stockner commented

2024-04-21 18:29:25 +02:00

Nitpick: I don't think we need this input.

👍 1

david494 marked this conversation as resolved

Lukas Stockner commented

2024-04-21 18:32:47 +02:00

@blender-bot build +gpu

david494 added 1 commit 2024-04-21 20:20:28 +02:00

c07b11cc51 fix formatting

david494 added 1 commit 2024-04-21 20:25:42 +02:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

36a0540af0 remove unneeded Normal input for ray portal

Lukas Stockner approved these changes 2024-04-22 01:57:49 +02:00

Lukas Stockner left a comment

LGTM now. The only open question from my side is the differentials topic, but I'd be fine with keeping it as-is.

Brecht Van Lommel commented

2024-04-22 12:40:30 +02:00

@blender-bot build

Brecht Van Lommel approved these changes 2024-04-22 13:53:20 +02:00

Brecht Van Lommel left a comment

We will need some simple blend files for automated tests, see the existing ones for a template:
https://projects.blender.org/blender/blender-test-data/src/branch/main/render

Mainly would like to see tested:

Mix of transparency and portal
Multiple portals
Bump map seen through portal (to test ray differentials)

We will need some simple blend files for automated tests, see the existing ones for a template: https://projects.blender.org/blender/blender-test-data/src/branch/main/render Mainly would like to see tested: * Mix of transparency and portal * Multiple portals * Bump map seen through portal (to test ray differentials)

❤️ 1

Brecht Van Lommel added 2 commits 2024-04-29 10:57:34 +02:00

6a4ff4d1f6 Merge branch 'main' into HEAD

111fd79b85 Alphabetical ordering in add menu

Brecht Van Lommel added 1 commit 2024-04-29 11:35:53 +02:00

6d1d68f1b3 Update for BSDF inputs refactor

Brecht Van Lommel added 1 commit 2024-04-29 12:31:24 +02:00

61cd2cef5f Add tests

Brecht Van Lommel merged commit ee51f643b0 into main

2024-04-29 12:38:02 +02:00

Brecht Van Lommel referenced this issue from a commit

2024-04-29 12:38:03 +02:00

Cycles: Ray Portal BSDF

Brecht Van Lommel deleted branch cycles-portal-bsdf

2024-04-29 12:38:05 +02:00

xZaki commented

2024-04-30 04:34:34 +02:00

First-time contributor

For the future, since for most users this will be something they'll have to look at a guide every time they want to set it up, so I think a simple portal setup would be a good fit for a built-in asset.

Rincewind commented

2024-05-01 16:05:50 +02:00

First-time contributor

For the future, since for most users this will be something they'll have to look at a guide every time they want to set it up, so I think a simple portal setup would be a good fit for a built-in asset.

Yeah, I agree. It's really not easy to use. Maybe some easy-to-use nodes in the built-in essential would be nice.

Btw, while testing it I recognized the following:

It's awesome that we finally a way to create a mirror where we can mask the object in the mirror with Crytomatte!. But once you use a portal, there is now way to get the original object in the compositor in Crytomatteor. I think it would be handy to have an option, somehow also to get the original object.
Portal BSDF creats a Depth pass. That's cool. But it ignores Camera DOF. If somehow possible, a checkbox in PortalBSDF, to consider Camera DOF would be awesome. Currently, the only way is to use the Depth pass and create DOF in the compositor.

> For the future, since for most users this will be something they'll have to look at a guide every time they want to set it up, so I think a simple portal setup would be a good fit for a built-in asset. Yeah, I agree. It's really not easy to use. Maybe some easy-to-use nodes in the built-in essential would be nice. Btw, while testing it I recognized the following: - It's awesome that we finally a way to create a mirror where we can mask the object in the mirror with Crytomatte!. But once you use a portal, there is now way to get the original object in the compositor in Crytomatteor. I think it would be handy to have an option, somehow also to get the original object. - Portal BSDF creats a Depth pass. That's cool. But it ignores Camera DOF. If somehow possible, a checkbox in PortalBSDF, to consider Camera DOF would be awesome. Currently, the only way is to use the Depth pass and create DOF in the compositor.

Alaska commented

2024-05-01 17:46:37 +02:00

But once you use a portal, there is no way to get the original object in the compositor in Crytomatteor.

Shader AOVs can still be used to select the "Portal Object" rather than the objects visible through the portal

Portal BSDF ignores Camera DOF.

The Portal BSDF is interacting with DOF as expected in my testing. If you believe you have encountered a issue, please create a bug report.

> But once you use a portal, there is no way to get the original object in the compositor in Crytomatteor. Shader AOVs can still be used to select the "Portal Object" rather than the objects visible through the portal > Portal BSDF ignores Camera DOF. The Portal BSDF is interacting with DOF as expected in my testing. If you believe you have encountered a issue, please create a bug report.

Mark Faderbauer commented

2024-05-02 21:56:12 +02:00

First-time contributor

I think there might be a slightly weird interaction with Geometry Nodes points (which allow perfect spheres in Cycles) - this might not actually be relevant here, and rather something about the default rendering coordinate systems for points?
but just in principle:

For some reason, with points, the Direction ends up being incorrect

I think there might be a slightly weird interaction with Geometry Nodes points (which allow perfect spheres in Cycles) - this might not actually be relevant here, and rather something about the default rendering coordinate systems for points? but just in principle: ![image](/attachments/6bca9bf9-3a72-43de-b8fd-5943b68d1d2d) ![image](/attachments/ec08f957-8501-4c22-95ff-e5392a5891b9) For some reason, with points, the Direction ends up being incorrect

image.png

471 KiB

image.png

650 KiB

image.png

650 KiB

image.png

1.1 MiB

Mark Faderbauer commented

2024-05-03 00:18:44 +02:00

First-time contributor

Never mind, I think it's just generally the default. I thought the default for Direction is negative incoming, but apparently that's not the case

Calibrator commented

2024-05-05 23:15:09 +02:00

First-time contributor

I was thinking this could be useful for screens, but then the DOF becomes an issue. Afaik the distance through the portal is added to the DOF, meaning it's gonna be problematic to focus on both the screen and what's through it. Is it possible to add a toggle to not add the distance through the portal?

Alaska commented

2024-05-06 03:19:34 +02:00

@Calibrator , feature requests should be made on right click select.

As for your concern about DOF going through the portal, there are a few ways to work around it.

If you're using the depth pass in the compositor to add DOF to your scene, then you can use Shader AOVs to mark out the area where the portal is and not apply DOF to it. Or use the shader AOV to get the deapth from camera and replace that section of the depth pass.
If you're using in-camera DOF, then all you need to do is specify a direction the ray should go once it hits the portal. And as long as the surface the portal is on is in focus, then what's through it will look in focus (assuming you haven't done anything weird with the direction).

@Calibrator , feature requests should be made on [right click select](https://blender.community/c/rightclickselect/). As for your concern about DOF going through the portal, there are a few ways to work around it. 1. If you're using the depth pass in the compositor to add DOF to your scene, then you can use Shader AOVs to mark out the area where the portal is and not apply DOF to it. Or use the shader AOV to get the deapth from camera and replace that section of the depth pass. 2. If you're using in-camera DOF, then all you need to do is specify a direction the ray should go once it hits the portal. And as long as the surface the portal is on is in focus, then what's through it will look in focus (assuming you haven't done anything weird with the direction).

Calibrator commented

2024-05-06 03:32:25 +02:00

First-time contributor

@Calibrator , feature requests should be made on right click select.

As for your concern about DOF going through the portal, there are a few ways to work around it.

If you're using the depth pass in the compositor to add DOF to your scene, then you can use Shader AOVs to mark out the area where the portal is and not apply DOF to it. Or use the shader AOV to get the deapth from camera and replace that section of the depth map.

If you're using in-camera DOF, then all you need to do is specify a direction the ray should go once it hits the portal. And as long as the surface the portal is on is in focus, then what's through it will look in focus (assuming you haven't done anything weird with the direction).

Ah got it. I thought this would be the relevant place to mention it, but I see now. My bad.

As for the solutions I'll need time to figure this stuff out. This is the most complicated BSDF shader so far to be honest. In the meanwhile I hope the # 2 solution works. I believe that would be the method the average user would need to use the portal for a screen.

> @Calibrator , feature requests should be made on [right click select](https://blender.community/c/rightclickselect/). > > As for your concern about DOF going through the portal, there are a few ways to work around it. > > 1. If you're using the depth pass in the compositor to add DOF to your scene, then you can use Shader AOVs to mark out the area where the portal is and not apply DOF to it. Or use the shader AOV to get the deapth from camera and replace that section of the depth map. > 2. If you're using in-camera DOF, then all you need to do is specify a direction the ray should go once it hits the portal. And as long as the surface the portal is on is in focus, then what's through it will look in focus (assuming you haven't done anything weird with the direction). Ah got it. I thought this would be the relevant place to mention it, but I see now. My bad. As for the solutions I'll need time to figure this stuff out. This is the most complicated BSDF shader so far to be honest. In the meanwhile I hope the # 2 solution works. I believe that would be the method the average user would need to use the portal for a screen.

Brecht Van Lommel commented

2024-05-06 12:03:12 +02:00

Depth of field works by modifying the initial ray position and direction, there is no other information that is tracked that could be modified. For a computer screen, you'd discard the ray direction and create a completely new one, since what's shown on the screen does not depend on the viewing angle.

👍 1

Sign in to join this conversation.

No reviewers