WIP: Single-frame job compiler #104194

k8ie · 2023-03-06T14:50:03+01:00

k8ie commented

2023-03-06 14:50:03 +01:00

First-time contributor

Same as #104189, aims to implement #104188. I accidentally closed the original PR by typing into the wrong window. I am very sorry about that.

Current task list:

Split the logic for the tiling into a function so that I can use it again for denoising
Denoising - I'll reuse the above mentioned tiling logic
The tiling logic math could use splitting into human-understandable variable names
Replace os with pathlib
Create helper variables for bpy.context.scene, etc.
Reject when not using Cycles
Fix merging being totally broken when using denoising (don't know the cause)
Don't hard-code EXR layer names, they might change between Blender versions
Don't hard-code the scene name, the scene can be renamed by the user

Same as #104189, aims to implement #104188. I accidentally closed the original PR by typing into the wrong window. I am very sorry about that. Current task list: - [x] Split the logic for the tiling into a function so that I can use it again for denoising - [x] Denoising - I'll reuse the above mentioned tiling logic - [x] The tiling logic math could use splitting into human-understandable variable names - [x] Replace `os` with `pathlib` - [x] Create helper variables for `bpy.context.scene`, etc. - [ ] Reject when not using Cycles - [ ] Fix merging being totally broken when using denoising (don't know the cause) - [ ] Don't hard-code EXR layer names, they might change between Blender versions - [ ] Don't hard-code the scene name, the scene can be renamed by the user

k8ie force-pushed single-frame from 46a83336f5 to b9aca93b54

2023-03-06 14:50:34 +01:00

Compare

k8ie added 1 commit 2023-03-06 14:56:18 +01:00

Use imperative property names 7c684cca0c

k8ie closed this pull request

2023-03-06 15:07:34 +01:00

k8ie reopened this pull request

2023-03-06 15:07:57 +01:00

Sybren A. Stüvel commented

2023-03-07 17:35:12 +01:00

No worries, things like that happen.

When you want me to review, be sure to remove the WIP: prefix from the title.

No worries, things like that happen. When you want me to review, be sure to remove the `WIP:` prefix from the title.

👍 1

k8ie added 1 commit 2023-03-13 19:59:19 +01:00

Render in tiles e6ef57d144

k8ie added 1 commit 2023-03-13 21:34:38 +01:00

Implement basic merging using the compositor 46a9db0bb3

k8ie added 1 commit 2023-03-14 00:03:24 +01:00

Finish up the tiling implementation 56a91bb7a2

k8ie added 1 commit 2023-03-15 17:45:52 +01:00

Fix tiling when using sizes over 50% e51a2aedff

k8ie added 1 commit 2023-03-16 13:36:12 +01:00

Fix the merge creating an empty layer in the merged image c87a5887e7

k8ie added 1 commit 2023-03-16 16:15:17 +01:00

Add support for denoising a2b191f127

k8ie commented

2023-03-16 16:45:33 +01:00

First-time contributor

Hi, I rewrote the chunking to be tile-based.

Overall this seems to work very well, path guiding works as expected. I still don't have overlap between tiles for adaptive sampling to work because I don't know how exactly I would go about cropping them before the merge.

There is one regression that bothers me quite a bit. I switched to using the compositor to merge the image. The node network I use looks like this:

The problem with this approach is that in order for the merge to happen, Blender has to keep all the files in memory. In a test with a 1080p image split into 100 tiles, the memory usage for a merge was ~4GB. 12GB for the same image but with denoising.

I thought about splitting the merging into separate tasks, but I don't know if that would be the ideal approach especially when you compare Blender's startup time with the amount of time it takes to merge two tiles.

There is another issue, and that is keeping the denoising data from the individual tiles. I'm not aware of any way to get the denoising data back into a multi-layer OpenEXR outside of the "File Output" node. This node allows adding an arbitrary amount of input sockets which will be stored in layers in the EXR file. I think I've read that the File Output node is a pretty hacky thing to use, so I would like to avoid using it if possible.

Hi, I rewrote the chunking to be tile-based. Overall this seems to work very well, path guiding works as expected. I still don't have overlap between tiles for adaptive sampling to work because I don't know how exactly I would go about cropping them before the merge. There is one regression that bothers me quite a bit. I switched to using the compositor to merge the image. The node network I use looks like this: ![image](/attachments/d8eb3a5f-446c-499c-8d31-0b832d0182cf) The problem with this approach is that in order for the merge to happen, Blender has to keep all the files in memory. In a test with a 1080p image split into 100 tiles, the memory usage for a merge was ~4GB. 12GB for the same image but with denoising. I thought about splitting the merging into separate tasks, but I don't know if that would be the ideal approach especially when you compare Blender's startup time with the amount of time it takes to merge two tiles. There is another issue, and that is keeping the denoising data from the individual tiles. I'm not aware of any way to get the denoising data back into a multi-layer OpenEXR outside of the "File Output" node. This node allows adding an arbitrary amount of input sockets which will be stored in layers in the EXR file. I think I've read that the File Output node is a pretty hacky thing to use, so I would like to avoid using it if possible.

image.png

890 KiB

k8ie added 1 commit 2023-03-16 16:55:05 +01:00

Fix using the wrong socket name 77013fb991

Sybren A. Stüvel commented

2023-03-20 12:12:53 +01:00

You made some nice progress!

About the memory usage, I think bpy.context.scene.render.use_crop_to_border = False is the culprit. If you set that to True, the image files will be much smaller, and only contain the rendered pixels. Of course you'll get a bit more complex flow with the merging, as you have to position each image correctly.

As for the Python code, there are some things to adjust.

Avoid copy-pasting bpy.context.... everywhere. It's harder to read, and it's also slower for Python to execute (but that doesn't matter too much in the grander scheme of things). If you need repeated access to the scene, just use scene = bpy.context.scene and then use scene. Same for accessing the node tree, etc.
Something similar for repeated calculations. There are a lot of repetitions of ${settings.tile_size} / 100, so just calculate that once, put the result in a variable, and use that instead. Having said that, I think it might be better to just input the desired number of tiles? The JS code can then determine how to split up the frame into that many tiles by itself. This avoids having to deal with remainders, like passing 33% as the tile size, but that leading to four tiles (3x 33% + 1x 1%).
Don't overwrite the renderer. If this approach can only work with Cycles, then just make the JavaScript reject the job when it uses any other renderer. How to do that gracefully is another thing, but silently switching render engines is not really nice to the artist.
Avoid string manipulation and os.path for handling paths. Restricting yourself to pathlib.Path will reduce the number of APIs you use, and make things simpler to read and understand. And it'll be less fragile. To make things a little shorter, you can use from pathlib import Path.

You made some nice progress! About the memory usage, I think `bpy.context.scene.render.use_crop_to_border = False` is the culprit. If you set that to `True`, the image files will be much smaller, and only contain the rendered pixels. Of course you'll get a bit more complex flow with the merging, as you have to position each image correctly. As for the Python code, there are some things to adjust. - Avoid copy-pasting `bpy.context....` everywhere. It's harder to read, and it's also slower for Python to execute (but that doesn't matter too much in the grander scheme of things). If you need repeated access to the scene, just use `scene = bpy.context.scene` and then use `scene`. Same for accessing the node tree, etc. - Something similar for repeated calculations. There are a lot of repetitions of `${settings.tile_size} / 100`, so just calculate that once, put the result in a variable, and use that instead. Having said that, I think it might be better to just input the desired number of tiles? The JS code can then determine how to split up the frame into that many tiles by itself. This avoids having to deal with remainders, like passing 33% as the tile size, but that leading to four tiles (3x 33% + 1x 1%). - Don't overwrite the renderer. If this approach can only work with Cycles, then just make the JavaScript reject the job when it uses any other renderer. How to do that gracefully is another thing, but silently switching render engines is not really nice to the artist. - Avoid string manipulation and `os.path` for handling paths. Restricting yourself to `pathlib.Path` will reduce the number of APIs you use, and make things simpler to read and understand. And it'll be less fragile. To make things a little shorter, you can use `from pathlib import Path`.

Sybren A. Stüvel requested changes 2023-03-20 12:16:37 +01:00

Sybren A. Stüvel left a comment

.

k8ie commented

2023-03-27 14:22:17 +02:00

First-time contributor

Hi, thanks for the feedback. You're probably right about the tile cropping and memory usage, fixing that is priority number one for me at the moment.

Sorry about the development being slower and slower, I have a bunch of IRL stuff that keeps me pretty busy, but rest assured, I'd still like to finish implementing this and get this merged.

Hi, thanks for the feedback. You're probably right about the tile cropping and memory usage, fixing that is priority number one for me at the moment. Sorry about the development being slower and slower, I have a bunch of IRL stuff that keeps me pretty busy, but rest assured, I'd still like to finish implementing this and get this merged.

Sybren A. Stüvel commented

2023-03-27 15:48:29 +02:00

Sorry about the development being slower and slower, I have a bunch of IRL stuff that keeps me pretty busy, but rest assured, I'd still like to finish implementing this and get this merged.

That's fine, you working at a slow pace on this is still infinitely faster than me not doing anything at all here ;-)

> Sorry about the development being slower and slower, I have a bunch of IRL stuff that keeps me pretty busy, but rest assured, I'd still like to finish implementing this and get this merged. That's fine, you working at a slow pace on this is still infinitely faster than me not doing anything at all here ;-)

❤️ 1

k8ie added 1 commit 2023-03-29 18:35:32 +02:00

Initial fix for the memory usage e349abb9c6

k8ie commented

2023-03-29 18:43:46 +02:00

First-time contributor

Alright, I finally got the memory usage under control. I still have a few things to do tho.

Split the logic for the tiling into a function so that I can use it again for denoising
Denoising - I'll reuse the above mentioned tiling logic
The tiling logic math could use splitting into human-understandable variable names
Replace os with pathlib
Create helper variables for bpy.context.scene, etc.
Reject when not using Cycles

Alright, I finally got the memory usage under control. I still have a few things to do tho. - Split the logic for the tiling into a function so that I can use it again for denoising - Denoising - I'll reuse the above mentioned tiling logic - The tiling logic math could use splitting into human-understandable variable names - Replace `os` with `pathlib` - Create helper variables for `bpy.context.scene`, etc. - Reject when not using Cycles

k8ie commented

2023-03-29 18:49:52 +02:00

First-time contributor

Oh, and looks like the merging doesn't work as expected for some tile sizes (interestingly enough, only the nice ones that fit the image exactly like 50%, 10%, etc.).

I guess I'll have to fix that first 😅

Oh, and looks like the merging doesn't work as expected for some tile sizes (interestingly enough, only the nice ones that fit the image exactly like 50%, 10%, etc.). - [x] I guess I'll have to fix that first 😅

k8ie added 1 commit 2023-03-29 21:02:22 +02:00

Fix merging for percentages without remainders 306e4a1fdf

k8ie added 1 commit 2023-03-29 21:10:22 +02:00

Show the tile size in the web UI 7e245730c5

Sybren A. Stüvel commented

2023-03-30 16:04:05 +02:00

Please don't track sub-tasks in a comment, just edit the PR description and put them there. That way Gitea recognises them (it shows like 3/5 in the PR list) and your local reviewer doesn't have to scroll up & down to find them ;-)

Please don't track sub-tasks in a comment, just edit the PR description and put them there. That way Gitea recognises them (it shows like `3/5` in the PR list) and your local reviewer doesn't have to scroll up & down to find them ;-)

k8ie commented

2023-03-30 17:02:32 +02:00

First-time contributor

Oh, ok, that makes sense, sorry 😅

Sybren A. Stüvel commented

2023-03-30 17:03:10 +02:00

Is faaaaaaaaaaaaaaain, we all learn all the time ;-)

👍 1

k8ie added 2 commits 2023-04-01 15:42:40 +02:00

Rename variable for more readability fd3c1ddf98

Make the tile placement logic a function c7d5d8fe9a

k8ie added 1 commit 2023-04-01 16:39:15 +02:00

Some more restructuring before implementing denoising adaa1ad0a0

k8ie added 1 commit 2023-04-01 17:22:16 +02:00

Final tweaks for denoising f6e18d53af

k8ie commented

2023-04-01 17:41:04 +02:00

First-time contributor

Alright, I think I'm pretty much done with the implementation of denoising.

There is an issue I ran into though. When combining the Denoising Albedos and Normals, the results are glitchy.
What's even weirder is that every time I run the composite, the result is glitched out in a different way.
I'll attach the two tiles I'm testing with.

This node network:

Results in this image:

For the complete context, this is the full node network I use:

Do you have any idea why this could be happening?

Alright, I think I'm pretty much done with the implementation of denoising. There is an issue I ran into though. When combining the Denoising Albedos and Normals, the results are glitchy. What's even weirder is that every time I run the composite, the result is glitched out in a different way. I'll attach the two tiles I'm testing with. This node network: ![image](/attachments/5bf06cab-6854-4663-be27-8b1365341d35) Results in this image: ![image](/attachments/afb4118b-9df1-48c9-9d7a-bdc679a1b358) For the complete context, this is the full node network I use: ![image](/attachments/c4723c40-eacd-4f0a-95cb-4de00bc6e18b) Do you have any idea why this could be happening?

image.png

447 KiB

r0c1_0670.exr

12 MiB

image.png

632 KiB

r0c0_0670.exr

9.8 MiB

image.png

1.0 MiB

k8ie added 1 commit 2023-04-04 15:55:22 +02:00

Replace os with pathlib 87581ebdb4

k8ie added 2 commits 2023-04-04 16:25:56 +02:00

Use helper variables for easier readability 22aedb6c3b

More readability 89935c5865

k8ie added 1 commit 2023-04-04 16:30:45 +02:00

Fix wrong variable definition 201ad62b83

Sybren A. Stüvel commented

2023-07-06 12:53:03 +02:00

Do you have any idea why this could be happening?

Sorry, I don't have the time to dive into this myself.

> Do you have any idea why this could be happening? Sorry, I don't have the time to dive into this myself.

k8ie commented

2023-07-07 19:05:17 +02:00

First-time contributor

@dr.sybren No worries ;)

Sybren A. Stüvel commented

2023-11-06 11:25:55 +01:00

Hey! How's the progress? The PR is still marked as 'work in progress', so I haven't looked at it in depth. Is there anything that's blocking you from removing the 'WIP' prefix?

k8ie commented

2023-11-06 13:32:57 +01:00

First-time contributor

Hey, I haven't worked on this for a while.

Last time I checked, I still had this happening when tiling denoising data together. I (hopefully) left enough info in that comment for anyone to be able to try it manually in Blender and reproduce the issue. The two EXR files used are included.

Unrelated - I think the layer names may have changed in EXRs somewhere between Blender versions, I'll need to account for that. (I'll add that to the original comment, should be pretty easy to do)

Otherwise I believe it should be pretty much ready. Just some cleanup here and there. Nothing huge other than that pesky tiling issue.

Hey, I haven't worked on this for a while. Last time I checked, I still had [this](https://projects.blender.org/studio/flamenco/pulls/104194#issuecomment-912820) happening when tiling denoising data together. I (hopefully) left enough info in that comment for anyone to be able to try it manually in Blender and reproduce the issue. The two EXR files used are included. Unrelated - I think the layer names may have changed in EXRs somewhere between Blender versions, I'll need to account for that. (I'll add that to the original comment, should be pretty easy to do) Otherwise I believe it should be pretty much ready. Just some cleanup here and there. Nothing huge other than that pesky tiling issue.

Sybren A. Stüvel commented

2023-11-06 14:39:27 +01:00

Last time I checked, I still had this happening when tiling denoising data together. I (hopefully) left enough info in that comment for anyone to be able to try it manually in Blender and reproduce the issue. The two EXR files used are included.

Did you file a bug report for Blender itself? If not, then this isn't going to get fixed. Blender developers (well, except me) aren't going to check this repository to find bugs. So if you haven't, please report it.

Unrelated - I think the layer names may have changed in EXRs somewhere between Blender versions, I'll need to account for that. (I'll add that to the original comment, should be pretty easy to do)

It would help to keep track of what still needs doing in the PR description. That way its current status is clear for anyone looking at it (without having to read through each comment in detail).

> Last time I checked, I still had [this](https://projects.blender.org/studio/flamenco/pulls/104194#issuecomment-912820) happening when tiling denoising data together. I (hopefully) left enough info in that comment for anyone to be able to try it manually in Blender and reproduce the issue. The two EXR files used are included. Did you file a bug report for Blender itself? If not, then this isn't going to get fixed. Blender developers (well, except me) aren't going to check this repository to find bugs. So if you haven't, please report it. > Unrelated - I think the layer names may have changed in EXRs somewhere between Blender versions, I'll need to account for that. (I'll add that to the original comment, should be pretty easy to do) It would help to keep track of what still needs doing in the PR description. That way its current status is clear for anyone looking at it (without having to read through each comment in detail).

k8ie commented

2023-11-06 15:40:05 +01:00

First-time contributor

I didn't file a bug report, I'll try doing that hopefully this week. Thanks for the pointer.

It would help to keep track of what still needs doing in the PR description.

Yup, that's what I meant 😉
It's added in there as a checkbox.

I didn't file a bug report, I'll try doing that hopefully this week. Thanks for the pointer. > It would help to keep track of what still needs doing in the PR description. Yup, that's what I meant 😉 It's added in there as a checkbox.

k8ie referenced this pull request from blender/blender

2023-12-07 08:39:26 +01:00

Composer's 'Translate' node shows random pixels when used in 3-component images #115877

k8ie commented

2023-12-07 08:40:28 +01:00

First-time contributor

@dr.sybren I have reported the issue upstream, we'll see what we learn 😉

k8ie force-pushed single-frame from 75f8068eae to 9048b23eb6

2024-07-17 20:13:59 +02:00

Compare

k8ie added 1 commit 2024-07-17 20:22:27 +02:00

Add job description 56754a4dcc

Sybren A. Stüvel commented

2024-07-22 11:43:55 +02:00

Heyyyy you're back!

Please be aware that @David-Zhang-10 is also working on a single-frame custom job type. Maybe it's a good idea to discuss things with him in #flamenco to see if you can join forces, or at least see if there are any differences between your approaches.

Heyyyy you're back! Please be aware that @David-Zhang-10 is also working on a single-frame custom job type. Maybe it's a good idea to discuss things with him in [#flamenco](https://blender.chat/channel/flamenco) to see if you can join forces, or at least see if there are any differences between your approaches.

❤️ 1

k8ie commented

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

First-time contributor

Oh, that's cool, I'll check his approach as well. Maybe I'll be able to assist in some way. Thanks for letting me know!

David Zhang commented

2024-08-20 02:36:51 +02:00

Hi! I am David, and I am also working on implementing distributed rendering for single images. Could you share how you find implementing denoising within Flamenco would be useful? How is it different from using the built-in denoising (not the denoising node)? Thank you so much for sharing!

k8ie commented

2024-08-20 19:19:42 +02:00

First-time contributor

@David-Zhang-10 Hi, I'm glad there is also someone else working on this. I'm not exactly a developer by trait nor a Blender expert.

Doing the denoising itself is not an issue, like you said, there is a node that does that.

The problem comes from using tiled rendering. To my knowledge you can't denoise the individual tiles and merge them afterwards. Like @dr.sybren mentioned here, the problem is that Blender doesn't currently export all data needed for denoising into a multilayer EXR file.

I still haven't had the time to dive into this again, my last experiment here was with Blender 4.0.1 ~8 months ago. Just for info the issue that caused the merging to screw up the image should be fixed because the default compositor in current versions is now full-frame although I haven't tested that.

@David-Zhang-10 Hi, I'm glad there is also someone else working on this. I'm not exactly a developer by trait nor a Blender expert. Doing the denoising itself is not an issue, like you said, there is a node that does that. The problem comes from using tiled rendering. To my knowledge you can't denoise the individual tiles and merge them afterwards. Like @dr.sybren mentioned [here](https://projects.blender.org/studio/flamenco/issues/104188#issuecomment-1218525), the problem is that Blender doesn't currently export all data needed for denoising into a multilayer EXR file. I still haven't had the time to dive into this again, my last experiment [here](https://projects.blender.org/studio/flamenco/pulls/104194#issuecomment-912820) was with Blender 4.0.1 ~8 months ago. Just for info the issue that caused the merging to screw up the image should be fixed because the default compositor in current versions is now full-frame although I haven't tested that.

This pull request is marked as a work in progress.

This branch is out-of-date with the base branch

View command line instructions.